Opened 7 years ago

Closed 6 years ago

#3113 closed defect (fixed)

Hooks unit-tests fail when --enable-static-link is used

Reported by: tomek Owned by: stephen
Priority: medium Milestone: Sprint-DHCP-20130918
Component: dhcp Version:
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

There are several build problems when linking statically. Mukund fixed some of them (the patch is expected to be merged to master today), but the hooks tests fail.

I tried to debug it, but I do not understand the code well enough to find out what is wrong here. Here's what I managed to find out:

I focused on the first failing test: There are several build problems when linking statically. Mukund fixed some of them (the patch is expected to be merged to master today), but the hooks tests fail.

I tried to debug it, but I do not understand the code well enough to find out what is wrong here. Here's what I managed to find out:

I focused on the first failing test haveHooksManagerTest.LoadLibraries?. I see hook point "hookpt_two" being registered in the HooksCommonTestClass? ctor (common_test_class.h:47). Yet later when full_callout_library.cc tries to register a callout on it (see load() in full_callout_library.cc:120), the ServerHooks::getIndex() can't find such a hook name and throws exception (server_hooks.cc:128).

To reproduce the issue:

./configure --enable-generate-docs --with-gtest=/home/thomson/devel/gtest-1.6.0 --enable-logger-checks --enable-static-link
make -j8
cd src/lib/hooks
make check

Subtickets

Change History (12)

comment:1 Changed 6 years ago by marcin

The LoadLibraries test loads the full_callout_library.cc sucessfuly. The load function exposed by this library registers the two other callouts by calling the:

    handle.registerCallout("hookpt_two", hook_nonstandard_two);
    handle.registerCallout("hookpt_three", hook_nonstandard_three);

The first call eventually leads to the invocation of CalloutManager::registerCallout function.

The first sign of the error shows up on the beginning of this function where logger prints an odd error:

2013-08-30 11:05:51.351 DEBUG [bind10.hooks/31496] HOOKS_CALLOUT_REGISTRATION  @@Missing placeholder %1 for '1'@@ @@Missing placeholder %2 for 'hookpt_two'@@

which suggests that the logger state gets reset when dynamically loaded library calls back the b10-hooks library.

Similar symptom is a couple of lines down this function:

    int hook_index = ServerHooks::getServerHooks().getIndex(name);

The call to ServerHooks::getServerHooks() returns the instance of the singleton, which seems to be ''new instance'' (other than the one created by the unit test program) and it lacks the hooks which have been registered by the unit test program in lib/hooks/tests/common_test_class.h. As a consequence, the registered hooks are not found in this new instance of ServerHooks object and the test falls over.

comment:2 Changed 6 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130904
  • Owner set to UnAssigned
  • Status changed from new to reviewing

I made necessary changes and committed them to the branch.

This patch disables static linking of the unit tests which use hooks. The shared test libraries have to be linked with the b10-libraries, such as logger and hooks. If unit tests are linked statically with these libraries we end up with two copies of logger, hooks lib etc. These libs are stateful (have static global objects), so having distinct copies of them does not make any sense. In order to allow these objects to be shared between the unit test and the shared test libs, the linked b10-libs must be linked dynamically.

Having said that, I wonder if we should update the documentation to point out that the programs using hooks should be linked dynamically, unless the shared libraries implementing callouts do not depend on the stateful b10-libs.

6XX.	[bug]		marcin
	Disabled static linking of unit tests using hooks. This allows
	to share the b10 libraries between the unit tests and libraries
	implementing hooks. This prevents the unit tests failures
	when --enable-static-link configure flag is specified.
	(Trac #3113, git abcd)


comment:3 Changed 6 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130904 to Sprint-DHCP-20130918

comment:5 Changed 6 years ago by stephen

  • Owner changed from stephen to marcin

I've looked at the problem in more detail and have made some changes to the hooks code. As this is a bit orthogonal to the changes in 3113, I've put them in the branch trac3113_test. (This is branched from trac3113 at the point at which this ticket was put for review). A review is requested; if OK, I'll merge them into both 3113 and master.

The changes include an update to both the maintenance guide and the user guide. The changes to the former should be read first as it describes the problem and provides the context for the code changes. The update to the latter includes instructions for the user in writing code for a statically-linked BIND 10. That update also includes some corrections to the example code based on experience of Tom when trying to build it.

comment:6 Changed 6 years ago by stephen

  • Owner changed from marcin to tmark

comment:7 Changed 6 years ago by stephen

The hooks tests on trac3113_test were failing under OS X for two reasons:

  • Logging was not initialized properly.
  • The user libraries were not finding the dynamic BIND 10 libraries against which they were linked.

Both these are now fixed.

comment:8 Changed 6 years ago by tmark

  • Owner changed from tmark to stephen

I tested this under OS-x and Fedora 19, using both dynamic and static linking and it works.

One minor suggestion to code comments:

In hooks.h, !hooksStaticLinkInit is prefaced with this:

" This function - to be called by the user library code in its load() function
when running against a statically linked BIND 10 ..."

The commentary should be a bit more explicit and say something like "it MUST BE CALLED by..." and perhapsinclude a description of what ill will befall them otherwise. Maybe an @warning tag for doxygen? I just think it might be a bit more prominently pointed out.

I will leave that to your judgement. I do not need to see the ticket again.


comment:9 Changed 6 years ago by stephen

Suggested ChangeLog entry in light of the modifications described in comment:5

Modifications to fix problems running unit tests if they are
statically linked. This includes the provision of an initialization
function that must be called by user-written hooks libraries if
they are loaded by a statically-linked image.

comment:10 Changed 6 years ago by tmark

  • Owner changed from stephen to tmark

comment:11 Changed 6 years ago by tmark

  • Owner changed from tmark to stephen

The ChangeLog entry comment looks good. I forget that people read this, good place to put it.

Merge full speed-ahead then!

comment:12 Changed 6 years ago by stephen

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

Merged in commit 3d19eee4dbfabc7cf7ae528351ee9e3a334cae92

Note: See TracTickets for help on using tickets.