Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4062 closed defect (fixed)

fix debug of DaemonTest.createPIDFileOverwrite

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea1.0-beta
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
Total Hours: 0 Internal?: no

Description

DaemonTest?.createPIDFileOverwrite fails when run under gdb. In fact waitpid() fails with EINTR with some kernel supports for debugging.

Subtickets

Change History (14)

comment:1 Changed 4 years ago by fdupont

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

Take the occasion to make daemon unit tests to work when the environment variable KEA_PIDFILE_DIR is set.

Ready for review.

comment:2 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to fdupont

Reviewed commit 6e2f5ea1dbc5e6c31e12ab0928ef430a4ac40758

Looks OK, but by removing the definition of KEA_PIDFILE_DIR (in daemon_unittest.cc) if it exists, the test may have a side-effect in causing any tests now, or written in the future, that rely on the definition being present to fail.

If the current tests require the environment variable not to be defined, I suggest that the test fixture's constructor (or SetUp method) check if it is set and store the value in the fixture, restoring it in the destructor or TearDown method.

Last edited 4 years ago by stephen (previous) (diff)

comment:3 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Done (in the constructor and destructor). Ready for a second review round.

comment:4 follow-up: Changed 4 years ago by stephen

Reviewed commit 5853bacc9bc240c627d743fb30c636dfccce27eb

Would prefer "copy_" to be named to something like "env_copy" as being clearer.

The logic of resetting KEA_PIDFILE_DIR seems a bit overly complicated. As the tests do not set a value for that environment variable, simpler would be to reset it unconditionally if it was set on entry, i.e.

if (!env_copy.empty()) {
    setenv("KEA_PIDFILE_DIR", env_copy.c_str(), 1);
}

This would probably be faster: although "setenv" is called unconditionally, calls to "getenv()" and "copy" are avoided.

(In fact, to be complete, you could unconditionally unset it in an "else" clause.)

comment:5 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

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

  • Owner changed from fdupont to stephen

Replying to stephen:

Reviewed commit 5853bacc9bc240c627d743fb30c636dfccce27eb

Would prefer "copy_" to be named to something like "env_copy" as being clearer.

=> changed "copy_" into "env_copy_"

The logic of resetting KEA_PIDFILE_DIR seems a bit overly complicated.

=> I looked at the getenv()/setenv() code and my conclusion is we MUST avoid to change the current environment (i.e., call {un}setenv()) when it is not required.

As the tests do not set a value for that environment variable, simpler

=> simpler but not safer. The current code has a minimum side effect and is the simplest and safest at runtime.

comment:7 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 38f3d27792b4423e97c366fc4882db414aec1193

Change to env_copy_ is OK.

I looked at the getenv()/setenv() code and my conclusion is we MUST avoid to change the current environment (i.e., call {un}setenv()) when it is not required.

I don't understand this. Either the environment variable is set on entry to the test (in which case env_copy_ is set to a non-empty string) or it isn't (in which case env_copy_ is empty). The code:

if (env_copy_.empty()) {
    static_cast<void>(unsetenv("KEA_PIDFILE_DIR"));
} else {
    static_cast<void>(setenv("KEA_PIDFILE_DIR", env_copy_.c_str(), 1);
}

...leaves the environment variable unset if it was unset on entry or set to the correct value if it was set on entry. (Assuming, of course, that there were no errors in the unsetenv/setenv. But the current code does not have error handling for that anyway.) There does not seem anything unsafe about it.

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

  • Owner changed from fdupont to stephen

Replying to stephen:

I looked at the getenv()/setenv() code and my conclusion is we MUST avoid to change the current environment (i.e., call {un}setenv()) when it is not required.

I don't understand this. Either the environment variable is set on entry to the test (in which case env_copy_ is set to a non-empty string) or it isn't (in which case env_copy_ is empty).

=> My point is that we should not try to restore the environment when it was not changed.

The code:

if (env_copy_.empty()) {
    static_cast<void>(unsetenv("KEA_PIDFILE_DIR"));
} else {
    static_cast<void>(setenv("KEA_PIDFILE_DIR", env_copy_.c_str(), 1);
}

...leaves the environment variable unset if it was unset on entry or set to the correct value if it was set on entry.

=> it can perform unneeded action.

(Assuming, of course, that there were no errors in the unsetenv/setenv.

=> when you read the code it seems to be a strong assumption...

comment:9 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

At the moment if KEA_PIDFILE_DIR is set on entry, it is restored to that value on exit.

However, a future test may explicitly set KEA_PIDFILE_DIR since the variable is related to the code being tested. If that environment variable is not set when the test starts, in the current code nothing will happen on exit and it will be left set. The suggested alternative leaves the environment variable correctly (un)set regardless of what happens in the test.

My point is that we should not try to restore the environment when it was not changed.

The environment is being left in the same state on exit as it was on entry. Although that may involve a redundant setenv operation, that does not affect the environment as seen by the rest of the code.

comment:10 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

We are looping. I put you proposed restore code in the destructor...
Ready for a final review (or you may merge it as I reviewed your proposal).

comment:11 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0

per team meeting 7 oct, move to 1.0. Stephen will review.

comment:12 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits up to and including 5e9aa3def6b77c84ce7df754035e1298bf865893

All OK, please merge. The change is small and not user-visible: I don't think it needs a ChangeLog entry.

comment:13 Changed 4 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged, closing.

comment:14 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.