[#3770] addressed review comments

src/bin/admin/tests/mysql_tests.sh.in

@@ -1889,7 +1889,7 @@ SET @disable_audit = 0"
     # Check upgrade from 29.0 to 30.0.
     mysql_upgrade_29_to_30_test
 
-    # Check upgrade from 29.0 to 30.0.
+    # Check upgrade from 30.0 to 31.0.
     mysql_upgrade_30_to_31_test
 
     # Let's wipe the whole database
@@ -3994,7 +3994,7 @@ mysql_migrate_client_class_test() {
 
 # Verifies migration of dhcpX_options.client_classes column to not null.
 mysql_migrate_dhcpX_options_client_classes() {
-    test_start "mysql.mysql_migrate_dhcp4_options_client_classes"
+    test_start "mysql.mysql_migrate_dhcpX_options_client_classes"
 
     # Let's wipe the whole database
     mysql_wipe
@@ -4031,6 +4031,7 @@ mysql_migrate_dhcpX_options_client_classes() {
     # Verify the inserted record counts.
     qry="select count(*) from dhcp4_options;"
     run_statement "#get 4_option_count before update" "$qry" 2
+
     qry="select count(*) from dhcp6_options;"
     run_statement "#get 6_option_count before update" "$qry" 2
 

src/hooks/dhcp/mysql/mysql_cb_impl.cc

@@ -562,7 +562,7 @@ MySqlConfigBackendImpl::getOption(const int index,
                                   const ServerSelector& server_selector,
                                   const uint16_t code,
                                   const std::string& space,
-                                  const ClientClassesPtr client_classes) {
+                                  const ClientClassesPtr client_classes /* = ClientClassesPtr() */) {
 
     if (server_selector.amUnassigned()) {
         isc_throw(NotImplemented, "managing configuration for no particular server"
@@ -581,7 +581,7 @@ MySqlConfigBackendImpl::getOption(const int index,
     }
     in_bindings.push_back(MySqlBinding::createString(space));
 
-    /// @todo Remove the if when v6 is ready for this.
+    /// @todo TKM Remove the if when v6 is ready.
     if (universe == Option::V4) {
         in_bindings.push_back(createClientClassesForWhereClause(client_classes));
     }

src/hooks/dhcp/mysql/mysql_host_data_source.cc

@@ -973,7 +973,7 @@ private:
                 user_context.assign(user_context_);
             }
 
-            // Convert clietn classes to string.
+            // Convert client classes to string.
             std::string client_classes;
             if (client_classes_null_ == MLM_FALSE) {
                 client_classes_[client_classes_length_] = '\0';

src/hooks/dhcp/pgsql/pgsql_cb_dhcp4.h

@@ -507,10 +507,8 @@ public:
     /// @return Number of deleted options.
     /// @throw NotImplemented if server selector is "unassigned".
     virtual uint64_t
-    deleteOption4(const db::ServerSelector& server_selector,
+    deleteOption4(const db::ServerSelector& server_selector, const SubnetID& subnet_id,
-                  const SubnetID& subnet_id,
+                  const uint16_t code, const std::string& space,
-                  const uint16_t code,
-                  const std::string& space,
                   const ClientClassesPtr client_classes = ClientClassesPtr());
 
     /// @brief Deletes pool level option.

src/hooks/dhcp/pgsql/pgsql_cb_impl.cc

@@ -548,8 +548,7 @@ PgSqlConfigBackendImpl::getOption(const int index,
                                   const ServerSelector& server_selector,
                                   const uint16_t code,
                                   const std::string& space,
-                                  const ClientClassesPtr client_classes 
+                                  const ClientClassesPtr client_classes /* = ClientClassesPtr() */) {
-                                  /* = ClientClassesPtr() */) {
     if (server_selector.amUnassigned()) {
         isc_throw(NotImplemented, "managing configuration for no particular server"
                                   " (unassigned) is unsupported at the moment");

src/hooks/dhcp/pgsql/pgsql_cb_impl.h

@@ -645,8 +645,7 @@ public:
     /// @param bindings PsqlBindArray to which the option value should be added.
     /// @param client_classes ClientClasses collection containing the class names.
     void addClientClassesForWhereClause(db::PsqlBindArray& bindings,
-                                        const ClientClassesPtr client_classes
+                                        const ClientClassesPtr client_classes = ClientClassesPtr());
-                                        = ClientClassesPtr());
 
     /// @brief Iterates over the class names in a JSON list element at a
     /// given column, adding each name to the given ClientClasses instance.

src/lib/dhcpsrv/cfg_option.cc

@@ -140,7 +140,7 @@ CfgOption::replace(const OptionDescriptor& desc, const std::string& option_space
     if (od_itr == idx6.end()) {
         isc_throw(isc::BadValue, "cannot replace option: "
                   << option_space << ":" << desc.option_->getType()
-                  << " , client-classes: " << desc.client_classes_.toText()
+                  << ", client-classes: " << desc.client_classes_.toText()
                   << ", it does not exist");
     }
 
@@ -264,7 +264,7 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp
     }
 
     // Indicate we replaced the definition.
-    return(true);
+    return (true);
 }
 
 void
@@ -456,7 +456,7 @@ CfgOption::del(const std::string& option_space, const uint16_t option_code,
         idx6.erase(range.first, range.second);
     }
 
-    return(count);
+    return (count);
 }
 
 size_t

src/lib/dhcpsrv/config_backend_pool_dhcp4.cc

@@ -139,8 +139,7 @@ ConfigBackendPoolDHCPv4::getOption4(const BackendSelector& backend_selector,
                                     const ServerSelector& server_selector,
                                     const uint16_t code,
                                     const std::string& space,
-                                    const ClientClassesPtr client_classes
+                                    const ClientClassesPtr client_classes /* = ClientClassesPtr() */) const {
-                                    /* = ClientClassesPtr */) const {
     OptionDescriptorPtr option;
     getPropertyPtrConst<OptionDescriptorPtr, uint16_t, const std::string&>
         (&ConfigBackendDHCPv4::getOption4, backend_selector, server_selector,
@@ -434,7 +433,7 @@ ConfigBackendPoolDHCPv4::deleteOption4(const BackendSelector& backend_selector,
                                        const ServerSelector& server_selector,
                                        const uint16_t code,
                                        const std::string& space,
-                                       const ClientClassesPtr client_classes /* = ClientClassesPtr */) {
+                                       const ClientClassesPtr client_classes /* = ClientClassesPtr() */) {
 
     return (createUpdateDeleteProperty<uint64_t, uint16_t, const std::string&>
             (&ConfigBackendDHCPv4::deleteOption4, backend_selector, server_selector,
@@ -447,7 +446,7 @@ ConfigBackendPoolDHCPv4::deleteOption4(const BackendSelector& backend_selector,
                                        const std::string& shared_network_name,
                                        const uint16_t code,
                                        const std::string& space,
-                                       const ClientClassesPtr client_classes /* = ClientClassesPtr */) {
+                                       const ClientClassesPtr client_classes /* = ClientClassesPtr() */) {
     return (createUpdateDeleteProperty<uint64_t, const std::string&, uint16_t,
                                        const std::string&>
             (&ConfigBackendDHCPv4::deleteOption4, backend_selector, server_selector,
@@ -460,7 +459,7 @@ ConfigBackendPoolDHCPv4::deleteOption4(const BackendSelector& backend_selector,
                                        const SubnetID& subnet_id,
                                        const uint16_t code,
                                        const std::string& space,
-                                       const ClientClassesPtr client_classes /* = ClientClassesPtr */) {
+                                       const ClientClassesPtr client_classes /* = ClientClassesPtr() */) {
     return (createUpdateDeleteProperty<uint64_t, const SubnetID&, uint16_t, const std::string&>
             (&ConfigBackendDHCPv4::deleteOption4, backend_selector, server_selector,
              subnet_id, code, space, client_classes));
@@ -473,7 +472,7 @@ ConfigBackendPoolDHCPv4::deleteOption4(const BackendSelector& backend_selector,
                                        const asiolink::IOAddress& pool_end_address,
                                        const uint16_t code,
                                        const std::string& space,
-                                       const ClientClassesPtr client_classes /* = ClientClassesPtr */) {
+                                       const ClientClassesPtr client_classes /* = ClientClassesPtr() */) {
     return (createUpdateDeleteProperty<uint64_t, const IOAddress&, const IOAddress&,
                                        uint16_t, const std::string&>
             (&ConfigBackendDHCPv4::deleteOption4, backend_selector, server_selector,

src/lib/dhcpsrv/tests/cfg_option_unittest.cc

@@ -1509,14 +1509,14 @@ TEST_F(CfgOptionTest, optionsWithClientClasses) {
     OptionDescriptorList list_options = cfg.getList(DHCP6_OPTION_SPACE, 777);
     ASSERT_EQ(options->size(), reference_options.size());
     auto reference_rdesc = reference_options.rbegin();
-    for (auto &returned_desc : list_options ){
+    for (auto &returned_desc : list_options) {
         ASSERT_EQ(*reference_rdesc, returned_desc);
         ++reference_rdesc;
     }
 
     // Verify that CfgOption::get() with client classes returns
     // each one correctly.
-    for (auto &reference_desc : reference_options ){
+    for (auto &reference_desc : reference_options) {
         OptionDescriptor found_desc = cfg.get(DHCP6_OPTION_SPACE, 777,
                                               reference_desc.client_classes_);
         ASSERT_TRUE(found_desc.option_);
@@ -1561,7 +1561,7 @@ TEST_F(CfgOptionTest, replaceWithClientClasses) {
     (boost::dynamic_pointer_cast<OptionUint16>(replacement.option_))->setValue(100);
     ASSERT_NO_THROW(cfg.replace(replacement, DHCP6_OPTION_SPACE));
 
-    // Make sure we can the updated option.
+    // Make sure we can get the updated option.
     OptionDescriptor found_desc = cfg.get(DHCP6_OPTION_SPACE, 777,
                                           replacement.client_classes_);
     ASSERT_TRUE(found_desc.option_);
@@ -1572,7 +1572,7 @@ TEST_F(CfgOptionTest, replaceWithClientClasses) {
     (boost::dynamic_pointer_cast<OptionUint16>(replacement2.option_))->setValue(300);
     ASSERT_NO_THROW(cfg.replace(replacement2, DHCP6_OPTION_SPACE));
 
-    // Make sure we can the updated option.
+    // Make sure we can get the updated option.
     found_desc = cfg.get(DHCP6_OPTION_SPACE, 777, replacement2.client_classes_);
     ASSERT_TRUE(found_desc.option_);
     ASSERT_EQ(found_desc, replacement2);
@@ -1631,7 +1631,7 @@ TEST_F(CfgOptionTest, deleteWithClientClasses) {
     ASSERT_FALSE(found_desc.option_);
     reference_options.pop_back();
 
-    // Delete the secon reference option.
+    // Delete the second reference option.
     ASSERT_NO_THROW(cfg.del(DHCP6_OPTION_SPACE, 777, reference_options[1].client_classes_));
 
     // Make sure we can no longer find the deleted option.

src/lib/dhcpsrv/testutils/generic_cb_dhcp4_unittest.cc

@@ -150,7 +150,6 @@ GenericConfigBackendDHCPv4Test::initTestSubnets() {
     subnet->getCfgOption()->add(*test_options_[1], test_options_[1]->space_name_);
     subnet->getCfgOption()->add(*test_options_[2], test_options_[2]->space_name_);
 
-
     test_subnets_.push_back(subnet);
 
     // Adding another subnet with the same subnet id to test
@@ -3663,7 +3662,7 @@ GenericConfigBackendDHCPv4Test::makeClassTaggedOptions() {
     };
 
     std::list<OptionDescriptorPtr> tagged_options;
-    for ( auto const& opt_to_make : opts_to_make) {
+    for (auto const& opt_to_make : opts_to_make) {
         OptionDescriptor desc = createOption<OptionString>(Option::V4, opt_to_make.code_,
                                                            true, false, false, opt_to_make.value_);
         desc.space_name_ = DHCP4_OPTION_SPACE;
@@ -3680,7 +3679,7 @@ GenericConfigBackendDHCPv4Test::makeClassTaggedOptions() {
 void
 GenericConfigBackendDHCPv4Test::updateClassTaggedOptions(
     std::list<OptionDescriptorPtr>& options) {
-    for ( auto& desc : options) {
+    for (auto& desc : options) {
         OptionStringPtr opt = boost::dynamic_pointer_cast<OptionString>(desc->option_);
         ASSERT_TRUE(opt);
         std::string new_value(opt->getValue() + std::string(".") + opt->getValue());
@@ -3688,7 +3687,7 @@ GenericConfigBackendDHCPv4Test::updateClassTaggedOptions(
     }
 }
 
-// Macro the make SCOPED_TRACE around equivalance functon more compact and helpful.
+// Macro the make SCOPED_TRACE around equivalance function more compact and helpful.
 #define SCOPED_OPT_COMPARE(exp_opt,test_opt)\
 {\
     std::stringstream oss;\
@@ -3758,7 +3757,6 @@ GenericConfigBackendDHCPv4Test::globalOption4WithClientClassesTest() {
     }
 }
 
-
 void
 GenericConfigBackendDHCPv4Test::getAllOptions4WithClientClassesTest() {
     // Describes an option to create.
@@ -3789,7 +3787,7 @@ GenericConfigBackendDHCPv4Test::getAllOptions4WithClientClassesTest() {
 
     // Add all of the global options.
     std::vector<OptionDescriptorPtr> ref_options;
-    for ( auto const& opt_to_make : opts_to_make) {
+    for (auto const& opt_to_make : opts_to_make) {
         OptionDescriptor desc = createOption<OptionInt<uint32_t>>(Option::V4, opt_to_make.code_,
                                                                   true, false, false, opt_to_make.value_);
         desc.space_name_ = DHCP4_OPTION_SPACE;
@@ -3819,7 +3817,7 @@ GenericConfigBackendDHCPv4Test::getAllOptions4WithClientClassesTest() {
         ++exp_option;
     }
 
-    // Try to fetch the collection of global options for the server1.
+    // Try to fetch the collection of global options for the server2.
     // Build list of options we expect to get back.
     exp_options.clear();
     exp_options.push_back(ref_options[3]);
@@ -3849,7 +3847,6 @@ GenericConfigBackendDHCPv4Test::getAllOptions4WithClientClassesTest() {
     }
 }
 
-
 void
 GenericConfigBackendDHCPv4Test::createUpdateDeleteSubnetOption4Test() {
     // Insert new subnet.
@@ -4940,7 +4937,7 @@ GenericConfigBackendDHCPv4Test::sharedNetworkOption4WithClientClassesTest() {
     // Make a network with options.
     SharedNetwork4Ptr network(new SharedNetwork4("net1"));
     auto ref_options = makeClassTaggedOptions();
-    for ( auto const& ref_option : ref_options) {
+    for (auto const& ref_option : ref_options) {
         network->getCfgOption()->add(*ref_option, ref_option->space_name_);
     }
 
@@ -4979,7 +4976,6 @@ GenericConfigBackendDHCPv4Test::sharedNetworkOption4WithClientClassesTest() {
     }
 
     // Now make sure that we can delete the options individually.
-    updateClassTaggedOptions(ref_options);
     for (auto const& ref_option : ref_options) {
         ASSERT_EQ(1, cbptr_->deleteOption4(ServerSelector::ANY(),
                                            network->getName(),
@@ -5005,7 +5001,7 @@ GenericConfigBackendDHCPv4Test::subnetOption4WithClientClassesTest() {
     Subnet4Ptr subnet(new Subnet4(IOAddress("192.0.2.0"), 24,
                                   30, 40, 60, 1024));
     auto ref_options = makeClassTaggedOptions();
-    for ( auto const& ref_option : ref_options) {
+    for (auto const& ref_option : ref_options) {
         subnet->getCfgOption()->add(*ref_option, ref_option->space_name_);
     }
 
@@ -5043,7 +5039,6 @@ GenericConfigBackendDHCPv4Test::subnetOption4WithClientClassesTest() {
     }
 
     // Now make sure that we can delete the options individually.
-    updateClassTaggedOptions(ref_options);
     for (auto const& ref_option : ref_options) {
         ASSERT_EQ(1, cbptr_->deleteOption4(ServerSelector::ANY(),
                                            subnet->getID(),
@@ -5074,7 +5069,7 @@ GenericConfigBackendDHCPv4Test::poolOption4WithClientClassesTest() {
 
     // Add the options to the pool.
     auto ref_options = makeClassTaggedOptions();
-    for ( auto const& ref_option : ref_options) {
+    for (auto const& ref_option : ref_options) {
         pool->getCfgOption()->add(*ref_option, ref_option->space_name_);
     }
 

src/lib/dhcpsrv/testutils/test_config_backend_dhcp4.cc

@@ -469,7 +469,6 @@ TestConfigBackendDHCPv4::getGlobalParameter4(const db::ServerSelector& server_se
     return (candidate);
 }
 
-
 StampedValueCollection
 TestConfigBackendDHCPv4::getAllGlobalParameters4(const db::ServerSelector& server_selector) const {
     auto tags = server_selector.getTags();
@@ -1237,7 +1236,6 @@ TestConfigBackendDHCPv4::deleteOption4(const db::ServerSelector& server_selector
         }
     }
 
-
     if (!found) {
         isc_throw(BadValue, "attempted to delete option in a "
                   "shared network " << shared_network_name

src/lib/dhcpsrv/testutils/test_config_backend_dhcp4.h

@@ -444,8 +444,7 @@ public:
     /// Defaults to an empty pointer.
     /// @return Number of deleted options.
     virtual uint64_t
-    deleteOption4(const db::ServerSelector& server_selector,
+    deleteOption4(const db::ServerSelector& server_selector, const uint16_t code,
-                  const uint16_t code,
                   const std::string& space,
                   ClientClassesPtr client_classes = ClientClassesPtr());
 

src/share/database/scripts/mysql/upgrade_030_to_031.sh.in

@@ -1,6 +1,7 @@
 #!/bin/sh
 
 # Copyright (C) 2025 Internet Systems Consortium, Inc. ("ISC") #
+#
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.