[#549] address review comments

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.

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.

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.
 

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_>`_.

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.
     ///

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.
     /// {

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 "

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
     ///

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"

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

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

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

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

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 {

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) {
+    bool deleted(false);
-        if ((*h)->getHostId() == host->getHostId()) {
+    if (host->getIPv4SubnetID() != SUBNET_ID_UNUSED) {
-            store_.erase(h);
+        vector<uint8_t> const& identifier(host->getIdentifier());
-            break;
+        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);
 }

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

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>,",

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>,",