BIND 10 #3054: Add library validation to the hooks framework

[BIND 10 Development]

#3054: Add library validation to the hooks framework
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130731
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 '''callout_manager.h'''
 Please update CalloutManager() constructor description. It says that
 BadValue if thrown for parameter less than or equal 0. It doesn't throw
 for 0.

 It would be better to pass unsigned int and then no exception would be
 needed at all.

 '''hooks_manager.h'''
 loadLibrariesInternal(): please update the comment to briefly explain the
 differences between loadLibraries() and loadLibrariesInternal()

 validateLibraries(): error is comment: command => comma. Also, I would
 prefer the method to return a container with separate names. Right now we
 need to parse the string if we want to extract that information. In time,
 we may get a parameter that tells the server what to do if one library
 fails to load - refuse whole configuration or continue with other libs, or
 even mark a given library as "broken" and not about it that much. In any
 case, it is much easier to concatenate values from a vector than to parse
 a string with commas and spaces.

 General comment: We had a discussion about memory allocated in the
 libraries being freed after the library is unloaded. One of the issues was
 a CalloutHandle object for existing libraries. There are 2 places that
 store the CalloutHandle: Dhcpv6Srv::getCalloutHandle(const Pkt6Ptr& pkt)
 and Dhcpv4Srv::getCalloutHandle(const Pkt4Ptr& pkt). I think both should
 be extended to release current CalloutHandle object when passed with a
 NULL Pkt{4,6}Ptr. I see that the master 3054 was branched from is a bit
 old and doesn't include DHCPv6 code (which was merged couple days ago). I
 think a new ticket should be created to point out that
 getCalloutHandle(NULL) should be called before unloading old libraries.

 I hope that the documentation ticket will include an overview of all the
 classes. I must admit that it is still a bit confusing. I was thinking
 about something like we have for configuration parsers (see Developer's
 guide -> DHCPv6 server component -> Configuration parsers in DHCPv6). It
 doesn't have to be long, just give brief overview with pointer to detailed
 class descriptions.

 The proposed ChangeLog entry looks ok.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3054#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development