Opened 4 years ago

Closed 4 years ago

#4310 closed defect (fixed)

double errors with LOG_FATAL()

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0.5
Total Hours: 0.5 Internal?: no

Description

Cf. #4307. I propose to introduce a new SAFE_LOG_FATAL() for early calls to LOG_FATAL() which can raise an exception including in a top-level exception handler.

Subtickets

Change History (11)

comment:1 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. Ready for review (BTW it can't be done with a macro).

comment:3 Changed 4 years ago by hschempf

  • Milestone changed from Kea1.1 to Kea-proposed

This ticket needs to be reviewed by the team before it's added to 1.1, sending it to Kea-proposed.

comment:4 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

Per 3/3/ team meeting, accept 1.1. Already in review, estimate = .5d.

comment:5 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:6 follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

I reviewed f693bc433c4330679335df29ade9f5b95ff6d403.

A couple of comments/questions:

  1. I'd recommend adding a SAFE_LOG_FATAL to avoid multiple try-catch clauses (you suggested that in the ticket and it seems fine idea),
  2. Please update the Kea coding guidelines https://kea.isc.org/wiki/CodingGuidelines#LogStatementSafety and mention that LOG statements are not exception safe and in some cases it may be required to surround them with a try-catch. Alternatively, suggest to use SAFE_LOG_FATAL if you introduce one
  3. Did we verify that exceptions emitted by log statements don't leak through the main function in D2?
  4. This change requires ChangeLog entry.

comment:7 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 0 to 0.5
  • Total Hours changed from 0 to 0.5

comment:8 in reply to: ↑ 6 Changed 4 years ago by fdupont

Replying to marcin:

I reviewed f693bc433c4330679335df29ade9f5b95ff6d403.

A couple of comments/questions:

  1. I'd recommend adding a SAFE_LOG_FATAL to avoid multiple try-catch clauses (you suggested that in the ticket and it seems fine idea),

=> unfortunately it is not possible the way LOG_XXX macros are written.

  1. Please update the Kea coding guidelines https://kea.isc.org/wiki/CodingGuidelines#LogStatementSafety and mention that LOG statements are not exception safe and in some cases it may be required to surround them with a try-catch.

=> good idea!

  1. Did we verify that exceptions emitted by log statements don't leak through the main function in D2?

=> there is no log statement (and no Coverity reports about it) but I agree the d2 main should be made more robust.

  1. This change requires ChangeLog entry.

=> yes, what about: "Protected DHCP server main() routines against errors raised from logger calls in the error handler (aka double errors). (Trac #4310, git xxx)"?

comment:9 follow-up: Changed 4 years ago by marcin

I gather you decided to not update D2? That's fine, but if there are any potential issues and improvements that could be made, can you open a ticket for it?

Other changes look good.

comment:10 in reply to: ↑ 9 Changed 4 years ago by fdupont

Replying to marcin:

I gather you decided to not update D2? That's fine, but if there are any potential issues and improvements that could be made, can you open a ticket for it?

=> #4307 should address D2 issues.

Other changes look good.

=> merging.

comment:11 Changed 4 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.