[5073a] Addressed review comments

diff --git a/doc/examples/kea4/classify.json b/doc/examples/kea4/classify.json
index fbe4753ec0cf3d08309fec0516d32dec7156b327..6964960e22a92da1e19126118af3c03d2e54b257 100644 (file)
--- a/doc/examples/kea4/classify.json
+++ b/doc/examples/kea4/classify.json
@@ -55,7 +55,7 @@
 // In this particular class, we want to set specific values
 // of certain DHCPv4 fields. If the incoming packet matches the
 // test, those fields will be set in outgoing responses.
-// The option 43 is defined to encapsulate suboption inf the aastra space.
+// The option 43 is defined to encapsulate suboption in the aastra space.
   {
       "name": "VoIP",
       "test": "substring(option[60].hex,0,6) == 'Aastra'",

diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml
index 4574e622e98f4ba5141cf4552a11994c3af11b5f..6b1ec5928c015f4bcfbd8fd987cf9d5cbf8b89f8 100644 (file)
--- a/doc/guide/dhcp4-srv.xml
+++ b/doc/guide/dhcp4-srv.xml
@@ -1528,9 +1528,9 @@ It is merely echoed by the server
       They can be defined at the global scope or at client class local
       scope: this allows to use option definitions depending on context
       and to set option data accordingly.
-      As the Vendor Specific Information option (code 43) can carry
-      in a not compatible way a raw binary value or sub-options this
-      mechanism is available for this option too.
+      As the Vendor Specific Information option (code 43) has vendor
+      specific format, i.e. can carry either raw binary value or
+      sub-options, this mechanism is available for this option too.
       </para>
       <para>
       The definition used to decode a VSI option is:
@@ -1637,6 +1637,11 @@ It is merely echoed by the server
     ...
 }</screen>
       </para>
+
+      <para>
+      Another possibility adde din Kea 1.3 is to redefine the option,
+      see <xref linkend="dhcp4-private-opts"/>.
+      </para>
     </section>
 
     <section id="dhcp4-option-spaces">

diff --git a/src/bin/dhcp4/dhcp4.dox b/src/bin/dhcp4/dhcp4.dox
index 190fa56140606afc6bae26009caa4107ecd37d90..e6034b5c54df723af3af426eb03cc37482a88a2c 100644 (file)
--- a/src/bin/dhcp4/dhcp4.dox
+++ b/src/bin/dhcp4/dhcp4.dox
@@ -171,7 +171,7 @@ Private (codes 224-254) and VSI (code 43) options are not decoded
 by @c LibDHCP::unpackOptions4 but by @ref isc::dhcp::Dhcpv4Srv::deferredUnpack
 function after classification. To make this function to perform or not
 deferred processing the simplest is to add or not the option code
-to the @ref isc::dhcp::Pkt4::deferredOptions list.
+to the @ref isc::dhcp::Pkt4::getDeferredOptions list.
 
 @section dhcpv4DDNSIntegration DHCPv4 Server Support for the Dynamic DNS Updates
 The DHCPv4 server supports processing of the DHCPv4 Client FQDN option (RFC4702)

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index c968a7d3bc104a240e69f23297cad5b702f68d06..521e7f267c5efda8224929c2c57fccd7448aa4bf 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -2827,7 +2827,7 @@ void
 Dhcpv4Srv::deferredUnpack(Pkt4Ptr& query)
 {
     // Iterate on the list of deferred option codes
-    BOOST_FOREACH(const uint16_t& code, query->deferredOptions()) {
+    BOOST_FOREACH(const uint16_t& code, query->getDeferredOptions()) {
         OptionDefinitionPtr def;
         // Iterate on client classes
         const ClientClasses& classes = query->getClasses();
@@ -2869,12 +2869,12 @@ Dhcpv4Srv::deferredUnpack(Pkt4Ptr& query)
         // Get the existing option for its content and remove all
         OptionPtr opt = query->getOption(code);
         if (!opt) {
-            /* should not happen but do not crash anyway */
+            // should not happen but do not crash anyway
             continue;
         }
         const OptionBuffer buf = opt->getData();
         while (query->delOption(code)) {
-            /* continue */
+            // continue
         }
         // Unpack the option and add it
         opt = def->optionFactory(Option::V4, code, buf.cbegin(), buf.cend());

diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index 38053c5543661d79ac74e08169157219f7211d4f..3d16aa9e87a10a6e58d65188e9bf403b6605851b 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/src/bin/dhcp4/dhcp4_srv.h
@@ -808,7 +808,10 @@ protected:
     /// @note Options 43 and 224-254 are processed after classification.
     /// If a class configures a definition it is applied, if none
     /// the global (user) definition is applied. For option 43
-    /// a last resort definition (same than for previous Kea) is used.
+    /// a last resort definition (same definition as used in previous Kea
+    /// versions) is applied when none is found.
+    ///
+    /// @param query Pointer to the client message.
     void deferredUnpack(Pkt4Ptr& query);
 
     /// @brief Allocation Engine.

diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index ec1a35585eac958a952af05f6cfaaed802d72585..85897604b15ebcc9468b281655fbf4b66dd4e581 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -2300,7 +2300,9 @@ TEST_F(Dhcpv4SrvTest, option43BadRaw) {
     NakedDhcpv4Srv srv(0);
 
     // The vendor-encapsulated-options has an incompatible data
-    // so won't have the expected content.
+    // so won't have the expected content but processing of truncated
+    // (suboption length > available length) suboptions does not raise
+    // an exception.
     string config = "{ \"interfaces-config\": {"
         "    \"interfaces\": [ \"*\" ] }, "
         "\"rebind-timer\": 2000, "
@@ -2343,7 +2345,7 @@ TEST_F(Dhcpv4SrvTest, option43BadRaw) {
     buf.push_back(0x02);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a PRL option to the query
     OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
@@ -2386,7 +2388,9 @@ TEST_F(Dhcpv4SrvTest, option43FailRaw) {
     NakedDhcpv4Srv srv(0);
 
     // The vendor-encapsulated-options has an incompatible data
-    // so won't have the expected content.
+    // so won't have the expected content. Here the processing
+    // of suboptions tries to unpack the uitn32 foo suboption and
+    // raises an exception.
     string config = "{ \"interfaces-config\": {"
         "    \"interfaces\": [ \"*\" ] }, "
         "\"rebind-timer\": 2000, "
@@ -2436,7 +2440,7 @@ TEST_F(Dhcpv4SrvTest, option43FailRaw) {
     buf.push_back(0x01);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a PRL option to the query
     OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
@@ -2505,7 +2509,7 @@ TEST_F(Dhcpv4SrvTest, option43RawGlobal) {
     buf.push_back(0x03);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a PRL option to the query
     OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
@@ -2600,7 +2604,7 @@ TEST_F(Dhcpv4SrvTest, option43RawClass) {
     buf.push_back(0x03);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a PRL option to the query
     OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
@@ -2705,7 +2709,7 @@ TEST_F(Dhcpv4SrvTest, option43Class) {
     buf.push_back(0x21);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a vendor-class-identifier (code 60)
     OptionStringPtr iopt(new OptionString(Option::V4,
@@ -2839,7 +2843,7 @@ TEST_F(Dhcpv4SrvTest, option43ClassPriority) {
     buf.push_back(0x21);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a vendor-class-identifier (code 60)
     OptionStringPtr iopt(new OptionString(Option::V4,
@@ -2979,7 +2983,7 @@ TEST_F(Dhcpv4SrvTest, option43Classes) {
     buf.push_back(0x21);
     OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf));
     query->addOption(vopt);
-    query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
+    query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS);
 
     // Create and add a vendor-class-identifier (code 60)
     OptionStringPtr iopt(new OptionString(Option::V4,
@@ -3086,7 +3090,7 @@ TEST_F(Dhcpv4SrvTest, privateOption) {
     buf.push_back(0x01);
     OptionPtr opt1(new Option(Option::V4, 234, buf));
     query->addOption(opt1);
-    query->deferredOptions().push_back(234);
+    query->getDeferredOptions().push_back(234);
 
     // Create and add a private option with code 245
     buf.clear();
@@ -3096,7 +3100,7 @@ TEST_F(Dhcpv4SrvTest, privateOption) {
     buf.push_back(0x21);
     OptionPtr opt2(new Option(Option::V4, 245, buf));
     query->addOption(opt2);
-    query->deferredOptions().push_back(245);
+    query->getDeferredOptions().push_back(245);
 
 
     // Create and add a PRL option to the query

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index ae430fa4873f1ddb56aa2857d8df244e406a6fe8..bc9fba18db8ce55c7aa899563cdc0fd3d921a1ef 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -294,7 +294,7 @@ LibDHCP::getLastResortOptionDefs(const std::string& space) {
 }
 
 bool
-LibDHCP::deferOption(const std::string& space, const uint16_t code) {
+LibDHCP::shouldDeferOptionUnpack(const std::string& space, const uint16_t code) {
     return ((space == DHCP4_OPTION_SPACE) &&
             ((code == DHO_VENDOR_ENCAPSULATED_OPTIONS) ||
              ((code >= 224) && (code <= 254))));
@@ -546,7 +546,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
         }
 
         // Check if option unpacking must be deferred
-        if (deferOption(option_space, opt_type)) {
+        if (shouldDeferOptionUnpack(option_space, opt_type)) {
             num_defs = 0;
             deferred.push_back(opt_type);
         }

diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index b3f8d7f452dafce300f41cadc11be26d5a9a5ba7..c505656793ea42541551b1a5cd3895901582c193 100644 (file)
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -144,11 +144,12 @@ public:
     ///
     /// DHCPv4 option 43 and 224-254 unpacking is done after classification.
     ///
-    /// @space Option space name.
+    /// @param space Option space name.
     /// @param code Option code.
     ///
     /// @return True if option processing should be deferred.
-    static bool deferOption(const std::string& space, const uint16_t code);
+    static bool shouldDeferOptionUnpack(const std::string& space,
+                                        const uint16_t code);
 
     /// @brief Factory function to create instance of option.
     ///
@@ -381,10 +382,13 @@ private:
     /// is incorrect. This is a programming error.
     static void initStdOptionDefs6();
 
+    /// Initialize last resort DHCPv4 option definitions.
     static void initLastResortOptionDefs();
 
+    /// Initialize DOCSIS DHCPv4 option definitions.
     static void initVendorOptsDocsis4();
 
+    /// Initialize DOCSIS DHCPv6 option definitions.
     static void initVendorOptsDocsis6();
 
     /// Initialize private DHCPv6 option definitions.

diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h
index 3c2607fa01c581ffeb150c374bced92b9b46e8d7..cd31fadb80c09b10f77906ef346e9bdc86e59113 100644 (file)
--- a/src/lib/dhcp/pkt4.h
+++ b/src/lib/dhcp/pkt4.h
@@ -364,8 +364,14 @@ public:
         return (local_hwaddr_);
     }
 
-    /// @brief Returns a reference to deferred option codes
-    std::list<uint16_t>& deferredOptions() {
+    /// @brief Returns a reference to option codes which unpacking
+    /// will be deferred.
+    ///
+    /// @notes Only options 42 and 224-254 are subject of deferred
+    /// unpacking: when the packet unpacking is performed each time
+    /// such an option is found it is unpacked as an unknown option
+    /// and the code added in this list.
+    std::list<uint16_t>& getDeferredOptions() {
         return (deferred_options_);
     }
 

diff --git a/src/lib/dhcp/std_option_defs.h b/src/lib/dhcp/std_option_defs.h
index 04f8488ec356b3a173b1278248fa3192fb958f67..16248e0882ea5271fe97cd16f326d9cfe99018e3 100644 (file)
--- a/src/lib/dhcp/std_option_defs.h
+++ b/src/lib/dhcp/std_option_defs.h
@@ -216,7 +216,8 @@ const OptionDefParams STANDARD_V4_OPTION_DEFINITIONS[] = {
 const int STANDARD_V4_OPTION_DEFINITIONS_SIZE =
     sizeof(STANDARD_V4_OPTION_DEFINITIONS) / sizeof(STANDARD_V4_OPTION_DEFINITIONS[0]);
 
-/// Last resort definitions (only option 43 for now).
+/// Last resort definitions (only option 43 for now, these definitions
+/// are applied in deferred unpacking when none is found).
 const OptionDefParams LAST_RESORT_V4_OPTION_DEFINITIONS[] = {
     { "vendor-encapsulated-options", DHO_VENDOR_ENCAPSULATED_OPTIONS,
       OPT_EMPTY_TYPE, false, NO_RECORD_DEF, "vendor-encapsulated-options-space" }

diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index 534a1fd967cda50c97f313a10c55f914220d295f..7a09681d694ba6110d5ef363c976ed601bbdaff5 100644 (file)
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -1815,10 +1815,10 @@ TEST_F(LibDhcpTest, setRuntimeOptionDefs) {
 
 // This test verifies the processing of option 43
 TEST_F(LibDhcpTest, option43) {
-    // Check deferOption()
-    EXPECT_TRUE(LibDHCP::deferOption(DHCP4_OPTION_SPACE, 43));
-    EXPECT_FALSE(LibDHCP::deferOption(DHCP4_OPTION_SPACE, 44));
-    EXPECT_FALSE(LibDHCP::deferOption(DHCP6_OPTION_SPACE, 43));
+    // Check shouldDeferOptionUnpack()
+    EXPECT_TRUE(LibDHCP::shouldDeferOptionUnpack(DHCP4_OPTION_SPACE, 43));
+    EXPECT_FALSE(LibDHCP::shouldDeferOptionUnpack(DHCP4_OPTION_SPACE, 44));
+    EXPECT_FALSE(LibDHCP::shouldDeferOptionUnpack(DHCP6_OPTION_SPACE, 43));
 
     // Check last resort
     OptionDefinitionPtr def;

diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
index a630b47c90a0de6a6b9340da82497f548251a9cd..9d6eba7613906deb64de6b9a504cb2b4be3b0cf0 100644 (file)
--- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
+++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
@@ -101,7 +101,8 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary,
             def = parser.parse(option_def);
             // Verify if the defition is for an option which are
             // in a deferred processing list.
-            if (!LibDHCP::deferOption(def.second, def.first->getCode())) {
+            if (!LibDHCP::shouldDeferOptionUnpack(def.second,
+                                                  def.first->getCode())) {
                 isc_throw(DhcpConfigError,
                           "Not allowed option definition for code '"
                           << def.first->getCode() << "' in space '"