[3259] Changes after review

1. Updated Kea guide to note that removing the hooks-libraries
   configuration element does not always have the expected effect.
2. Moved some documentation from the dhcp_parsers.cc file to the .h file.
3. Expanded checking of the contents of the hooks-libraries configuration
   element.

doc/guide/hooks.xml

@@ -84,7 +84,18 @@
           <listitem><para>
           An empty list has the same effect as omitting the
           <command>hooks-libraries</command> configuration element all together.
-          </para></listitem>
+          </para>
+          <note><para>
+          There is one case where this is not true: if Kea
+          is running with a configuration that contains a
+          <command>hooks-libraries</command> item, and that item is
+          removed and the configuration reloaded, the removal will be
+          ignored and the libraries remain loaded.  As a workaround,
+          instead of removing the <command>hooks-libraries</command>
+          item, change it to an empty list.  This will be fixed in a
+          future version of Kea.
+          </para></note>
+          </listitem>
         </itemizedlist>
       </para>
       <para>

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -248,25 +248,8 @@ HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name)
     }
 }
 
-// The syntax for specifying hooks libraries allow for library-specific
-// parameters to be specified along with the library, e.g.
-//
-//      "hooks-libraries": [
-//          {
-//              "library": "hook-lib-1.so",
-//              "parameters": {
-//                  "alpha": "a string",
-//                  "beta": 42
-//              }
-//          },
-//          {
-//              "library": "hook-lib-2.so",
-//                  :
-//          }
-//      ]
-//
-// Kea has not yet implemented parameters, so the parsing code only checks
-// that:
+// Parse the configuration.  As Kea has not yet implemented parameters, the
+// parsing code only checks that:
 //
 // 1. Each element in the hooks-libraries list is a map
 // 2. The map contains an element "library" whose value is a string: all
@@ -293,15 +276,25 @@ HooksLibrariesParser::build(ConstElementPtr value) {
             if (entry_item.first == "library") {
                 if (entry_item.second->getType() != Element::string) {
                     isc_throw(DhcpConfigError, "hooks library configuration"
-                        " error: value of 'library' element is not a valid"
-                        " path to a hooks library (" <<
+                        " error: value of 'library' element is not a string"
+                        " giving the path to a hooks library (" <<
                         entry_item.second->getPosition() << ")");
                 }
 
                 // Get the name of the library and add it to the list after
                 // removing quotes.
                 string libname = (entry_item.second)->stringValue();
+
+                // Remove leading/trailing quotes and any leading/trailing
+                // spaces.
                 boost::erase_all(libname, "\"");
+                libname = isc::util::str::trim(libname);
+                if (libname.empty()) {
+                    isc_throw(DhcpConfigError, "hooks library configuration"
+                        " error: value of 'library' element must not be"
+                        " blank (" <<
+                        entry_item.second->getPosition() << ")");
+                }
                 libraries_.push_back(libname);
 
                 // Note we have found the library name.

src/lib/dhcpsrv/parsers/dhcp_parsers.h

@@ -479,6 +479,29 @@ public:
     /// checks each of the libraries in the list for validity (they exist and
     /// have a "version" function that returns the correct value).
     ///
+    /// The syntax for specifying hooks libraries allow for library-specific
+    /// parameters to be specified along with the library, e.g.
+    ///
+    /// @code
+    ///      "hooks-libraries": [
+    ///          {
+    ///              "library": "hook-lib-1.so",
+    ///              "parameters": {
+    ///                  "alpha": "a string",
+    ///                  "beta": 42
+    ///              }
+    ///          },
+    ///          :
+    ///      ]
+    /// @endcode
+    ///
+    /// As Kea has not yet implemented parameters, the parsing code only checks
+    /// that:
+    ///
+    /// -# Each element in the hooks-libraries list is a map
+    /// -# The map contains an element "library" whose value is a string: all
+    ///    other elements in the map are ignored.
+    ///
     /// @param value pointer to the content of parsed values
     virtual void build(isc::data::ConstElementPtr value);
 

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -1177,27 +1177,53 @@ TEST_F(ParseConfigTest, invalidSyntaxHooksLibraries) {
         "{ \"library\": \"/opt/lib/lib1\" }, "
         "{ \"library\": 123 } "
         "] }";
-    string error2 = "value of 'library' element is not a valid"
-                    " path to a hooks library";
+    string error2 = "value of 'library' element is not a string giving"
+                    " the path to a hooks library";
 
     rcode = parseConfiguration(config2);
     ASSERT_NE(0, rcode);
     EXPECT_TRUE(error_text_.find(error2) != string::npos) <<
         "Error text returned from parse failure is " << error_text_;
 
+    // Element holds valid maps, except one where the library element is the
+    // empty string.
+    string config3 = "{ \"hooks-libraries\": [ "
+        "{ \"library\": \"/opt/lib/lib1\" }, "
+        "{ \"library\": \"\" } "
+        "] }";
+    string error3 = "value of 'library' element must not be blank";
+
+    rcode = parseConfiguration(config3);
+    ASSERT_NE(0, rcode);
+    EXPECT_TRUE(error_text_.find(error3) != string::npos) <<
+        "Error text returned from parse failure is " << error_text_;
+
+    // Element holds valid maps, except one where the library element is all
+    // spaces.
+    string config4 = "{ \"hooks-libraries\": [ "
+        "{ \"library\": \"/opt/lib/lib1\" }, "
+        "{ \"library\": \"      \" } "
+        "] }";
+    string error4 = "value of 'library' element must not be blank";
+
+    rcode = parseConfiguration(config4);
+    ASSERT_NE(0, rcode);
+    EXPECT_TRUE(error_text_.find(error3) != string::npos) <<
+        "Error text returned from parse failure is " << error_text_;
+
     // Element holds valid maps, except one that does not contain a
     // 'library' element.
-    string config3 = "{ \"hooks-libraries\": [ "
+    string config5 = "{ \"hooks-libraries\": [ "
         "{ \"library\": \"/opt/lib/lib1\" }, "
         "{ \"parameters\": { \"alpha\": 123 } }, "
         "{ \"library\": \"/opt/lib/lib2\" } "
         "] }";
-    string error3 = "one or more hooks-libraries elements are missing the"
+    string error5 = "one or more hooks-libraries elements are missing the"
                     " name of the library";
 
-    rcode = parseConfiguration(config3);
+    rcode = parseConfiguration(config5);
     ASSERT_NE(0, rcode);
-    EXPECT_TRUE(error_text_.find(error3) != string::npos) <<
+    EXPECT_TRUE(error_text_.find(error5) != string::npos) <<
         "Error text returned from parse failure is " << error_text_;
 }