[#2765] addressed review comments

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index b7fd394509534109f58b3bd3c2160f6481bcf79d..8c9832004072e84a25315e20dec72d66ce670e9c 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1854,7 +1854,8 @@ Dhcpv4Srv::appendRequestedOptions(Dhcpv4Exchange& ex) {
              desc != range.second; ++desc) {
             // Add the persistent option code to requested options
             if (desc->option_) {
-                static_cast<void>(requested_opts.insert(desc->option_->getType()));
+                uint16_t code = desc->option_->getType();
+                static_cast<void>(requested_opts.insert(code));
             }
         }
     }
@@ -1891,7 +1892,8 @@ Dhcpv4Srv::appendRequestedOptions(Dhcpv4Exchange& ex) {
             OptionVendorClassPtr vendor_opts;
             vendor_opts = boost::dynamic_pointer_cast<OptionVendorClass>(opt.second);
             if (vendor_opts) {
-                static_cast<void>(vendor_ids.insert(vendor_opts->getVendorId()));
+                uint32_t vendor_id = vendor_opts->getVendorId();
+                static_cast<void>(vendor_ids.insert(vendor_id));
             }
         }
         // Iterate on the configured option list.
@@ -1927,7 +1929,8 @@ Dhcpv4Srv::appendRequestedOptions(Dhcpv4Exchange& ex) {
             OptionVendorPtr vendor_opts;
             vendor_opts = boost::dynamic_pointer_cast<OptionVendor>(opt.second);
             if (vendor_opts) {
-                static_cast<void>(vendor_ids.insert(vendor_opts->getVendorId()));
+                uint32_t vendor_id = vendor_opts->getVendorId();
+                static_cast<void>(vendor_ids.insert(vendor_id));
             }
         }
         // Iterate on the configured option list
@@ -1963,6 +1966,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
     Subnet4Ptr subnet = ex.getContext()->subnet_;
 
     const CfgOptionList& co_list = ex.getCfgOptionList();
+
     // Leave if there is no subnet matching the incoming packet.
     // There is no need to log the error message here because
     // it will be logged in the assignLease() when it fails to
@@ -2004,6 +2008,17 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
         }
     }
 
+    // Finally, try to get the vendor-id from the client packet's vendor-class
+    // option (124).
+    for (auto opt : query->getOptions(DHO_VIVCO_SUBOPTIONS)) {
+        OptionVendorClassPtr vendor_class;
+        vendor_class = boost::dynamic_pointer_cast<OptionVendorClass>(opt.second);
+        if (vendor_class) {
+            uint32_t vendor_id = vendor_class->getVendorId();
+            static_cast<void>(vendor_ids.insert(vendor_id));
+        }
+    }
+
     // If there's no vendor option in either request or response, then there's no way
     // to figure out what the vendor-id values are and we give up.
     if (vendor_ids.empty()) {
@@ -2051,7 +2066,8 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
                     continue;
                 }
                 // Add the persistent option code to requested options
-                static_cast<void>(requested_opts[vendor_id].insert(desc->option_->getType()));
+                uint16_t code = desc->option_->getType();
+                static_cast<void>(requested_opts[vendor_id].insert(code));
             }
         }
 

diff --git a/src/bin/dhcp4/tests/vendor_opts_unittest.cc b/src/bin/dhcp4/tests/vendor_opts_unittest.cc
index d5915aead5f5940c4ca0413351d96a570861be36..c0d485227b0fcd8770c776f6ec54b535da2ccde4 100644 (file)
--- a/src/bin/dhcp4/tests/vendor_opts_unittest.cc
+++ b/src/bin/dhcp4/tests/vendor_opts_unittest.cc
@@ -77,15 +77,15 @@ public:
     /// server: 4491 or both 4491 and 3561.
     /// @param requested_vendor_ids Then vendor IDs that are present in ORO.
     /// @param requested_options The requested options in ORO.
-    void testVendorOptionsORO(std::vector<uint32_t> configured_vendor_ids,
-                              std::vector<uint32_t> requested_vendor_ids,
-                              std::vector<uint16_t> requested_options) {
-        std::vector<uint32_t> result_vendor_ids;
+    void testVendorOptionsORO(std::set<uint32_t> configured_vendor_ids,
+                              std::set<uint32_t> requested_vendor_ids,
+                              std::set<uint16_t> requested_options) {
+        std::set<uint32_t> result_vendor_ids;
         ASSERT_FALSE(configured_vendor_ids.empty());
-        ASSERT_EQ(configured_vendor_ids[0], VENDOR_ID_CABLE_LABS);
+        ASSERT_TRUE(configured_vendor_ids.find(VENDOR_ID_CABLE_LABS) != configured_vendor_ids.end());
         for (uint32_t req : requested_vendor_ids) {
             if (req == VENDOR_ID_CABLE_LABS) {
-                result_vendor_ids.push_back(req);
+                result_vendor_ids.insert(req);
             }
         }
         // Create a config with custom options.
@@ -209,8 +209,7 @@ public:
 
         // Let's add a vendor-option (vendor-id=4491) with a single sub-option.
         // That suboption has code 1 and is a docsis ORO option.
-        boost::shared_ptr<OptionUint8Array> vendor_oro(new OptionUint8Array(Option::V4,
-                                                                            DOCSIS3_V4_ORO));
+        OptionUint8ArrayPtr vendor_oro(new OptionUint8Array(Option::V4, DOCSIS3_V4_ORO));
         for (uint16_t option : requested_options) {
             vendor_oro->addValue(option);
         }
@@ -307,26 +306,24 @@ public:
     /// @param add_vendor_option The flag which indicates if the request should
     /// contain a OptionVendor option or should the server always send all the
     /// OptionVendor options and suboptions.
-    void testVendorOptionsPersistent(std::vector<uint32_t> configured_vendor_ids,
-                                     std::vector<uint32_t> requested_vendor_ids,
-                                     std::vector<uint16_t> configured_options,
+    void testVendorOptionsPersistent(std::set<uint32_t> configured_vendor_ids,
+                                     std::set<uint32_t> requested_vendor_ids,
+                                     std::set<uint16_t> configured_options,
                                      bool add_vendor_option) {
-        std::vector<uint32_t> result_vendor_ids;
+        std::set<uint32_t> result_vendor_ids;
         ASSERT_FALSE(configured_vendor_ids.empty());
-        ASSERT_EQ(configured_vendor_ids[0], VENDOR_ID_CABLE_LABS);
+        ASSERT_TRUE(configured_vendor_ids.find(VENDOR_ID_CABLE_LABS) != configured_vendor_ids.end());
         if (add_vendor_option) {
-            for (const auto& req : requested_vendor_ids) {
-                if (std::find(configured_vendor_ids.begin(),
-                              configured_vendor_ids.end(), req) !=
-                    configured_vendor_ids.end()) {
-                    result_vendor_ids.push_back(req);
+            for (uint32_t req : requested_vendor_ids) {
+                if (configured_vendor_ids.find(req) != configured_vendor_ids.end()) {
+                    result_vendor_ids.insert(req);
                 }
             }
         } else {
             result_vendor_ids = configured_vendor_ids;
         }
         ASSERT_FALSE(configured_options.empty());
-        ASSERT_EQ(configured_options[0], DOCSIS3_V4_TFTP_SERVERS);
+        ASSERT_TRUE(configured_options.find(DOCSIS3_V4_TFTP_SERVERS) != configured_options.end());
         // Create a config with custom options.
         string config = R"(
             {
@@ -502,8 +499,7 @@ public:
 
             for (uint16_t option : configured_options) {
                 if (add_vendor_option &&
-                    std::find(requested_vendor_ids.begin(), requested_vendor_ids.end(),
-                              vendor_resp->getVendorId()) == requested_vendor_ids.end()) {
+                    requested_vendor_ids.find(vendor_resp->getVendorId()) == requested_vendor_ids.end()) {
                     // If explicitly sending OptionVendor and the vendor is not
                     // configured, options should not be present.
                     if (option == DOCSIS3_V4_TFTP_SERVERS) {
@@ -582,16 +578,16 @@ public:
     /// @param requested_options The requested options in ORO.
     /// @param configured_options The configured options. The suboption 22 has
     /// always send flag set to true so it will always be sent.
-    void testVendorOptionsOROAndPersistent(std::vector<uint32_t> configured_vendor_ids,
-                                           std::vector<uint32_t> requested_vendor_ids,
-                                           std::vector<uint16_t> requested_options,
-                                           std::vector<uint16_t> configured_options) {
-        std::vector<uint32_t> result_vendor_ids;
+    void testVendorOptionsOROAndPersistent(std::set<uint32_t> configured_vendor_ids,
+                                           std::set<uint32_t> requested_vendor_ids,
+                                           std::set<uint16_t> requested_options,
+                                           std::set<uint16_t> configured_options) {
+        std::set<uint32_t> result_vendor_ids;
         ASSERT_FALSE(configured_vendor_ids.empty());
-        ASSERT_EQ(configured_vendor_ids[0], VENDOR_ID_CABLE_LABS);
+        ASSERT_TRUE(configured_vendor_ids.find(VENDOR_ID_CABLE_LABS) != configured_vendor_ids.end());
         result_vendor_ids = configured_vendor_ids;
         ASSERT_FALSE(configured_options.empty());
-        ASSERT_EQ(configured_options[0], DOCSIS3_V4_TFTP_SERVERS);
+        ASSERT_TRUE(configured_options.find(DOCSIS3_V4_TFTP_SERVERS) != configured_options.end());
         // Create a config with custom options.
         string config = R"(
             {
@@ -766,10 +762,8 @@ public:
             for (uint16_t option : configured_options) {
                 if (option == DOCSIS3_V4_TFTP_SERVERS) {
                     if (vendor_resp->getVendorId() == VENDOR_ID_CABLE_LABS &&
-                        std::find(requested_options.begin(), requested_options.end(),
-                                  option) != requested_options.end() &&
-                        std::find(requested_vendor_ids.begin(), requested_vendor_ids.end(),
-                                  vendor_resp->getVendorId()) != requested_vendor_ids.end()) {
+                        requested_options.find(option) != requested_options.end() &&
+                        requested_vendor_ids.find(vendor_resp->getVendorId()) != requested_vendor_ids.end()) {
                         // Option 2 should be present.
                         OptionPtr docsis2 = vendor_resp->getOption(DOCSIS3_V4_TFTP_SERVERS);
                         ASSERT_TRUE(docsis2);
@@ -1504,7 +1498,7 @@ TEST_F(VendorOptsTest, vivsoInResponseOnly) {
         get(DHCP4_OPTION_SPACE, DHO_VIVSO_SUBOPTIONS);
     ASSERT_TRUE(cdesc.option_);
     // If the config was altered these two EXPECT will fail.
-    EXPECT_EQ(0, cdesc.option_->getOptions().size());
+    EXPECT_TRUE(cdesc.option_->getOptions().empty());
     EXPECT_FALSE(cdesc.option_->getOption(2));
 }
 

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index a2ca314f025114fd8a4c94853aa32199db222e33..9709f3bc6d40a5044829e5fe49f7d932a6dcb293 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -1545,7 +1545,7 @@ Dhcpv6Srv::appendRequestedOptions(const Pkt6Ptr& question, Pkt6Ptr& answer,
                 static_cast<void>(vendor_ids.insert(vendor_id));
             }
         }
-        // Iterate on the configured option list
+        // Iterate on the configured option list.
         for (auto const& copts : co_list) {
             for (OptionDescriptor desc : copts->getList(DHCP6_OPTION_SPACE,
                                                         D6O_VENDOR_CLASS)) {

diff --git a/src/bin/dhcp6/tests/vendor_opts_unittest.cc b/src/bin/dhcp6/tests/vendor_opts_unittest.cc
index 4fdf2cb5d377d66f2155b5dccfbadd3e09ea0830..d96dbcbe3d9b0be647c15f7e141d9209b46cee08 100644 (file)
--- a/src/bin/dhcp6/tests/vendor_opts_unittest.cc
+++ b/src/bin/dhcp6/tests/vendor_opts_unittest.cc
@@ -69,15 +69,15 @@ public:
     /// server: 4491 or both 4491 and 3561.
     /// @param requested_vendor_ids Then vendor IDs that are present in ORO.
     /// @param requested_options The requested options in ORO.
-    void testVendorOptionsORO(std::vector<uint32_t> configured_vendor_ids,
-                              std::vector<uint32_t> requested_vendor_ids,
-                              std::vector<uint16_t> requested_options) {
-        std::vector<uint32_t> result_vendor_ids;
+    void testVendorOptionsORO(std::set<uint32_t> configured_vendor_ids,
+                              std::set<uint32_t> requested_vendor_ids,
+                              std::set<uint16_t> requested_options) {
+        std::set<uint32_t> result_vendor_ids;
         ASSERT_FALSE(configured_vendor_ids.empty());
-        ASSERT_EQ(configured_vendor_ids[0], VENDOR_ID_CABLE_LABS);
+        ASSERT_TRUE(configured_vendor_ids.find(VENDOR_ID_CABLE_LABS) != configured_vendor_ids.end());
         for (uint32_t req : requested_vendor_ids) {
             if (req == VENDOR_ID_CABLE_LABS) {
-                result_vendor_ids.push_back(req);
+                result_vendor_ids.insert(req);
             }
         }
         // Create a config with custom options.
@@ -216,7 +216,7 @@ public:
         // top of the now-absent options.
         OptionCollection tmp = adv->getOptions(D6O_VENDOR_OPTS);
         ASSERT_EQ(tmp.size(), result_vendor_ids.size());
-        if (!result_vendor_ids.size()) {
+        if (result_vendor_ids.empty()) {
             return;
         }
 
@@ -286,26 +286,24 @@ public:
     /// @param add_vendor_option The flag which indicates if the request should
     /// contain a OptionVendor option or should the server always send all the
     /// OptionVendor options and suboptions.
-    void testVendorOptionsPersistent(std::vector<uint32_t> configured_vendor_ids,
-                                     std::vector<uint32_t> requested_vendor_ids,
-                                     std::vector<uint16_t> configured_options,
+    void testVendorOptionsPersistent(std::set<uint32_t> configured_vendor_ids,
+                                     std::set<uint32_t> requested_vendor_ids,
+                                     std::set<uint16_t> configured_options,
                                      bool add_vendor_option) {
-        std::vector<uint32_t> result_vendor_ids;
+        std::set<uint32_t> result_vendor_ids;
         ASSERT_FALSE(configured_vendor_ids.empty());
-        ASSERT_EQ(configured_vendor_ids[0], VENDOR_ID_CABLE_LABS);
+        ASSERT_TRUE(configured_vendor_ids.find(VENDOR_ID_CABLE_LABS) != configured_vendor_ids.end());
         if (add_vendor_option) {
             for (uint32_t req : requested_vendor_ids) {
-                if (std::find(configured_vendor_ids.begin(),
-                              configured_vendor_ids.end(), req) !=
-                    configured_vendor_ids.end()) {
-                    result_vendor_ids.push_back(req);
+                if (configured_vendor_ids.find(req) != configured_vendor_ids.end()) {
+                    result_vendor_ids.insert(req);
                 }
             }
         } else {
             result_vendor_ids = configured_vendor_ids;
         }
         ASSERT_FALSE(configured_options.empty());
-        ASSERT_EQ(configured_options[0], DOCSIS3_V6_CONFIG_FILE);
+        ASSERT_TRUE(configured_options.find(DOCSIS3_V6_CONFIG_FILE) != configured_options.end());
         // Create a config with custom options.
         string config = R"(
             {
@@ -470,8 +468,7 @@ public:
 
             for (uint16_t option : configured_options) {
                 if (add_vendor_option &&
-                    std::find(requested_vendor_ids.begin(), requested_vendor_ids.end(),
-                              vendor_resp->getVendorId()) == requested_vendor_ids.end()) {
+                    requested_vendor_ids.find(vendor_resp->getVendorId()) == requested_vendor_ids.end()) {
                     // If explicitly sending OptionVendor and the vendor is not
                     // configured, options should not be present.
                     if (option == DOCSIS3_V6_CONFIG_FILE) {
@@ -534,16 +531,16 @@ public:
     /// @param requested_options The requested options in ORO.
     /// @param configured_options The configured options. The suboption 22 has
     /// always send flag set to true so it will always be sent.
-    void testVendorOptionsOROAndPersistent(std::vector<uint32_t> configured_vendor_ids,
-                                           std::vector<uint32_t> requested_vendor_ids,
-                                           std::vector<uint16_t> requested_options,
-                                           std::vector<uint16_t> configured_options) {
-        std::vector<uint32_t> result_vendor_ids;
+    void testVendorOptionsOROAndPersistent(std::set<uint32_t> configured_vendor_ids,
+                                           std::set<uint32_t> requested_vendor_ids,
+                                           std::set<uint16_t> requested_options,
+                                           std::set<uint16_t> configured_options) {
+        std::set<uint32_t> result_vendor_ids;
         ASSERT_FALSE(configured_vendor_ids.empty());
-        ASSERT_EQ(configured_vendor_ids[0], VENDOR_ID_CABLE_LABS);
+        ASSERT_TRUE(configured_vendor_ids.find(VENDOR_ID_CABLE_LABS) != configured_vendor_ids.end());
         result_vendor_ids = configured_vendor_ids;
         ASSERT_FALSE(configured_options.empty());
-        ASSERT_EQ(configured_options[0], DOCSIS3_V6_CONFIG_FILE);
+        ASSERT_TRUE(configured_options.find(DOCSIS3_V6_CONFIG_FILE) != configured_options.end());
         // Create a config with custom options.
         string config = R"(
             {
@@ -665,8 +662,7 @@ public:
 
         // Let's add a vendor-option (vendor-id=4491) with a single sub-option.
         // That suboption has code 1 and is a docsis ORO option.
-        boost::shared_ptr<OptionUint16Array> vendor_oro(new OptionUint16Array(Option::V6,
-                                                                              DOCSIS3_V6_ORO));
+        OptionUint16ArrayPtr vendor_oro(new OptionUint16Array(Option::V6, DOCSIS3_V6_ORO));
         for (uint16_t option : requested_options) {
             vendor_oro->addValue(option);
         }
@@ -708,10 +704,8 @@ public:
             for (uint16_t option : configured_options) {
                 if (option == DOCSIS3_V6_CONFIG_FILE) {
                     if (vendor_resp->getVendorId() == VENDOR_ID_CABLE_LABS &&
-                        std::find(requested_options.begin(), requested_options.end(),
-                                  option) != requested_options.end() &&
-                        std::find(requested_vendor_ids.begin(), requested_vendor_ids.end(),
-                                  vendor_resp->getVendorId()) != requested_vendor_ids.end()) {
+                        requested_options.find(option) != requested_options.end() &&
+                        requested_vendor_ids.find(vendor_resp->getVendorId()) != requested_vendor_ids.end()) {
                         // Option 33 should be present.
                         OptionPtr docsis33 = vendor_resp->getOption(DOCSIS3_V6_CONFIG_FILE);
                         ASSERT_TRUE(docsis33);
@@ -1303,7 +1297,7 @@ TEST_F(VendorOptsTest, vendorOptionsOROAndPersistentMultipleOptionsDifferentVend
 // provided and vendor options are expected to not be present in the response.
 TEST_F(VendorOptsTest, vendorOptionsOROAndPersistentOneOptionDifferentVendorIDMultipleVendorsMatchNoneNoneRequested) {
     testVendorOptionsOROAndPersistent({ VENDOR_ID_CABLE_LABS, 3561 },
-                                      { 32768, 16384},
+                                      { 32768, 16384 },
                                       {},
                                       { DOCSIS3_V6_CONFIG_FILE });
 }
@@ -1312,7 +1306,7 @@ TEST_F(VendorOptsTest, vendorOptionsOROAndPersistentOneOptionDifferentVendorIDMu
 // provided and vendor options are expected to not be present in the response.
 TEST_F(VendorOptsTest, vendorOptionsOROAndPersistentMultipleOptionDifferentVendorIDMultipleVendorsMatchNoneNoneRequested) {
     testVendorOptionsOROAndPersistent({ VENDOR_ID_CABLE_LABS, 3561 },
-                                      { 32768, 16384},
+                                      { 32768, 16384 },
                                       {},
                                       { DOCSIS3_V6_CONFIG_FILE, 12 });
 }
@@ -1489,7 +1483,7 @@ TEST_F(VendorOptsTest, vendorOpsInResponseOnly) {
         get(DHCP6_OPTION_SPACE, D6O_VENDOR_OPTS);
     ASSERT_TRUE(cdesc.option_);
     // If the config was altered these two EXPECT will fail.
-    EXPECT_EQ(0, cdesc.option_->getOptions().size());
+    EXPECT_TRUE(cdesc.option_->getOptions().empty());
     EXPECT_FALSE(cdesc.option_->getOption(2));
 }