[5073a] Addressed review comments

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'",

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">

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)

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());

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.

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

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);
         }

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.

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_);
     }
 

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" }

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;

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 '"