[2981] Changes as a result of review.

src/bin/dhcp4/config_parser.cc

@@ -403,7 +403,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
 
     // Some of the parsers alter the state of the system in a way that can't
     // easily be undone. (Or alter it in a way such that undoing the change has
-    // the same risk of failurre as doing the change.)
+    // the same risk of failure as doing the change.)
     ParserPtr hooks_parser_;
 
     // The subnet parsers implement data inheritance by directly
@@ -506,6 +506,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
             if (iface_parser) {
                 iface_parser->commit();
             }
+
+            // This occurs last as if it succeeds, there is no easy way
+            // revert it.  As a result, the failure to commit a subsequent
+            // change causes problems when trying to roll back.
+            if (hooks_parser_) {
+                hooks_parser_->commit();
+            }
         }
         catch (const isc::Exception& ex) {
             LOG_ERROR(dhcp4_logger, DHCP4_PARSER_COMMIT_FAIL).arg(ex.what());
@@ -527,12 +534,6 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         return (answer);
     }
 
-    // Now commit changes that have been validated but not yet committed,
-    // and which can't be rolled back.
-    if (hooks_parser_) {
-        hooks_parser_->commit();
-    }
-
     LOG_INFO(dhcp4_logger, DHCP4_CONFIG_COMPLETE).arg(config_details);
 
     // Everything was fine. Configuration is successful.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -145,13 +145,14 @@ ControlledDhcpv4Srv::dhcp4CommandHandler(const string& command, ConstElementPtr
         ConstElementPtr answer = isc::config::createAnswer(0,
                                  "Shutting down.");
         return (answer);
+
     } else if (command == "libreload") {
         // TODO delete any stored CalloutHandles referring to the old libraries
         // Get list of currently loaded libraries and reload them.
         vector<string> loaded = HooksManager::getLibraryNames();
         bool status = HooksManager::loadLibraries(loaded);
         if (!status) {
-            LOG_ERROR(dhcp4_logger, DHCP4_RELOAD_FAIL);
+            LOG_ERROR(dhcp4_logger, DHCP4_HOOKS_LIBS_RELOAD_FAIL);
             ConstElementPtr answer = isc::config::createAnswer(1,
                                      "Failed to reload hooks libraries.");
             return (answer);

src/bin/dhcp4/dhcp4_messages.mes

@@ -89,6 +89,11 @@ point, the setting of the flag instructs the server not to choose a
 subnet, an action that severely limits further processing; the server
 will be only able to offer global options - no addresses will be assigned.
 
+% DHCP4_HOOKS_LIBS_RELOAD_FAIL reload of hooks libraries failed
+A "libreload" command was issued to reload the hooks libraries but for
+some reason the reload failed.  Other error messages issued from the
+hooks framework will indicate the nature of the problem.
+
 % DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3)
 This debug message indicates that the server successfully advertised
 a lease. It is up to the client to choose one server out of othe advertised
@@ -226,11 +231,6 @@ a different hardware address. One possible reason for using different
 hardware address is that a cloned virtual machine was not updated and
 both clones use the same client-id.
 
-% DHCP4_RELOAD_FAIL reload of hooks libraries failed
-A "libreload" command was issued to reload the hooks libraries but for
-some reason the reload failed.  Other error messages issued from the
-hooks framework will indicate the nature of the problem.
-
 % DHCP4_RESPONSE_DATA responding with packet type %1, data is <%2>
 A debug message listing the data returned to the client.
 

src/bin/dhcp4/tests/callout_library_common.h

@@ -21,9 +21,9 @@
 /// To check that they libraries are loaded and unloaded correctly, the load
 /// and unload functions in this library maintain two marker files - the load
 /// marker file and the unload marker file.  The functions append a single
-/// to the single line in the file, creating the file if need be.  In
+/// line to the file, creating the file if need be.  In this way, the test code
-/// this way, the test code can determine whether the load/unload functions
+/// can determine whether the load/unload functions have been run and, if so,
-/// have been run and, if so, in what order.
+/// in what order.
 ///
 /// This file is the common library file for the tests.  It will not compile
 /// by itself - it is included into each callout library which specifies the
@@ -69,11 +69,13 @@ version() {
     return (BIND10_HOOKS_VERSION);
 }
 
-int load(LibraryHandle&) {
+int
+load(LibraryHandle&) {
     return (appendDigit(LOAD_MARKER_FILE));
 }
 
-int unload() {
+int
+unload() {
     return (appendDigit(UNLOAD_MARKER_FILE));
 }
 

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -60,6 +60,15 @@ public:
         CfgMgr::instance().deleteActiveIfaces();
     }
 
+    // Check that no hooks libraries are loaded.  This is a pre-condition for
+    // a number of tests, so is checked in one place.  As this uses an
+    // ASSERT call - and it is not clear from the documentation that Gtest
+    // predicates can be used in a constructor - the check is placed in SetUp.
+    void SetUp() {
+        std::vector<std::string> libraries = HooksManager::getLibraryNames();
+        ASSERT_TRUE(libraries.empty());
+    }
+
     // Checks if global parameter of name have expected_value
     void checkGlobalUint32(string name, uint32_t expected_value) {
         const Uint32StoragePtr uint32_defaults =
@@ -317,10 +326,10 @@ public:
                                                "reset configuration database"));
     }
 
-    boost::scoped_ptr<Dhcpv4Srv> srv_;
 
-    int rcode_;
+    boost::scoped_ptr<Dhcpv4Srv> srv_;      // DHCP4 server under test
-    ConstElementPtr comment_;
+    int rcode_;                             // Return code from element parsing
+    ConstElementPtr comment_;               // Reason for parse fail
 };
 
 // Goal of this test is a verification if a very simple config update
@@ -1785,7 +1794,9 @@ TEST_F(Dhcp4ParserTest, stdOptionDataEncapsulate) {
     EXPECT_FALSE(desc.option->getOption(3));
 }
 
-// Tests of the hooks libraries configuration.
+// Tests of the hooks libraries configuration.  All tests have the pre-
+// condition (checked in the test fixture's SetUp() method) that no hooks
+// libraries are loaded at the start of the tests.
 
 // Helper function to return a configuration containing an arbitrary number
 // of hooks libraries.
@@ -1862,10 +1873,6 @@ buildHooksLibrariesConfig(const char* library1 = NULL,
 // The goal of this test is to verify the configuration of hooks libraries if
 // none are specified.
 TEST_F(Dhcp4ParserTest, NoHooksLibraries) {
-    // Ensure that no libraries are loaded at the start of the test.
-    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
-
     // Parse a configuration containing no names.
     string config = buildHooksLibrariesConfig();
     if (!executeConfiguration(config,
@@ -1874,17 +1881,13 @@ TEST_F(Dhcp4ParserTest, NoHooksLibraries) {
 
     } else {
         // No libraries should be loaded at the end of the test.
-        libraries = HooksManager::getLibraryNames();
+        std::vector<std::string> libraries = HooksManager::getLibraryNames();
         EXPECT_TRUE(libraries.empty());
     }
 }
 
 // Verify parsing fails with one library that will fail validation.
 TEST_F(Dhcp4ParserTest, InvalidLibrary) {
-    // Ensure that no libraries are loaded at the start of the test.
-    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
-
     // Parse a configuration containing a failing library.
     string config = buildHooksLibrariesConfig(NOT_PRESENT_LIBRARY);
 
@@ -1902,10 +1905,6 @@ TEST_F(Dhcp4ParserTest, InvalidLibrary) {
 
 // Verify the configuration of hooks libraries with two being specified.
 TEST_F(Dhcp4ParserTest, LibrariesSpecified) {
-    // Ensure that no libraries are loaded at the start of the test.
-    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
-
     // Marker files should not be present.
     EXPECT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE));
     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
@@ -1918,7 +1917,7 @@ TEST_F(Dhcp4ParserTest, LibrariesSpecified) {
 
     // Expect two libraries to be loaded in the correct order (load marker file
     // is present, no unload marker file).
-    libraries = HooksManager::getLibraryNames();
+    std::vector<std::string> libraries = HooksManager::getLibraryNames();
     ASSERT_EQ(2, libraries.size());
     EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));

src/bin/dhcp6/config_parser.cc

@@ -572,6 +572,13 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
             if (iface_parser) {
                 iface_parser->commit();
             }
+
+            // This occurs last as if it succeeds, there is no easy way to
+            // revert it.  As a result, the failure to commit a subsequent
+            // change causes problems when trying to roll back.
+            if (hooks_parser) {
+                hooks_parser->commit();
+            }
         }
         catch (const isc::Exception& ex) {
             LOG_ERROR(dhcp6_logger, DHCP6_PARSER_COMMIT_FAIL).arg(ex.what());
@@ -595,12 +602,6 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
         return (answer);
     }
 
-    // Now commit any changes that have been validated but not yet committed,
-    // and which can't be rolled back.
-    if (hooks_parser) {
-        hooks_parser->commit();
-    }
-
     LOG_INFO(dhcp6_logger, DHCP6_CONFIG_COMPLETE).arg(config_details);
 
     // Everything was fine. Configuration is successful.

src/bin/dhcp6/ctrl_dhcp6_srv.cc

@@ -145,13 +145,14 @@ ControlledDhcpv6Srv::dhcp6CommandHandler(const string& command, ConstElementPtr
         ConstElementPtr answer = isc::config::createAnswer(0,
                                  "Shutting down.");
         return (answer);
+
     } else if (command == "libreload") {
         // TODO delete any stored CalloutHandles referring to the old libraries
         // Get list of currently loaded libraries and reload them.
         vector<string> loaded = HooksManager::getLibraryNames();
         bool status = HooksManager::loadLibraries(loaded);
         if (!status) {
-            LOG_ERROR(dhcp6_logger, DHCP6_RELOAD_FAIL);
+            LOG_ERROR(dhcp6_logger, DHCP6_HOOKS_LIBS_RELOAD_FAIL);
             ConstElementPtr answer = isc::config::createAnswer(1,
                                      "Failed to reload hooks libraries.");
             return (answer);

src/bin/dhcp6/dhcp6_messages.mes

@@ -96,6 +96,11 @@ subnet, an action that severely limits further processing; the server
 will be only able to offer global options - no addresses or prefixes
 will be assigned.
 
+% DHCP6_HOOKS_LIBS_RELOAD_FAIL reload of hooks libraries failed
+A "libreload" command was issued to reload the hooks libraries but for
+some reason the reload failed.  Other error messages issued from the
+hooks framework will indicate the nature of the problem.
+
 % DHCP6_LEASE_ADVERT lease %1 advertised (client duid=%2, iaid=%3)
 This debug message indicates that the server successfully advertised
 a lease. It is up to the client to choose one server out of the
@@ -246,11 +251,6 @@ mandatory client-id option. This is most likely caused by a buggy client
 (or a relay that malformed forwarded message). This request will not be
 processed and a response with error status code will be sent back.
 
-% DHCP6_RELOAD_FAIL reload of hooks libraries failed
-A "libreload" command was issued to reload the hooks libraries but for
-some reason the reload failed.  Other error messages issued from the
-hooks framework will indicate the nature of the problem.
-
 % DHCP6_RENEW_UNKNOWN_SUBNET RENEW message received from client on unknown subnet (duid=%1, iaid=%2)
 A warning message indicating that a client is attempting to renew his lease,
 but the server does not have any information about the subnet this client belongs

src/bin/dhcp6/tests/callout_library_common.h

@@ -21,9 +21,9 @@
 /// To check that they libraries are loaded and unloaded correctly, the load
 /// and unload functions in this library maintain two marker files - the load
 /// marker file and the unload marker file.  The functions append a single
-/// to the single line in the file, creating the file if need be.  In
+/// line to the file, creating the file if need be.  In this way, the test code
-/// this way, the test code can determine whether the load/unload functions
+/// can determine whether the load/unload functions have been run and, if so,
-/// have been run and, if so, in what order.
+/// in what order.
 ///
 /// This file is the common library file for the tests.  It will not compile
 /// by itself - it is included into each callout library which specifies the
@@ -69,11 +69,13 @@ version() {
     return (BIND10_HOOKS_VERSION);
 }
 
-int load(LibraryHandle&) {
+int
+load(LibraryHandle&) {
     return (appendDigit(LOAD_MARKER_FILE));
 }
 
-int unload() {
+int
+unload() {
     return (appendDigit(UNLOAD_MARKER_FILE));
 }
 

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -81,6 +81,15 @@ public:
         resetConfiguration();
     }
 
+    // Check that no hooks libraries are loaded.  This is a pre-condition for
+    // a number of tests, so is checked in one place.  As this uses an
+    // ASSERT call - and it is not clear from the documentation that Gtest
+    // predicates can be used in a constructor - the check is placed in SetUp.
+    void SetUp() {
+        std::vector<std::string> libraries = HooksManager::getLibraryNames();
+        ASSERT_TRUE(libraries.empty());
+    }
+
     ~Dhcp6ParserTest() {
         // Reset configuration database after each test.
         resetConfiguration();
@@ -1895,7 +1904,10 @@ TEST_F(Dhcp6ParserTest, stdOptionDataEncapsulate) {
     EXPECT_FALSE(desc.option->getOption(112));
 }
 
-// Tests of the hooks libraries configuration.
+// Tests of the hooks libraries configuration.  All tests have the pre-
+// condition (checked in the test fixture's SetUp() method) that no hooks
+// libraries are loaded at the start of the tests.
+
 
 // Helper function to return a configuration containing an arbitrary number
 // of hooks libraries.
@@ -1977,10 +1989,6 @@ buildHooksLibrariesConfig(const char* library1 = NULL,
 // The goal of this test is to verify the configuration of hooks libraries if
 // none are specified.
 TEST_F(Dhcp6ParserTest, NoHooksLibraries) {
-    // Ensure that no libraries are loaded at the start of the test.
-    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
-
     // Parse a configuration containing no names.
     string config = buildHooksLibrariesConfig();
     if (!executeConfiguration(config,
@@ -1989,17 +1997,13 @@ TEST_F(Dhcp6ParserTest, NoHooksLibraries) {
 
     } else {
         // No libraries should be loaded at the end of the test.
-        libraries = HooksManager::getLibraryNames();
+        std::vector<std::string> libraries = HooksManager::getLibraryNames();
         EXPECT_TRUE(libraries.empty());
     }
 }
 
 // Verify parsing fails with one library that will fail validation.
 TEST_F(Dhcp6ParserTest, InvalidLibrary) {
-    // Ensure that no libraries are loaded at the start of the test.
-    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
-
     // Parse a configuration containing a failing library.
     string config = buildHooksLibrariesConfig(NOT_PRESENT_LIBRARY);
 
@@ -2017,10 +2021,6 @@ TEST_F(Dhcp6ParserTest, InvalidLibrary) {
 
 // Verify the configuration of hooks libraries with two being specified.
 TEST_F(Dhcp6ParserTest, LibrariesSpecified) {
-    // Ensure that no libraries are loaded at the start of the test.
-    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-    ASSERT_TRUE(libraries.empty());
-
     // Marker files should not be present.
     EXPECT_FALSE(checkMarkerFileExists(LOAD_MARKER_FILE));
     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
@@ -2033,10 +2033,10 @@ TEST_F(Dhcp6ParserTest, LibrariesSpecified) {
 
     // Expect two libraries to be loaded in the correct order (load marker file
     // is present, no unload marker file).
-     libraries = HooksManager::getLibraryNames();
+    std::vector<std::string> libraries = HooksManager::getLibraryNames();
-     ASSERT_EQ(2, libraries.size());
+    ASSERT_EQ(2, libraries.size());
-     EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
+    EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12"));
-     EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
+    EXPECT_FALSE(checkMarkerFileExists(UNLOAD_MARKER_FILE));
 
     // Unload the libraries.  The load file should not have changed, but
     // the unload one should indicate the unload() functions have been run.

src/lib/dhcpsrv/dhcp_parsers.h

@@ -338,7 +338,7 @@ private:
     std::string param_name_;
 };
 
-/// @brief parser for hooks library list
+/// @brief Parser for hooks library list
 ///
 /// This parser handles the list of hooks libraries.  This is an optional list,
 /// which may be empty.
@@ -364,7 +364,7 @@ public:
     /// @brief Constructor
     ///
     /// As this is a dedicated parser, it must be used to parse
-    /// "hooks_libraries" parameter only. All other types will throw exception.
+    /// "hooks-libraries" parameter only. All other types will throw exception.
     ///
     /// @param param_name name of the configuration parameter being parsed.
     ///
@@ -395,9 +395,9 @@ public:
     /// an indication as to whether the list is different from the list of
     /// libraries already loaded.
     ///
-    /// @param libraries (out) List of libraries that were specified in the
+    /// @param libraries [out] List of libraries that were specified in the
     ///        new configuration.
-    /// @param changed (out) true if the list is different from that currently
+    /// @param changed [out] true if the list is different from that currently
     ///        loaded.
     void getLibraries(std::vector<std::string>& libraries, bool& changed);