[#4389] addressed review comments

diff --git a/ChangeLog b/ChangeLog
index 30125e84096e0005b9f9406c0db1ddabae096a8f..29b0269439926363a971f1de702202c68374344c 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2448.  [func]          razvan
+       The kea servers return a new status ("result" with value 5) for
+       commands which have failed and have partially altered the running
+       configuration. The rollback mechanism was not able to restore
+       the previous configuration.
+       (Gitlab #4389)
+
 2447.  [func]          razvan
        When using raw sockets, Kea now properly handles VLAN
        interfaces and VLAN tagged packets.

diff --git a/doc/sphinx/arm/ctrl-channel.rst b/doc/sphinx/arm/ctrl-channel.rst
index cc04ead78b1651ff2168532eff8318abeaa6ed1e..386966b255978b5418ce9f79af5ca4d119f38adb 100644 (file)
--- a/doc/sphinx/arm/ctrl-channel.rst
+++ b/doc/sphinx/arm/ctrl-channel.rst
@@ -164,6 +164,9 @@ following general status codes are currently supported:
 -  ``4`` - the well-formed command has been processed but the requested
    changes could not be applied, because they were in conflict with the
    server state or its notion of the configuration.
+-  ``5`` - the command processed has failed and the configuration has been
+   partially altered. The rollback mechanism was not able to restore the previous
+   configuration.
 
 For example, a well-formed command that requests a subnet that exists
 in a server's configuration returns the result 0. If the server encounters

diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc
index 02d1e80ee77433a01293028a34e06706c0162ad2..0348f79442f4d98844b1f107fbf27f5123c5eec2 100644 (file)
--- a/src/bin/dhcp4/json_config_parser.cc
+++ b/src/bin/dhcp4/json_config_parser.cc
@@ -767,6 +767,9 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set,
 
     SrvConfigPtr srv_config;
 
+    // Parsing stage is complete. The current configuration has not been altered.
+    // From this stage on, every error might have irreversible consequences and the
+    // configuration might not be restored to a working state.
     if (status_code == CONTROL_RESULT_SUCCESS) {
         if (check_only) {
             if (extra_checks) {
@@ -789,6 +792,10 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set,
             }
         } else {
 
+            // Usually unit tests create managers before calling configureDhcp4Server and
+            // do not call ControlledDhcpv4Srv::processConfig.
+            // Runtime code path creates the managers after calling configureDhcp4Server
+            // and they need to be reset just after successful configuration parsing.
             if (!IfaceMgr::instance().isTestMode()) {
                 // Destroy lease manager before hooks unload.
                 LeaseMgrFactory::destroy();

diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc
index 42d0410582a776f56354f371453ee46e108fe9bb..3dad56ab16f6f53cc8b63d4c381fbdc582181a95 100644 (file)
--- a/src/bin/dhcp6/json_config_parser.cc
+++ b/src/bin/dhcp6/json_config_parser.cc
@@ -885,6 +885,9 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set,
 
     SrvConfigPtr srv_config;
 
+    // Parsing stage is complete. The current configuration has not been altered.
+    // From this stage on, every error might have irreversible consequences and the
+    // configuration might not be restored to a working state.
     if (status_code == CONTROL_RESULT_SUCCESS) {
         if (check_only) {
             if (extra_checks) {
@@ -907,6 +910,10 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set,
             }
         } else {
 
+            // Usually unit tests create managers before calling configureDhcp6Server and
+            // do not call ControlledDhcpv6Srv::processConfig.
+            // Runtime code path creates the managers after calling configureDhcp6Server
+            // and they need to be reset just after successful configuration parsing.
             if (!IfaceMgr::instance().isTestMode()) {
                 // Destroy lease manager before hooks unload.
                 LeaseMgrFactory::destroy();

diff --git a/src/lib/cc/command_interpreter.h b/src/lib/cc/command_interpreter.h
index a41ada9c791c969efbb7b9bbc11a6ac7ca920392..16865e554f5e96b78a4cf787a72cec5855d6ced4 100644 (file)
--- a/src/lib/cc/command_interpreter.h
+++ b/src/lib/cc/command_interpreter.h
@@ -59,7 +59,7 @@ const int CONTROL_RESULT_EMPTY = 3;
 const int CONTROL_RESULT_CONFLICT = 4;
 
 /// @brief Status code indicating that the command was unsuccessful and the
-/// configuration cound not be reverted to a working state.
+/// configuration could not be reverted to a working state.
 const int CONTROL_RESULT_FATAL_ERROR = 5;
 
 /// @brief A standard control channel exception that is thrown if a function

diff --git a/src/lib/process/d_cfg_mgr.cc b/src/lib/process/d_cfg_mgr.cc
index 7a2d64e32c23572242b10181d77e5979c61538ce..be690bdc5627d4800185378b8e7c06f560ad69fc 100644 (file)
--- a/src/lib/process/d_cfg_mgr.cc
+++ b/src/lib/process/d_cfg_mgr.cc
@@ -114,7 +114,7 @@ DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set,
                     if (post_config_cb) {
                         post_config_cb();
                     }
-                } catch (std::exception& ex) {
+                } catch (std::exception&) {
                     fatal = true;
                     throw;
                 } catch (...) {