]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[#1518] addressed review comments
authorRazvan Becheriu <razvan@isc.org>
Wed, 18 Jan 2023 16:07:52 +0000 (18:07 +0200)
committerRazvan Becheriu <razvan@isc.org>
Fri, 20 Jan 2023 15:36:06 +0000 (17:36 +0200)
doc/sphinx/arm/dhcp4-srv.rst
doc/sphinx/arm/dhcp6-srv.rst
src/lib/dhcp/libdhcp++.cc
src/lib/dhcp/option_vendor.cc
src/lib/dhcp/option_vendor.h
src/lib/dhcp/tests/libdhcp++_unittest.cc

index b6852eb473676b9b27dc106f5aef00734e4a566e..217289ecfb2954b73cc71df3796c307dc95eeb69 100644 (file)
@@ -2495,54 +2495,56 @@ following:
 
 ::
 
-   {
+   "Dhcp4": {
        // First, we need to define that the sub-option 2 in vivso option for
        // vendor-id 25167 has a specific format (it's a plain string in this example).
        // After this definition, we can specify values for option tftp.
        "option-def": [
-       {
-           // We define a short name, so the option can be referenced by name.
-           // The option has code 2 and resides within vendor space 25167.
-           // Its data is a plain string.
-           "name": "tftp",
-           "code": 2,
-           "space": "vendor-25167",
-           "type": "string"
-       } ],
+           {
+               // We define a short name, so the option can be referenced by name.
+               // The option has code 2 and resides within vendor space 25167.
+               // Its data is a plain string.
+               "name": "tftp",
+               "code": 2,
+               "space": "vendor-25167",
+               "type": "string"
+           }
+       ],
 
        "client-classes": [
-       {
-           // We now need to tell Kea how to recognize when to use vendor space 25167.
-           // Usually we can use a simple expression, such as checking if the device
-           // sent a vivso option with specific vendor-id, e.g. "vendor[4491].exists".
-           // Unfortunately, Genexis is a bit unusual in this aspect, because it
-           // doesn't send vivso. In this case we need to look into the vendor class
-           // (option code 60) and see if there's a specific string that identifies
-           // the device.
-           "name": "cpe_genexis",
-           "test": "substring(option[60].hex,0,7) == 'HMC1000'",
-
-           // Once the device is recognized, we want to send two options:
-           // the vivso option with vendor-id set to 25167, and a sub-option 2.
-           "option-data": [
-               {
-                   "name": "vivso-suboptions",
-                   "data": "25167"
-               },
+           {
+               // We now need to tell Kea how to recognize when to use vendor space 25167.
+               // Usually we can use a simple expression, such as checking if the device
+               // sent a vivso option with specific vendor-id, e.g. "vendor[4491].exists".
+               // Unfortunately, Genexis is a bit unusual in this aspect, because it
+               // doesn't send vivso. In this case we need to look into the vendor class
+               // (option code 60) and see if there's a specific string that identifies
+               // the device.
+               "name": "cpe_genexis",
+               "test": "substring(option[60].hex,0,7) == 'HMC1000'",
+
+               // Once the device is recognized, we want to send two options:
+               // the vivso option with vendor-id set to 25167, and a sub-option 2.
+               "option-data": [
+                   {
+                       "name": "vivso-suboptions",
+                       "data": "25167"
+                   },
 
-               // The sub-option 2 value is defined as any other option. However,
-               // we want to send this sub-option 2, even when the client didn't
-               // explicitly request it (often there is no way to do that for
-               // vendor options). Therefore we use always-send to force Kea
-               // to always send this option when 25167 vendor space is involved.
-               {
-                   "name": "tftp",
-                   "space": "vendor-25167",
-                   "data": "tftp://192.0.2.1/genexis/HMC1000.v1.3.0-R.img",
-                   "always-send": true
-               }
-           ]
-       } ]
+                   // The sub-option 2 value is defined as any other option. However,
+                   // we want to send this sub-option 2, even when the client didn't
+                   // explicitly request it (often there is no way to do that for
+                   // vendor options). Therefore we use always-send to force Kea
+                   // to always send this option when 25167 vendor space is involved.
+                   {
+                       "name": "tftp",
+                       "space": "vendor-25167",
+                       "data": "tftp://192.0.2.1/genexis/HMC1000.v1.3.0-R.img",
+                       "always-send": true
+                   }
+               ]
+           }
+       ]
    }
 
 By default, Kea sends back only those options that are requested by a client,
@@ -2565,12 +2567,60 @@ that in this particular case an option is defined in vendor space 25167. With
 with vendor space 25167.
 This is also how the Kea server can be configured to send multiple vendor
 enterprise numbers and multiple options, specific for each vendor.
+If these options need to be sent by the server regardless if the client
+specified any enterprise number, the ``"always-send": true`` must be configured
+for the option with code 125 (``vivso-suboptions``) for each enterprise number.
+
+::
+
+   "Dhcp4": {
+       "option-data": [
+           {
+               "always-send": true,
+               "name": "vivso-suboptions",
+               "space": "dhcp4",
+               "data": "2234"
+           },
+           {
+               "always-send": true,
+               "name": "vivso-suboptions",
+               "space": "dhcp4",
+               "data": "3561"
+           },
+           {
+               "always-send": true,
+               "data": "tagged",
+               "name": "tag",
+               "space": "vendor-2234"
+           },
+           {
+               "always-send": true,
+               "name": "url",
+               "space": "vendor-3561",
+               "data": "https://example.com:1234/path"
+           }
+       ],
+       "option-def": [
+           {
+               "code": 22,
+               "name": "tag",
+               "space": "vendor-2234",
+               "type": "string"
+           },
+           {
+               "code": 11,
+               "name": "url",
+               "space": "vendor-3561",
+               "type": "string"
+           }
+       ]
+   }
 
 Another possibility is to redefine the option; see :ref:`dhcp4-private-opts`.
 
 Kea comes with several example configuration files. Some of them showcase
 how to configure options 60 and 43. See ``doc/examples/kea4/vendor-specific.json``
-and ``doc/examples/kea6/vivso.json`` in the Kea sources.
+and ``doc/examples/kea4/vivso.json`` in the Kea sources.
 
 .. note::
 
index 68c6123d7b4131d4879c67a6fab8e6e429c7d9af..c06e68bf0e4e051c0339281b01ec6f980542a062 100644 (file)
@@ -6678,7 +6678,7 @@ option is actually needed. An example configuration looks as follows:
        "subnet6": [ {
            "pd-pools": [
            {
-               "prefix":  "2001:db8::",
+               "prefix": "2001:db8::",
                "prefix-len": 56,
                "delegated-len": 64,
 
index 20c1c00cf2fd773cb89c4b31fd0167efcd7c3762..c6dd3de7ef3d80faacd89c8e949c883d53fa3d06 100644 (file)
@@ -688,39 +688,44 @@ LibDHCP::fuseOptions4(OptionCollection& options) {
 void
 LibDHCP::extendVendorOptions4(isc::dhcp::OptionCollection& options) {
     map<uint32_t, OptionCollection> vendors_data;
-    for (auto const& option : options) {
-        if (option.second->getType() == DHO_VIVSO_SUBOPTIONS) {
-            uint32_t offset = 0;
-            auto const& data = option.second->getData();
-            auto const& begin = data.begin();
-            auto const& end = data.end();
-            size_t size;
-            while ((size = distance(begin + offset, end)) != 0) {
-                if (size < sizeof(uint32_t)) {
-                    options.erase(DHO_VIVSO_SUBOPTIONS);
-                    isc_throw(SkipRemainingOptionsError,
-                              "Truncated vendor-specific information option"
-                              << ", length=" << size);
-                }
-                uint32_t vendor_id = isc::util::readUint32(&(*begin) + offset, distance(begin, end));
-                offset += 4;
-                OptionBuffer vendor_buffer(begin + offset, end);
-                try {
-                    offset += LibDHCP::unpackVendorOptions4(vendor_id, vendor_buffer, vendors_data[vendor_id]);
-                } catch (const SkipThisOptionError&)  {
-                } catch (const Exception&) {
-                    options.erase(DHO_VIVSO_SUBOPTIONS);
-                    throw;
-                }
+    auto const& it = options.find(DHO_VIVSO_SUBOPTIONS);
+    // After calling fuseOptions4 there should be at most one option of
+    // DHO_VIVSO_SUBOPTIONS type.
+    if (it != options.end()) {
+        uint32_t offset = 0;
+        auto const& data = it->second->getData();
+        auto const& begin = data.begin();
+        size_t size;
+        while ((size = data.size() - offset) != 0) {
+            if (size < sizeof(uint32_t)) {
+                options.erase(DHO_VIVSO_SUBOPTIONS);
+                isc_throw(SkipRemainingOptionsError,
+                          "Truncated vendor-specific information option"
+                          << ", length=" << size);
+            }
+            uint32_t vendor_id = isc::util::readUint32(&(*begin) + offset, data.size());
+            offset += 4;
+            const OptionBuffer vendor_buffer(begin + offset, data.end());
+            try {
+                offset += LibDHCP::unpackVendorOptions4(vendor_id, vendor_buffer, vendors_data[vendor_id]);
+            } catch (const SkipThisOptionError&) {
+                // Ignore this kind of error and continue.
+            } catch (const Exception&) {
+                options.erase(DHO_VIVSO_SUBOPTIONS);
+                throw;
             }
         }
     }
+    // Delete the initial option.
     options.erase(DHO_VIVSO_SUBOPTIONS);
+    // Create a new instance of OptionVendor for each enterprise ID.
     for (auto const& vendor : vendors_data) {
         OptionVendorPtr vendor_opt(new OptionVendor(Option::V4, vendor.first));
         for (auto const& option : vendor.second) {
             vendor_opt->addOption(option.second);
         }
+        // Add the new instance of VendorOption with respective sub-options for
+        // this enterprise ID.
         options.insert(std::make_pair(DHO_VIVSO_SUBOPTIONS, vendor_opt));
     }
 }
index d4b9b9baeebf368030327a2a94a8111e6f4080eb..8ab9b4649b7adb259837a34cae8b740ea74316e6 100644 (file)
@@ -27,11 +27,6 @@ OptionVendor::OptionVendor(Option::Universe u, OptionBufferConstIter begin,
     unpack(begin, end);
 }
 
-OptionVendor::OptionVendor(const OptionVendor& option) : Option(option),
-        vendor_id_(option.vendor_id_) {
-    Option::operator=(option);
-}
-
 OptionPtr
 OptionVendor::clone() const {
     return (cloneInternal<OptionVendor>());
@@ -69,8 +64,6 @@ void OptionVendor::unpack(OptionBufferConstIter begin,
                   << ", length=" << distance(begin, end));
     }
 
-    options_.clear();
-
     vendor_id_ = isc::util::readUint32(&(*begin), distance(begin, end));
 
     OptionBuffer vendor_buffer(begin + 4, end);
@@ -120,11 +113,3 @@ OptionVendor::toText(int indent) const {
 
     return (output.str());
 }
-
-OptionVendor& OptionVendor::operator=(const OptionVendor& rhs) {
-    if (&rhs != this) {
-        Option::operator=(rhs);
-        vendor_id_ = rhs.vendor_id_;
-    }
-    return (*this);
-}
index b3f81ff20fe2894fc32a33393795739d963c8ba1..73b1b4784e2ad36202f2c5c4a92b449da3fe815f 100644 (file)
@@ -50,14 +50,6 @@ public:
     OptionVendor(Option::Universe u, OptionBufferConstIter begin,
                  OptionBufferConstIter end);
 
-    /// @brief Copy constructor.
-    ///
-    /// This constructor makes a deep copy of the option and all of the
-    /// suboptions. It calls @ref getOptionsCopy to deep copy suboptions.
-    ///
-    /// @param source Option to be copied.
-    OptionVendor(const OptionVendor& option);
-
     /// @brief Copies this option and returns a pointer to the copy.
     OptionPtr clone() const;
 
@@ -110,15 +102,6 @@ public:
     /// @return Vendor option in the textual format.
     virtual std::string toText(int indent = 0) const;
 
-    /// @brief Assignment operator.
-    ///
-    /// The assignment operator performs a deep copy of the option and
-    /// its suboptions. It calls @ref getOptionsCopy to deep copy
-    /// suboptions.
-    ///
-    /// @param rhs Option to be assigned.
-    OptionVendor& operator=(const OptionVendor& rhs);
-
 private:
 
     /// @brief Enterprise-id
index 1896b772e0ae465e24e479deb4907c6005bd2f76..7e83ce15b9c7646b1a6d61d372a56e98b47ef809 100644 (file)
@@ -1107,20 +1107,21 @@ TEST_F(LibDhcpTest, extentVendorOptions4) {
     }
     ASSERT_NO_THROW(LibDHCP::fuseOptions4(options));
     ASSERT_EQ(options.size(), 1);
+    options.insert(make_pair(2, opt3));
+    options.insert(make_pair(1, opt4));
+    ASSERT_EQ(options.size(), 3);
     container.clear();
-    container.insert(make_pair(1, options.begin()->second));
-    container.insert(make_pair(2, opt3));
-    container.insert(make_pair(1, opt4));
-    ASSERT_EQ(container.size(), 3);
-    options.clear();
-    for (auto const& option : container) {
+    for (auto const& option : options) {
         const OptionBuffer& buffer = option.second->toBinary();
-        options.insert(make_pair(option.second->getType(),
+        container.insert(make_pair(option.second->getType(),
                                  OptionPtr(new Option(Option::V4,
                                                       option.second->getType(),
                                                       buffer))));
     }
-    ASSERT_EQ(options.size(), 3);
+    ASSERT_EQ(container.size(), 3);
+    options = container;
+    ASSERT_NO_THROW(LibDHCP::fuseOptions4(options));
+    ASSERT_EQ(options.size(), 1);
     LibDHCP::extendVendorOptions4(options);
     ASSERT_EQ(options.size(), 2);
     for (auto const& option : options) {