[#549] address review comments

diff --git a/doc/sphinx/arm/hooks-class-cmds.rst b/doc/sphinx/arm/hooks-class-cmds.rst
index 7d698c8ea14e94ced766395be7d522e3803269df..f3cf788f4e4ee5b92cb46af638d88ee99e8aa4c0 100644 (file)
--- a/doc/sphinx/arm/hooks-class-cmds.rst
+++ b/doc/sphinx/arm/hooks-class-cmds.rst
@@ -112,7 +112,7 @@ As with other update commands, this command overwrites all the contents of the
 entry. If the client class previously had a resource assigned to it, and the
 ``class-update`` command is missing the resource, it is deleted from the server
 configuration. If an incremental update of the class is desired, then this can
-be achieved by doing a `class-get <command-class-get_>`_ to get the full picture
+be achieved by doing a `class-get <command-class-get_>`_ to get the current state
 of the client class, picking the client class out of the response, modifying it
 to the required outcome, and then issuing the ``client-update`` command with the
 resulting client class attached.

diff --git a/doc/sphinx/arm/hooks-host-cmds.rst b/doc/sphinx/arm/hooks-host-cmds.rst
index f3cb8a3d60b0a34d80fb383a175eecc074c4ff6f..35de8cd11dc756cef1fb11b01abe4ac54db00ae0 100644 (file)
--- a/doc/sphinx/arm/hooks-host-cmds.rst
+++ b/doc/sphinx/arm/hooks-host-cmds.rst
@@ -144,7 +144,7 @@ address can be assigned like so:
        }
    }
 
-But it can also take many more parameters, for example:
+It can also take many more parameters, for example:
 
 .. code-block:: json
 
@@ -750,7 +750,7 @@ address can be assigned like so:
         }
     }
 
-But it can also take many more parameters, for example:
+It can also take many more parameters, for example:
 
 .. code-block:: json
 
@@ -838,7 +838,7 @@ As with other update and set commands, this command overwrites all the contents
 of the entry. If the host previously had a resource assigned to it, and the
 ``reservation-update`` command is missing the resource, it is deleted from the
 database. If an incremental update of the host is desired, then this can be
-achieved by doing a ``reservation-get-by-id`` to get the full picture of the
+achieved by doing a ``reservation-get-by-id`` to get the current state of the
 host, picking the host out of the response, modifying it to the required
 outcome, and then issuing the ``reservation-update`` command with the resulting
 host attached.

diff --git a/doc/sphinx/arm/hooks-lease-cmds.rst b/doc/sphinx/arm/hooks-lease-cmds.rst
index cd5c671c18032f9127f1d82a8209a04ef688b869..6c3626cde344f84c0c5f223c4554ecf46a3de7d1 100644 (file)
--- a/doc/sphinx/arm/hooks-lease-cmds.rst
+++ b/doc/sphinx/arm/hooks-lease-cmds.rst
@@ -908,7 +908,7 @@ entry. If the lease previously had a resource assigned to it, and the
 deleted from the lease database. If an incremental update of the lease is
 desired, then this can be achieved by doing a
 `lease4-get <command-lease4-get_>`_ / `lease6-get <command-lease6-get_>`_
-command to get the full picture of the lease, picking the lease out of the
+command to get the current state of the lease, picking the lease out of the
 response, modifying it to the required outcome, and then issuing the
 ``lease4-update``/``lease6-update`` command with the resulting lease attached.
 

diff --git a/doc/sphinx/arm/hooks-subnet-cmds.rst b/doc/sphinx/arm/hooks-subnet-cmds.rst
index ff5b01423b941bc880ebbaa3368e432ba2c05f49..7a21d061ceee82c0dbe27764bb4c15e526dccc81 100644 (file)
--- a/doc/sphinx/arm/hooks-subnet-cmds.rst
+++ b/doc/sphinx/arm/hooks-subnet-cmds.rst
@@ -466,7 +466,7 @@ The response to this command has the following structure:
    }
 
 As with other update commands, this command overwrites all the contents of the
-entry. If the IPv4 subnet previously had a resource assigned to it, and the
+entry. If the IPv6 subnet previously had a resource assigned to it, and the
 ``subnet6-update`` command is missing the resource, it is deleted from the
 server configuration. If an incremental update of the subnet is desired, then
 this can be achieved with `subnet6-delta-add <command-subnet6-delta-add_>`_.

diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.h b/src/hooks/dhcp/lease_cmds/lease_cmds.h
index 54ca5f74ab919ace7cbb738050769c5c484b5fcd..0a4f386ddd7776d856cc3a8fb86ce91a2314b476 100644 (file)
--- a/src/hooks/dhcp/lease_cmds/lease_cmds.h
+++ b/src/hooks/dhcp/lease_cmds/lease_cmds.h
@@ -33,7 +33,7 @@ public:
     /// @brief lease4-add, lease6-add command handler
     ///
     /// This command attempts to add a lease.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -169,7 +169,7 @@ public:
     /// @brief lease4-get, lease6-get command handler
     ///
     /// This command attempts to retrieve a lease that match selected criteria.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -346,7 +346,7 @@ public:
     ///
     /// This command attempts to delete an IPv4 lease that match selected
     /// criteria.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.  If the lease is deleted successfully, then a call
     /// to @ref isc::dhcp::queueNCR() is issued, which to generate an
@@ -384,7 +384,7 @@ public:
     /// @brief lease6-del command handler
     ///
     /// This command attempts to delete a lease that match selected criteria.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.  If the lease is deleted successfully, then a call
     /// to @ref isc::dhcp::queueNCR() is issued, which to generate an
@@ -427,7 +427,7 @@ public:
     /// specified will replace existing lease. The only condition is that
     /// the IP address must not change. If you want to change the IP address,
     /// please use lease4-del and lease4-add instead.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -458,7 +458,7 @@ public:
     /// specified will replace existing lease. The only condition is that
     /// the IP address must not change. If you want to change the IP address,
     /// please use lease6-del and lease6-add instead.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -490,7 +490,7 @@ public:
     /// subnet. Currently the leases are removed from the database,
     /// without any processing (like calling hooks or doing DDNS
     /// cleanups).
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -514,7 +514,7 @@ public:
     /// subnet. Currently the leases are removed from the database,
     /// without any processing (like calling hooks or doing DDNS
     /// cleanups).
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -537,7 +537,7 @@ public:
     /// This command attempts to resend the DDNS updates for the IPv4 lease that
     /// matches the selection criteria.
     ///
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -562,7 +562,7 @@ public:
     /// This command attempts to resend the DDNS updates for the IPv6 lease that
     /// matches the selection criteria.
     ///
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///
@@ -587,7 +587,7 @@ public:
     /// This commands attempts to write the lease database to a CSV file.
     /// Currently it is supported only by the memfile database and
     /// should be reserved to emergency situations.
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     ///

diff --git a/src/hooks/dhcp/stat_cmds/stat_cmds.h b/src/hooks/dhcp/stat_cmds/stat_cmds.h
index 2474574fa7d6e54e05f9333754a9368a038ce8a9..7d1c652701aa092733121e005c47bd9edc74af52 100644 (file)
--- a/src/hooks/dhcp/stat_cmds/stat_cmds.h
+++ b/src/hooks/dhcp/stat_cmds/stat_cmds.h
@@ -31,7 +31,7 @@ public:
     ///
     /// This command attempts to fetch lease4 statistics for one or
     /// more subnets based upon subnet selection criteria (or lack thereof).
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// arguments accordingly.
     /// {
@@ -75,7 +75,7 @@ public:
     ///
     /// This command attempts to fetch lease6 statistics for one or
     /// more subnets based upon subnet selection criteria (or lack thereof).
-    /// It extracts the command name and arguments from the given Callouthandle,
+    /// It extracts the command name and arguments from the given CalloutHandle,
     /// attempts to process them, and then set's the handle's "response"
     /// argument accordingly.
     /// {

diff --git a/src/lib/cc/command_interpreter.cc b/src/lib/cc/command_interpreter.cc
index 26574cd35279dc4159dfc636229e5659b290ad97..c5b478dd5e5f2da558723200dc88fe8c2fabce6a 100644 (file)
--- a/src/lib/cc/command_interpreter.cc
+++ b/src/lib/cc/command_interpreter.cc
@@ -170,7 +170,7 @@ createCommand(const std::string& command,
 std::string
 parseCommand(ConstElementPtr& arg, ConstElementPtr command) {
     if (!command) {
-        isc_throw(CtrlChannelError, "No command specified");
+        isc_throw(CtrlChannelError, "no command specified");
     }
     if (command->getType() != Element::map) {
         isc_throw(CtrlChannelError, "invalid command: expected toplevel entry to be a map, got "

diff --git a/src/lib/dhcpsrv/base_host_data_source.h b/src/lib/dhcpsrv/base_host_data_source.h
index 03ab6f87a21eb0a2faf5ed1f0379c4f33053926a..5e53424864082eabf1727236ab1034b72bb8f207 100644 (file)
--- a/src/lib/dhcpsrv/base_host_data_source.h
+++ b/src/lib/dhcpsrv/base_host_data_source.h
@@ -462,8 +462,6 @@ public:
     /// @brief Attempts to update an existing host entry.
     ///
     /// @param host the host up to date with the requested changes
-    ///
-    /// @return true if deletion was successful, false if the host was not there.
     virtual void update(HostPtr const& host) = 0;
 
     /// @brief Return backend type
@@ -480,7 +478,7 @@ public:
     /// @return Parameters of the backend.
     virtual isc::db::DatabaseConnection::ParameterMap getParameters() const {
         return (isc::db::DatabaseConnection::ParameterMap());
-    };
+    }
 
     /// @brief Commit Transactions
     ///

diff --git a/src/lib/dhcpsrv/cfg_hosts.cc b/src/lib/dhcpsrv/cfg_hosts.cc
index 7d20f721a2714398ab06fae439dff0d10a032f80..a0765006d7b5a5874e6a5687c2a0c33a181ed3af 100644 (file)
--- a/src/lib/dhcpsrv/cfg_hosts.cc
+++ b/src/lib/dhcpsrv/cfg_hosts.cc
@@ -960,7 +960,7 @@ CfgHosts::add(const HostPtr& host) {
                   " is added to the configuration");
     }
 
-    // At least one subnet ID must be used
+    // At least one subnet ID must be used.
     if (host->getIPv4SubnetID() == SUBNET_ID_UNUSED &&
         host->getIPv6SubnetID() == SUBNET_ID_UNUSED) {
         isc_throw(BadValue, "must not use both IPv4 and IPv6 subnet ids of"

diff --git a/src/lib/dhcpsrv/cfg_hosts.h b/src/lib/dhcpsrv/cfg_hosts.h
index aea5b711fd6f27c66ea8ec0c1f1e84c278377c91..448bca65cf03136773febfb5183a6dfbffebd349 100644 (file)
--- a/src/lib/dhcpsrv/cfg_hosts.h
+++ b/src/lib/dhcpsrv/cfg_hosts.h
@@ -590,6 +590,10 @@ public:
                       const uint8_t* identifier_begin, const size_t identifier_len);
 
     /// @brief Implements @ref BaseHostDataSource::update() for config hosts.
+    ///
+    /// Attempts to update an existing host entry.
+    ///
+    /// @param host the host up to date with the requested changes
     void update(HostPtr const& host);
 
     /// @brief Return backend type

diff --git a/src/lib/dhcpsrv/host_mgr.h b/src/lib/dhcpsrv/host_mgr.h
index 82d039adb1b5efe5364ced93834c3df71dbb7dbb..54208aea01cd8cda5eef1fcd0bd26c03aee95ecf 100644 (file)
--- a/src/lib/dhcpsrv/host_mgr.h
+++ b/src/lib/dhcpsrv/host_mgr.h
@@ -556,6 +556,10 @@ public:
          const uint8_t* identifier_begin, const size_t identifier_len);
 
     /// @brief Implements @ref BaseHostDataSource::update() for alternate sources.
+    ///
+    /// Attempts to update an existing host entry.
+    ///
+    /// @param host the host up to date with the requested changes
     void update(HostPtr const& host);
 
     /// @brief Return backend type

diff --git a/src/lib/dhcpsrv/mysql_host_data_source.h b/src/lib/dhcpsrv/mysql_host_data_source.h
index 8284dd2a92c446ca803d21218dedf3b0b2a49397..cf934dd92adbd38a75f92cc8e5d65014eec6f198 100644 (file)
--- a/src/lib/dhcpsrv/mysql_host_data_source.h
+++ b/src/lib/dhcpsrv/mysql_host_data_source.h
@@ -404,6 +404,10 @@ public:
             const asiolink::IOAddress& address) const;
 
     /// @brief Implements @ref BaseHostDataSource::update() for MySQL.
+    ///
+    /// Attempts to update an existing host entry.
+    ///
+    /// @param host the host up to date with the requested changes
     void update(HostPtr const& host);
 
     /// @brief Return backend type

diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.h b/src/lib/dhcpsrv/pgsql_host_data_source.h
index 23004ecbde8b1d883eb2ad255722aac457f66746..be342e7a912507d4048dd4b0f440bf6ceaa740a6 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_host_data_source.h
+++ b/src/lib/dhcpsrv/pgsql_host_data_source.h
@@ -452,6 +452,10 @@ public:
             const asiolink::IOAddress& address) const;
 
     /// @brief Implements @ref BaseHostDataSource::update() for PostgreSQL.
+    ///
+    /// Attempts to update an existing host entry.
+    ///
+    /// @param host the host up to date with the requested changes
     void update(HostPtr const& host);
 
     /// @brief Return backend type

diff --git a/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc
index cedcfcd5baefe9ca28cd685377156387470bc0c2..8f55a4459fe24ccca37d65fc6d5463e87e13e79c 100644 (file)
--- a/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc
+++ b/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc
@@ -1445,6 +1445,7 @@ TEST_F(PgSqlHostDataSourceTest, updateMultiThreading) {
     MultiThreadingTest mt(true);
     testUpdate();
 }
+
 /// @brief Test fixture class for validating @c HostMgr using
 /// PostgreSQL as alternate host data source.
 class PgSQLHostMgrTest : public HostMgrTest {

diff --git a/src/lib/dhcpsrv/testutils/memory_host_data_source.cc b/src/lib/dhcpsrv/testutils/memory_host_data_source.cc
index 89b6e2bf2616d62f0d78949d4659aa5265c7c218..5bf5d79c4e20efba53512fef8fa300391431adc9 100644 (file)
--- a/src/lib/dhcpsrv/testutils/memory_host_data_source.cc
+++ b/src/lib/dhcpsrv/testutils/memory_host_data_source.cc
@@ -377,11 +377,20 @@ MemHostDataSource::del6(const SubnetID& subnet_id,
 
 void
 MemHostDataSource::update(HostPtr const& host) {
-    for (auto h = store_.begin(); h != store_.end(); ++h) {
-        if ((*h)->getHostId() == host->getHostId()) {
-            store_.erase(h);
-            break;
-        }
+    bool deleted(false);
+    if (host->getIPv4SubnetID() != SUBNET_ID_UNUSED) {
+        vector<uint8_t> const& identifier(host->getIdentifier());
+        deleted = del4(host->getIPv4SubnetID(), host->getIdentifierType(), identifier.data(),
+             identifier.size());
+    } else if (host->getIPv6SubnetID() != SUBNET_ID_UNUSED) {
+        vector<uint8_t> const& identifier(host->getIdentifier());
+        deleted = del6(host->getIPv6SubnetID(), host->getIdentifierType(), identifier.data(),
+             identifier.size());
+    } else {
+        isc_throw(HostNotFound, "Mandatory 'subnet-id' parameter missing.");
+    }
+    if (!deleted) {
+        isc_throw(HostNotFound, "Host not updated (not found).");
     }
     store_.push_back(host);
 }

diff --git a/src/lib/dhcpsrv/testutils/memory_host_data_source.h b/src/lib/dhcpsrv/testutils/memory_host_data_source.h
index bf8f6a35e6524b4ab86591da53988e70c7e3fcfc..ceabe704dd2b2a33b6fe7bb75b40a6d0a661793b 100644 (file)
--- a/src/lib/dhcpsrv/testutils/memory_host_data_source.h
+++ b/src/lib/dhcpsrv/testutils/memory_host_data_source.h
@@ -258,6 +258,10 @@ public:
                       const uint8_t* identifier_begin, const size_t identifier_len);
 
     /// @brief Implements @ref BaseHostDataSource::update() for memory hosts.
+    ///
+    /// Attempts to update an existing host entry.
+    ///
+    /// @param host the host up to date with the requested changes
     void update(HostPtr const& host);
 
     /// @brief Return backend type

diff --git a/src/share/api/reservation-add.json b/src/share/api/reservation-add.json
index 3eee6a03fead8604f1492a6c1e9b44cd52c63791..ecee118bc5435ccf91d871dd3171fa7ecf4f4dc3 100644 (file)
--- a/src/share/api/reservation-add.json
+++ b/src/share/api/reservation-add.json
@@ -5,7 +5,7 @@
         "This command adds a new host reservation. The reservation may include IPv4 addresses, IPv6 addresses, IPv6 prefixes, various identifiers, a class the client will be assigned to, DHCPv4 and DHCPv6 options, and more."
     ],
     "cmd-comment": [
-        "Note that ip-address, client-id, next-server, server-hostname, and boot-file-name are IPv4-specific. duid, ip-addresses, and prefixes are IPv6-specific."
+        "Note that ip-address, client-id, next-server, server-hostname, and boot-file-name are IPv4-specific. ip-addresses, and prefixes are IPv6-specific."
     ],
     "cmd-syntax": [
         "{",
@@ -13,7 +13,6 @@
         "    \"arguments\": {",
         "        \"reservation\": {",
         "            \"boot-file-name\": <string>,",
-        "            \"comment\": <string>,",
         "            \"client-id\": <string>,",
         "            \"circuit-id\": <string>,",
         "            \"duid\": <string>,",

diff --git a/src/share/api/reservation-update.json b/src/share/api/reservation-update.json
index 95853d4bbb439def0bb1d6f65f75bd2357845b79..5c0a0b81b1b208aa9f89629ac88f8b137b43b525 100644 (file)
--- a/src/share/api/reservation-update.json
+++ b/src/share/api/reservation-update.json
@@ -1,11 +1,11 @@
 {
     "access": "write",
-    "avail": "2.4.0",
+    "avail": "2.3.7",
     "brief": [
         "This command updates an existing host reservation. The reservation has to include host identifiers and a subnet identifier and may include IPv4 addresses, IPv6 addresses, IPv6 prefixes, various identifiers, a class the client will be assigned to, DHCPv4 and DHCPv6 options, and more."
     ],
     "cmd-comment": [
-        "Note that ip-address, client-id, next-server, server-hostname, and boot-file-name are IPv4-specific. duid, ip-addresses, and prefixes are IPv6-specific."
+        "Note that ip-address, client-id, next-server, server-hostname, and boot-file-name are IPv4-specific. ip-addresses, and prefixes are IPv6-specific."
     ],
     "cmd-syntax": [
         "{",
@@ -13,7 +13,6 @@
         "    \"arguments\": {",
         "        \"reservation\": {",
         "            \"boot-file-name\": <string>,",
-        "            \"comment\": <string>,",
         "            \"client-id\": <string>,",
         "            \"circuit-id\": <string>,",
         "            \"duid\": <string>,",