From: Andrei Pavel Date: Wed, 19 Apr 2023 16:19:44 +0000 (+0300) Subject: [#549] address review comments X-Git-Tag: Kea-2.3.7~13 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c8048badbb308178d4e8dd04490a09d6f75690f5;p=thirdparty%2Fkea.git [#549] address review comments --- diff --git a/doc/sphinx/arm/hooks-class-cmds.rst b/doc/sphinx/arm/hooks-class-cmds.rst index 7d698c8ea1..f3cf788f4e 100644 --- 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 `_ to get the full picture +be achieved by doing a `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 f3cb8a3d60..35de8cd11d 100644 --- 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 cd5c671c18..6c3626cde3 100644 --- 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 `_ / `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 ff5b01423b..7a21d061ce 100644 --- 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 `_. diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.h b/src/hooks/dhcp/lease_cmds/lease_cmds.h index 54ca5f74ab..0a4f386ddd 100644 --- 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 2474574fa7..7d1c652701 100644 --- 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 26574cd352..c5b478dd5e 100644 --- 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 03ab6f87a2..5e53424864 100644 --- 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 7d20f721a2..a0765006d7 100644 --- 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 aea5b711fd..448bca65cf 100644 --- 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 82d039adb1..54208aea01 100644 --- 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 8284dd2a92..cf934dd92a 100644 --- 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 23004ecbde..be342e7a91 100644 --- 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 cedcfcd5ba..8f55a4459f 100644 --- 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 89b6e2bf26..5bf5d79c4e 100644 --- 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 const& identifier(host->getIdentifier()); + deleted = del4(host->getIPv4SubnetID(), host->getIdentifierType(), identifier.data(), + identifier.size()); + } else if (host->getIPv6SubnetID() != SUBNET_ID_UNUSED) { + vector 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 bf8f6a35e6..ceabe704dd 100644 --- 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 3eee6a03fe..ecee118bc5 100644 --- 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\": ,", - " \"comment\": ,", " \"client-id\": ,", " \"circuit-id\": ,", " \"duid\": ,", diff --git a/src/share/api/reservation-update.json b/src/share/api/reservation-update.json index 95853d4bbb..5c0a0b81b1 100644 --- 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\": ,", - " \"comment\": ,", " \"client-id\": ,", " \"circuit-id\": ,", " \"duid\": ,",