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

[BIND 10 Development]

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

 * owner:  stephen => tomek


Comment:

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

 > It would be better to pass unsigned int and then no exception would be
 needed at all.
 I had thought of that, but "ints" are used in the code and in some cases
 negative values have meaning.  I'm wary about mixing signed and unsigned
 values as it is all to easy to write code in which a negative number is
 promoted to an unsigned int without warning.

 '''hooks_manager.h'''
 > loadLibrariesInternal(): please update the comment to briefly explain
 the differences between loadLibraries() and loadLibrariesInternal()
 Isn't that covered by the group header for the xxxInternal methods which
 states:
 {{{
     /// The following methods correspond to similarly-named static
 methods,
     /// but actually do the work on the singleton instance of the
 HooksManager.
     /// See the descriptions of the static methods for more details.
 }}}


 > 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.
 The reason for this ticket is to perform the validation of libraries for
 #2981 (which allows them to be specified in the configuration).  The idea
 is that it is "all or nothing" - when the configuration is loaded, the
 libraries are checked.  If any fail validation, the configuration is
 rejected.  Supplying a parameter that allows partial loading is an idea,
 but to be consistent it would have to be applied to all configuration
 items.  At present, if any configuration checking fails, the entire commit
 is rejected.

 Currently, the only use case for a list of libraries that fail to validate
 is to include it the answer sent back to bindctl.  That requires the
 elements of the list to be concatenated into a single string.  However, on
 reflection your comment is valid - the set of failing libraries should be
 returned in some form of structure (vector) and it is the function that
 receives that data that should format the strings into an error message.
 The code has been changed.

 > 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 was planning to do that in #2981 but have created #3062 for it.  I'll
 need to complete that ticket before I complete #2981.

 > 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.
 I was going to do this as part of #2938, but instead have created #3063
 for the Doxygen form of the documentation.  I'll still update the wiki
 design document and will close #2938 at that point.

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