From: Stephen Morris Date: Mon, 9 Nov 2015 12:41:52 +0000 (+0000) Subject: [3259] Changes after review X-Git-Tag: trac4121_base~1^2 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=68f02accb431e141534d2b5957eb582eefae466d;p=thirdparty%2Fkea.git [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. --- diff --git a/doc/guide/hooks.xml b/doc/guide/hooks.xml index 42653b7dcf..6c275fdc20 100644 --- a/doc/guide/hooks.xml +++ b/doc/guide/hooks.xml @@ -84,7 +84,18 @@ An empty list has the same effect as omitting the hooks-libraries configuration element all together. - + + + There is one case where this is not true: if Kea + is running with a configuration that contains a + hooks-libraries 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 hooks-libraries + item, change it to an empty list. This will be fixed in a + future version of Kea. + + diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc index d877349cce..e95a7d3e02 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc +++ b/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. diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.h b/src/lib/dhcpsrv/parsers/dhcp_parsers.h index 30d530aab3..2243ecece3 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.h +++ b/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); diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 5e56696ba8..008c32e539 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/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_; }