[4297] Addressed review comments:

 - returnining different values in callout_params_library.cc
 - returned values are now described for getParameter
 - corrected index syntax comment in getParameter
 - added missing include
 - hooks_user.dox updated
 - hooks unit test moved, so it is now next to other hooks tests

src/lib/cc/Makefile.am

@@ -13,4 +13,9 @@ libkea_cc_la_LIBADD += $(BOOST_LIBS)
 
 libkea_cc_la_LDFLAGS = -no-undefined -version-info 1:0:0
 
+# Since data.h is now used in the hooks interface, it needs to be
+# installed on target system.
+libkea_cc_includedir = $(pkgincludedir)
+libkea_cc_include_HEADERS = data.h
+
 CLEANFILES = *.gcno *.gcda

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -251,10 +251,11 @@ HooksLibrariesParser::build(ConstElementPtr value) {
     // Initialize.
     libraries_.clear();
     changed_ = false;
-    ConstElementPtr parameters;
 
     // This is the new syntax.  Iterate through it and get each map.
     BOOST_FOREACH(ConstElementPtr library_entry, value->listValue()) {
+        ConstElementPtr parameters;
+
         // Is it a map?
         if (library_entry->getType() != Element::map) {
             isc_throw(DhcpConfigError, "hooks library configuration error:"
@@ -321,7 +322,7 @@ HooksLibrariesParser::build(ConstElementPtr value) {
     // configuration is changed).
 
     // We no longer rely on this. Parameters can change. And even if the
-    // parameters stay the same, they could point to a files that could
+    // parameters stay the same, they could point to files that could
     // change.
     vector<string> current_libraries = HooksManager::getLibraryNames();
     if (current_libraries.empty() && libraries_.empty()) {

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -1336,6 +1336,71 @@ TEST_F(ParseConfigTest, invalidSyntaxHooksLibraries) {
         "Error text returned from parse failure is " << error_text_;
 }
 
+// Check that some parameters may have configuration parameters configured.
+TEST_F(ParseConfigTest, HooksLibrariesParameters) {
+    // Check that no libraries are currently loaded
+    vector<string> hooks_libraries = HooksManager::getLibraryNames();
+    EXPECT_TRUE(hooks_libraries.empty());
+
+    // Configuration string.  This contains an invalid library which should
+    // trigger an error in the "build" stage.
+    const std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
+                                                       CALLOUT_LIBRARY_2,
+                                                       CALLOUT_PARAMS_LIBRARY);
+
+    // Verify that the configuration fails to parse. (Syntactically it's OK,
+    // but the library is invalid).
+    const int rcode = parseConfiguration(config);
+    ASSERT_EQ(0, rcode);
+
+    // Check that the parser recorded the names.
+    HookLibsCollection libraries;
+    bool changed = false;
+    hooks_libraries_parser_->getLibraries(libraries, changed);
+    EXPECT_TRUE(changed);
+    ASSERT_EQ(3, libraries.size());
+    EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first);
+    EXPECT_EQ(CALLOUT_LIBRARY_2, libraries[1].first);
+    EXPECT_EQ(CALLOUT_PARAMS_LIBRARY, libraries[2].first);
+
+    // Also, check that the third library has its parameters specified.
+    // They were set by setHooksLibrariesConfig. The first has no
+    // parameters, the second one has an empty map and the third
+    // one has actual parameters.
+    EXPECT_FALSE(libraries[0].second);
+    EXPECT_TRUE(libraries[1].second);
+    ASSERT_TRUE(libraries[2].second);
+
+    // Ok, get the partameter for the third library.
+    ConstElementPtr params = libraries[2].second;
+
+    // It must be a map.
+    ASSERT_EQ(Element::map, params->getType());
+
+    // This map should have 3 parameters:
+    // - svalue (and will expect its value to be "string value")
+    // - ivalue (and will expect its value to be 42)
+    // - bvalue (and will expect its value to be true)
+    ConstElementPtr svalue = params->get("svalue");
+    ConstElementPtr ivalue = params->get("ivalue");
+    ConstElementPtr bvalue = params->get("bvalue");
+
+    // There should be no extra parameters.
+    EXPECT_FALSE(params->get("nonexistent"));
+
+    ASSERT_TRUE(svalue);
+    ASSERT_TRUE(ivalue);
+    ASSERT_TRUE(bvalue);
+
+    ASSERT_EQ(Element::string, svalue->getType());
+    ASSERT_EQ(Element::integer, ivalue->getType());
+    ASSERT_EQ(Element::boolean, bvalue->getType());
+
+    EXPECT_EQ("string value", svalue->stringValue());
+    EXPECT_EQ(42, ivalue->intValue());
+    EXPECT_EQ(true, bvalue->boolValue());
+}
+
 /// @brief Checks that a valid, enabled D2 client configuration works correctly.
 TEST_F(ParseConfigTest, validD2Config) {
 
@@ -2156,72 +2221,6 @@ TEST_F(ParseConfigTest, validRelayInfo6) {
     EXPECT_THROW(parser->build(json_bogus2), DhcpConfigError);
 }
 
-// Check that some parameters may have configuration parameters configured.
-TEST_F(ParseConfigTest, HooksLibrariesParameters) {
-    // Check that no libraries are currently loaded
-    vector<string> hooks_libraries = HooksManager::getLibraryNames();
-    EXPECT_TRUE(hooks_libraries.empty());
-
-    // Configuration string.  This contains an invalid library which should
-    // trigger an error in the "build" stage.
-    const std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
-                                                       CALLOUT_LIBRARY_2,
-                                                       CALLOUT_PARAMS_LIBRARY);
-
-    // Verify that the configuration fails to parse. (Syntactically it's OK,
-    // but the library is invalid).
-    const int rcode = parseConfiguration(config);
-    ASSERT_EQ(0, rcode);
-
-    // Check that the parser recorded the names.
-    HookLibsCollection libraries;
-    bool changed;
-    hooks_libraries_parser_->getLibraries(libraries, changed);
-    EXPECT_TRUE(changed);
-    ASSERT_EQ(3, libraries.size());
-    EXPECT_EQ(CALLOUT_LIBRARY_1, libraries[0].first);
-    EXPECT_EQ(CALLOUT_LIBRARY_2, libraries[1].first);
-    EXPECT_EQ(CALLOUT_PARAMS_LIBRARY, libraries[2].first);
-
-    // Also, check that the third library has its parameters specified.
-    // They were set by setHooksLibrariesConfig. The first has no
-    // no parameters, the second one has an empty map and the third
-    // one has actual parameters.
-    EXPECT_FALSE(libraries[0].second);
-    EXPECT_TRUE(libraries[1].second);
-    ASSERT_TRUE(libraries[2].second);
-
-    // Ok, get the partameter for the third library.
-    ConstElementPtr params = libraries[2].second;
-
-    // It must be a map.
-    ASSERT_EQ(Element::map, params->getType());
-
-    // This map should have 3 parameters:
-    // - svalue (and will expect its value to be "string value")
-    // - ivalue (and will expect its value to be 42)
-    // - bvalue (and will expect its value to be true)
-    ConstElementPtr svalue = params->get("svalue");
-    ConstElementPtr ivalue = params->get("ivalue");
-    ConstElementPtr bvalue = params->get("bvalue");
-
-    // There should be no extra parameters.
-    EXPECT_FALSE(params->get("nonexistent"));
-
-    ASSERT_TRUE(svalue);
-    ASSERT_TRUE(ivalue);
-    ASSERT_TRUE(bvalue);
-
-    ASSERT_EQ(Element::string, svalue->getType());
-    ASSERT_EQ(Element::integer, ivalue->getType());
-    ASSERT_EQ(Element::boolean, bvalue->getType());
-
-    EXPECT_EQ("string value", svalue->stringValue());
-    EXPECT_EQ(42, ivalue->intValue());
-    EXPECT_EQ(true, bvalue->boolValue());
-}
-
-
 // There's no test for ControlSocketParser, as it is tested in the DHCPv4 code
 // (see CtrlDhcpv4SrvTest.commandSocketBasic in
 // src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc).

src/lib/hooks/hooks_manager.h

@@ -173,7 +173,7 @@ public:
 
     /// @brief Return list of loaded libraries with its parameters.
     ///
-    /// Returns the names of the loaded libraries and its parameters.
+    /// Returns the names of the loaded libraries and their parameters.
     ///
     /// @return List of loaded libraries (names + parameters)
     static HookLibsCollection getLibraryInfo();

src/lib/hooks/hooks_user.dox

@@ -168,7 +168,7 @@ to namespaces.
 As the names suggest, "load" is called when a library is loaded and
 "unload" called when it is unloaded.  (It is always guaranteed that
 "load" is called: "unload" may not be called in some circumstances,
-e.g. if the system shuts down abnormally.)  These functions are the
+e.g., if the system shuts down abnormally.)  These functions are the
 places where any library-wide resources are allocated and deallocated.
 "load" is also the place where any callouts with non-standard names
 (names that are not hook point names) can be registered:
@@ -344,7 +344,7 @@ an argument as an @c int and the callout attempted to retrieve it as a
 @c long, an exception would be thrown even though any value that can
 be stored in an @c int will fit into a @c long.  This restriction also
 applies the "const" attribute but only as applied to data pointed to by
-pointers, e.g. if an argument is defined as a @c char*, an exception will
+pointers, e.g., if an argument is defined as a @c char*, an exception will
 be thrown if an attempt is made to retrieve it into a variable of type
 @c const @c char*.  (However, if an argument is set as a @c const @c int,
 it can be retrieved into an @c int.)  The documentation of each hook
@@ -434,7 +434,7 @@ status (see @ref hooksdgNextStep) was provided by
 a boolean flag called "Skip". However, since it only allowed to either continue
 or skip the next processing step and was not extensible to other decisions,
 setSkip(bool) call was replaced with a setStatus(enum) in Kea 1.0. This
-new approach is extensible. If we decide to add new results (e.g. WAIT
+new approach is extensible. If we decide to add new results (e.g., WAIT
 or RATELIMIT), we will be able to do so without changing the API again.
 
 If you have your hooks libraries that take advantage of skip flag, migrating
@@ -473,7 +473,7 @@ if (status == CalloutHandle::NEXT_STEP_SKIP) {
 @subsubsection hooksdgCalloutContext Per-Request Context
 
 Although the Kea modules can be characterized as handling a single
-packet at a time - e.g. the DHCPv4 server receives a DHCPDISCOVER packet,
+packet at a time - e.g., the DHCPv4 server receives a DHCPDISCOVER packet,
 processes it and responds with an DHCPOFFER, this may not always be true.
 Future developments may have the server processing multiple packets
 simultaneously, or to suspend processing on a packet and resume it at
@@ -962,7 +962,7 @@ only shared pointers have the correct behavior for the copy operation.)
 As briefly mentioned in @ref hooksdgExampleCallouts, the standard is for
 callouts in the user library to have the same name as the name of the
 hook to which they are being attached.  This convention was followed
-in the tutorial, e.g.  the callout that needed to be attached to the
+in the tutorial, e.g.,  the callout that needed to be attached to the
 "pkt4_receive" hook was named pkt4_receive.
 
 The reason for this convention is that when the library is loaded, the
@@ -1026,7 +1026,7 @@ a hook point.  Although it is likely to be rare for user code to need to
 do this, there may be instances where it make sense.
 
 To register multiple callouts on a hook, just call
-@c LibraryHandle::registerCallout multiple times on the same hook, e.g.
+@c LibraryHandle::registerCallout multiple times on the same hook, e.g.,
 
 @code
     libhandle.registerCallout("pkt4_receive", classify);
@@ -1184,7 +1184,7 @@ may be present, but will not affect the context values set by a library's
 callouts.
 
 Configuring multiple libraries just requires listing the libraries
-as separate elements of the hooks-libraries configuration element, e.g.
+as separate elements of the hooks-libraries configuration element, e.g.,
 
 @code
 "Dhcp4": {
@@ -1311,14 +1311,14 @@ specify it as an empty map. The third library is more interesting. It has five
 parameters specified. The first one called 'mail' is a string. The second one
 is an integer and the third one is boolean. Fourth and fifth parameters are
 slightly more complicated as they are a list and a map respectively. JSON
-structures can be nested if necessary, e.g. you can have a list, which contains
+structures can be nested if necessary, e.g., you can have a list, which contains
 maps, maps that contain maps that contain other maps etc. Any valid JSON
-structure can be repesented. One important limitation here is that the top
+structure can be represented. One important limitation here is that the top
 level "parameters" structure must either be a map or not present at all.
 
 Those parameters can be accessed in load() method. Passed isc::hooks::LibraryHandle
 object has a method called getParameter that returns an instance of
-isc::data::ConstElementPtr or NULL if there was no parameter specified. This pointer
+isc::data::ConstElementPtr or null pointer if there was no parameter specified. This pointer
 will point to an object derived from isc::data::Element class. For detailed
 explanation how to use those objects, see isc::data::Element class.
 
@@ -1338,12 +1338,12 @@ Here's a brief overview of how to use those elements:
    x->find("klingon"), x->contains("french"), x->size()
 
 Keep in mind that the user can structure his config file incorrectly.
-Remember to check if the structure has expected type before using type specific
+Remember to check if the structure has the expected type before using type specific
 method. For example calling stringValue on IntElement will throw an exception.
 You can do this by calling isc::data::Element::getType.
 
 Here's an example that obtains all of the parameters specified above.
-If you want to get nested elemented, Element::get(index) and Element::find(name)
+If you want to get nested elements, Element::get(index) and Element::find(name)
 will return ElementPtr, which can be iterated in similar manner.
 
 @code
@@ -1367,7 +1367,7 @@ int load(LibraryHandle& handle) {
 
     // In the following examples safety checks are omitted for clarity.
     // Make sure you do it properly similar to mail example above
-    // or you risk dereferencing NULL pointer or at least throwing
+    // or you risk dereferencing null pointer or at least throwing
     // an exception!
 
     // Integer handling example

src/lib/hooks/libinfo.h

@@ -13,6 +13,7 @@
 
 #include <string>
 #include <vector>
+#include <utility>
 
 namespace isc {
 namespace hooks {

src/lib/hooks/library_handle.cc

@@ -85,17 +85,17 @@ LibraryHandle::getParameter(const std::string& name) {
     }
 
     // Some indexes have special meaning:
-    // - 0 - pre-user library callout
-    // - 1..numlib - indexes for actual libraries
-    // INT_MAX post-user library callout
+    // * 0 - pre-user library callout
+    // * 1..numlib - indexes for actual libraries
+    // * INT_MAX - post-user library callout
 
-    // Try to find appropriate parameter. May return NULL
+    // Try to find appropriate parameter. May return null pointer
     isc::data::ConstElementPtr params = libinfo[index - 1].second;
     if (!params) {
         return (isc::data::ConstElementPtr());
     }
 
-    // May return NULL if there's no parameter.
+    // May return null pointer if there's no parameter.
     return (params->get(name));
 }
 

src/lib/hooks/library_handle.h

@@ -149,7 +149,7 @@ public:
     /// - x = getParameter("users") will return an instance of ListElement.
     ///   Its content can be accessed with the following methods:
     ///   x->size(), x->get(index)
-    /// - x = getParameter("watch-list") will return an instance of isc::data::MapElement.
+    /// - x = getParameter("header") will return an instance of isc::data::MapElement.
     ///   Its content can be accessed with the following methods:
     ///   x->find("klingon"), x->contains("french"), x->size()
     ///
@@ -166,6 +166,8 @@ public:
     /// unittests in data_unittests.cc.
     ///
     /// @param name text name of the parameter.
+    /// @return ElementPtr representing requested parameter (may be null, if
+    ///         there is no such parameter.)
     isc::data::ConstElementPtr
     getParameter(const std::string& name);
 

src/lib/hooks/tests/callout_params_library.cc

@@ -48,7 +48,7 @@ int load(LibraryHandle& handle) {
 
     if (string_elem->getType() != Element::string) {
         // Parameter is specified, but it's not a string.
-        return (1);
+        return (2);
     }
 
     std::string str = string_elem->stringValue();
@@ -58,43 +58,43 @@ int load(LibraryHandle& handle) {
         // This library is used for testing, so it expects exact value of the
         // parameter. Normal library would likely use whatever value user
         // specified.
-        return (1);
+        return (3);
     }
 
     // Integer handling example
     if (!int_elem) {
         // Parameter was not specified at all.
-        return (1);
+        return (4);
     }
 
     if (int_elem->getType() != Element::integer) {
         // Parameter is specified, but it's not an integer.
-        return (1);
+        return (5);
     }
 
     int int_value = int_elem->intValue();
     if (int_value != 42) {
         // Parameter specified, is an integer, but has a value different than
         // expected.
-        return (1);
+        return (6);
     }
 
     // Boolean handling example
     if (!bool_elem) {
         // Parameter was not specified at all.
-        return (1);
+        return (7);
     }
 
     if (bool_elem->getType() != Element::boolean) {
         // Parameter is specified, but it's not a boolean.
-        return (1);
+        return (8);
     }
 
     bool flag = bool_elem->boolValue();
     if (flag != true) {
         // Parameter specified, is a boolean, but has a value different than
         // expected.
-        return (1);
+        return (9);
     }
 
     // All validation steps were successful. The library has all the parameters