Opened 7 years ago

Closed 7 years ago

#1964 closed defect (fixed)

regressions due to --enable-logger-checks

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: Sprint-20120515
Component: logging Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 3.5 Internal?: no

Description

This feature added in #1892 caused at least a couple of troubles.
The code also seems to have other potential issues.

First, it breaks compile when the check is disabled due to the unused
function:
http://git.bind10.isc.org/~tester/builder/BIND10-systest/20120508204501-MacOS/logs/build.out

Second, a related unit test fails at least on some environments
including mine:

[ RUN      ] FormatterTest.mismatchedPlaceholders
log_formatter_unittest.cc:117: Failure
Death test: { isc::testutils::dontCreateCoreDumps(); Formatter(isc::log::INFO, s("Too many arguments in %1 %2"), this).arg("only one"); }
    Result: illegal return in test statement.
 Error msg: 
[  FAILED  ] FormatterTest.mismatchedPlaceholders (4474 ms)

Third, the way dontCreateCoreDumps() is implemented is broken:

  • we must not use an unnamed name space in a publicly shared header file.
  • liblog is a quite primitive library while testutils are generally considered utilities for a more advanced modules such as data source or the auth server. It's probably better to this set up in lib/util/unittests

Combining these, I'd suggest declaring this function in a normal named
namespace in lib/util as a non inline function and defining it in a
separate .cc file. It will also work as a solution to the
unused-function problem.

Unfortunately the defect is quite big and it won't be a one-line fix.
We need a branch and normal review process at the highest priority.

Pushing this to the current sprint.

Subtickets

Change History (7)

comment:1 Changed 7 years ago by jinmei

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

comment:2 Changed 7 years ago by jinmei

trac1964 is ready for review. Please review it at the highest
priority.

The background reason for the test failure was that it throws an
exception from the destructor. We'll eventually need to develop a
cleaner solution, but for the urgent fix I chose to use assert()
(which fails) instead of throwing an exception.

comment:3 Changed 7 years ago by jinmei

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

comment:4 in reply to: ↑ description ; follow-up: Changed 7 years ago by muks

  • Owner changed from UnAssigned to jinmei

Hi jinmei

Replying to jinmei:

This feature added in #1892 caused at least a couple of troubles.
The code also seems to have other potential issues.

First, it breaks compile when the check is disabled due to the unused
function:
http://git.bind10.isc.org/~tester/builder/BIND10-systest/20120508204501-MacOS/logs/build.out

Second, a related unit test fails at least on some environments
including mine:

[ RUN      ] FormatterTest.mismatchedPlaceholders
log_formatter_unittest.cc:117: Failure
Death test: { isc::testutils::dontCreateCoreDumps(); Formatter(isc::log::INFO, s("Too many arguments in %1 %2"), this).arg("only one"); }
    Result: illegal return in test statement.
 Error msg: 
[  FAILED  ] FormatterTest.mismatchedPlaceholders (4474 ms)

The way we use log strings is such that we can only detect broken cases (after all .arg() calls are processed) in the destructor. Before that, every formatter can have a valid format string still.
Throwing from the destructor is known, but that caused a abort in our environments which was checked in the unit tests. It seems that this is not always the case.

Third, the way dontCreateCoreDumps() is implemented is broken:

  • we must not use an unnamed name space in a publicly shared header file.

It used a static function for it initially, and the unnamed namespace was committed during the review. It's less than clean, but putting it in testutils brought a circular library dependency issue (see comments in #1892).

  • liblog is a quite primitive library while testutils are generally considered utilities for a more advanced modules such as data source or the auth server. It's probably better to this set up in lib/util/unittests

*nod*. I didn't see lib/util/unittests, after finding testutils.

Combining these, I'd suggest declaring this function in a normal named
namespace in lib/util as a non inline function and defining it in a
separate .cc file. It will also work as a solution to the
unused-function problem.

I've checked the code changes you have made and they look fine.

comment:5 Changed 7 years ago by muks

I guess clang++ has -Wunused-function on by default. Anyway, putting it in lib/util/unittests as shared text is indeed the best approach.

comment:6 in reply to: ↑ 4 Changed 7 years ago by jinmei

Replying to muks:

Combining these, I'd suggest declaring this function in a normal named
namespace in lib/util as a non inline function and defining it in a
separate .cc file. It will also work as a solution to the
unused-function problem.

I've checked the code changes you have made and they look fine.

Thanks for the prompt review. I've just merged the fix branch.
Closing the ticket.

Giving this ticket an estimation of 3 at my discretion.

comment:7 Changed 7 years ago by jinmei

  • Estimated Difficulty changed from 0 to 3
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 3.5
Note: See TracTickets for help on using tickets.