[3194] Changes after review:

 - config parsers updated
 - new unit-tests for vendor options written
 - libdhcp++ cleaned up
 - fixes in option_vendor.cc
 - comments added and cleaned up

src/bin/dhcp4/config_parser.cc

@@ -94,13 +94,13 @@ protected:
         } else if (option_space == "dhcp6") {
             isc_throw(DhcpConfigError, "'dhcp6' option space name is reserved"
                      << " for DHCPv6 server");
-        }
-
-        // Check if this is a vendor-option. If it is, get vendor-specific
-        // definition.
-        uint32_t vendor_id = SubnetConfigParser::optionSpaceToVendorId(option_space);
-        if (vendor_id) {
-            def = LibDHCP::getVendorOptionDef(Option::V4, vendor_id, option_code);
+        } else {
+            // Check if this is a vendor-option. If it is, get vendor-specific
+            // definition.
+            uint32_t vendor_id = SubnetConfigParser::optionSpaceToVendorId(option_space);
+            if (vendor_id) {
+                def = LibDHCP::getVendorOptionDef(Option::V4, vendor_id, option_code);
+            }
         }
 
         return (def);

src/bin/dhcp4/dhcp4_srv.cc

@@ -671,10 +671,8 @@ Dhcpv4Srv::appendRequestedVendorOptions(const Pkt4Ptr& question, Pkt4Ptr& answer
     // Let's try to get ORO within that vendor-option
     /// @todo This is very specific to vendor-id=4491 (Cable Labs). Other vendors
     /// may have different policies.
-    OptionPtr oro = vendor_req->getOption(DOCSIS3_V4_ORO);
-
-    /// @todo: see OPT_UINT8_TYPE definition in OptionDefinition::optionFactory().
-    /// I think it should be OptionUint8Array, not OptionGeneric
+    OptionUint8ArrayPtr oro =
+        boost::dynamic_pointer_cast<OptionUint8Array>(vendor_req->getOption(DOCSIS3_V4_ORO));
 
     // Option ORO not found. Don't do anything then.
     if (!oro) {
@@ -685,9 +683,9 @@ Dhcpv4Srv::appendRequestedVendorOptions(const Pkt4Ptr& question, Pkt4Ptr& answer
 
     // Get the list of options that client requested.
     bool added = false;
-    const OptionBuffer& requested_opts = oro->getData();
+    const std::vector<uint8_t>& requested_opts = oro->getData();
 
-    for (OptionBuffer::const_iterator code = requested_opts.begin();
+    for (std::vector<uint8_t>::const_iterator code = requested_opts.begin();
          code != requested_opts.end(); ++code) {
         Subnet::OptionDescriptor desc = subnet->getVendorOptionDescriptor(vendor_id, *code);
         if (desc.option) {

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -23,6 +23,7 @@
 #include <dhcp/option4_addrlst.h>
 #include <dhcp/option_custom.h>
 #include <dhcp/option_int.h>
+#include <dhcp/docsis3_option_defs.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <hooks/hooks_manager.h>
@@ -1794,6 +1795,120 @@ TEST_F(Dhcp4ParserTest, stdOptionDataEncapsulate) {
     EXPECT_FALSE(desc.option->getOption(3));
 }
 
+// This test checks if vendor options can be specified in the config file
+// (in hex format), and later retrieved from configured subnet
+TEST_F(Dhcp4ParserTest, vendorOptionsHex) {
+
+    // This configuration string is to configure two options
+    // sharing the code 1 and belonging to the different vendor spaces.
+    // (different vendor-id values).
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000,"
+        "\"renew-timer\": 1000,"
+        "\"option-data\": [ {"
+        "    \"name\": \"option-one\","
+        "    \"space\": \"vendor-4491\"," // VENDOR_ID_CABLE_LABS = 4491
+        "    \"code\": 100," // just a random code
+        "    \"data\": \"AB CDEF0105\","
+        "    \"csv-format\": False"
+        " },"
+        " {"
+        "    \"name\": \"option-two\","
+        "    \"space\": \"vendor-1234\","
+        "    \"code\": 100,"
+        "    \"data\": \"1234\","
+        "    \"csv-format\": False"
+        " } ],"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1-192.0.2.10\" ],"
+        "    \"subnet\": \"192.0.2.0/24\""
+        " } ]"
+        "}";
+
+    ConstElementPtr status;
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+    ASSERT_TRUE(status);
+    checkResult(status, 0);
+
+    // Options should be now available for the subnet.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.5"));
+    ASSERT_TRUE(subnet);
+
+    // Try to get the option from the vendor space 4491
+    Subnet::OptionDescriptor desc1 = subnet->getVendorOptionDescriptor(VENDOR_ID_CABLE_LABS, 100);
+    ASSERT_TRUE(desc1.option);
+    EXPECT_EQ(100, desc1.option->getType());
+    // Try to get the option from the vendor space 1234
+    Subnet::OptionDescriptor desc2 = subnet->getVendorOptionDescriptor(1234, 100);
+    ASSERT_TRUE(desc2.option);
+    EXPECT_EQ(100, desc1.option->getType());
+
+    // Try to get the non-existing option from the non-existing
+    // option space and  expect that option is not returned.
+    Subnet::OptionDescriptor desc3 = subnet->getVendorOptionDescriptor(5678, 100);
+    ASSERT_FALSE(desc3.option);
+}
+
+// This test checks if vendor options can be specified in the config file,
+// (in csv format), and later retrieved from configured subnet
+TEST_F(Dhcp4ParserTest, vendorOptionsCsv) {
+
+    // This configuration string is to configure two options
+    // sharing the code 1 and belonging to the different vendor spaces.
+    // (different vendor-id values).
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000,"
+        "\"renew-timer\": 1000,"
+        "\"option-data\": [ {"
+        "    \"name\": \"foo\","
+        "    \"space\": \"vendor-4491\","
+        "    \"code\": 100,"
+        "    \"data\": \"this is a string vendor-opt\","
+        "    \"csv-format\": True"
+        " } ],"
+        "\"option-def\": [ {"
+        "    \"name\": \"foo\","
+        "    \"code\": 100,"
+        "    \"type\": \"string\","
+        "    \"array\": False,"
+        "    \"record-types\": \"\","
+        "    \"space\": \"vendor-4491\","
+        "    \"encapsulate\": \"\""
+        " } ],"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" "
+        " } ]"
+        "}";
+
+    ConstElementPtr status;
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+    ASSERT_TRUE(status);
+    checkResult(status, 0);
+
+    // Options should be now available for the subnet.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.5"));
+    ASSERT_TRUE(subnet);
+
+    // Try to get the option from the vendor space 4491
+    Subnet::OptionDescriptor desc1 = subnet->getVendorOptionDescriptor(VENDOR_ID_CABLE_LABS, 100);
+    ASSERT_TRUE(desc1.option);
+    EXPECT_EQ(100, desc1.option->getType());
+
+    // Try to get the non-existing option from the non-existing
+    // option space and  expect that option is not returned.
+    Subnet::OptionDescriptor desc2 = subnet->getVendorOptionDescriptor(5678, 100);
+    ASSERT_FALSE(desc2.option);
+}
+
+
+
 // Tests of the hooks libraries configuration.  All tests have the pre-
 // condition (checked in the test fixture's SetUp() method) that no hooks
 // libraries are loaded at the start of the tests.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -40,6 +40,7 @@
 #include <gtest/gtest.h>
 #include <hooks/server_hooks.h>
 #include <hooks/hooks_manager.h>
+#include <config/ccsession.h>
 
 #include <boost/scoped_ptr.hpp>
 
@@ -1178,6 +1179,10 @@ TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho) {
     EXPECT_TRUE(rai_response->equal(rai_query));
 }
 
+/// @todo move vendor options tests to a separate file.
+/// @todo Add more extensive vendor options tests, including multiple
+///       vendor options
+
 // Checks if vendor options are parsed correctly and requested vendor options
 // are echoed back.
 TEST_F(Dhcpv4SrvTest, vendorOptionsDocsis) {
@@ -1197,10 +1202,10 @@ TEST_F(Dhcpv4SrvTest, vendorOptionsDocsis) {
         "\"subnet4\": [ { "
         "    \"pool\": [ \"10.254.226.0/25\" ],"
         "    \"subnet\": \"10.254.226.0/24\", "
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"valid-lifetime\": 4000,"
         "    \"interface\": \"" + valid_iface_ + "\" "
-        " }, {"
-        "    \"pool\": [ \"192.0.3.0/25\" ],"
-        "    \"subnet\": \"192.0.3.0/24\" "
         " } ],"
         "\"valid-lifetime\": 4000 }";
 
@@ -2712,6 +2717,187 @@ TEST_F(HooksDhcpv4SrvTest, lease4ReleaseSkip) {
     //EXPECT_EQ(leases.size(), 1);
 }
 
+// Checks if server is able to handle a relayed traffic from DOCSIS3.0 modems
+TEST_F(Dhcpv4SrvTest, docsisVendorOptionsParse) {
+
+    // Let's get a traffic capture from DOCSIS3.0 modem
+    Pkt4Ptr dis = captureRelayedDiscover();
+    ASSERT_NO_THROW(dis->unpack());
+
+    // Check if the packet contain
+    OptionPtr opt = dis->getOption(DHO_VIVSO_SUBOPTIONS);
+    ASSERT_TRUE(opt);
+
+    boost::shared_ptr<OptionVendor> vendor = boost::dynamic_pointer_cast<OptionVendor>(opt);
+    ASSERT_TRUE(vendor);
+
+    // This particular capture that we have included options 1 and 5
+    EXPECT_TRUE(vendor->getOption(1));
+    EXPECT_TRUE(vendor->getOption(5));
+
+    // It did not include options any other options
+    EXPECT_FALSE(vendor->getOption(2));
+    EXPECT_FALSE(vendor->getOption(3));
+    EXPECT_FALSE(vendor->getOption(17));
+}
+
+// Checks if server is able to parse incoming docsis option and extract suboption 1 (docsis ORO)
+TEST_F(Dhcpv4SrvTest, docsisVendorORO) {
+
+    // Let's get a traffic capture from DOCSIS3.0 modem
+    Pkt4Ptr dis = captureRelayedDiscover();
+    EXPECT_NO_THROW(dis->unpack());
+
+    // Check if the packet contains vendor specific information option
+    OptionPtr opt = dis->getOption(DHO_VIVSO_SUBOPTIONS);
+    ASSERT_TRUE(opt);
+
+    boost::shared_ptr<OptionVendor> vendor = boost::dynamic_pointer_cast<OptionVendor>(opt);
+    ASSERT_TRUE(vendor);
+
+    opt = vendor->getOption(DOCSIS3_V4_ORO);
+    ASSERT_TRUE(opt);
+
+    OptionUint8ArrayPtr oro = boost::dynamic_pointer_cast<OptionUint8Array>(opt);
+    EXPECT_TRUE(oro);
+}
+
+// This test checks if Option Request Option (ORO) in docsis (vendor-id=4491)
+// vendor options is parsed correctly and the requested options are actually assigned.
+TEST_F(Dhcpv4SrvTest, vendorOptionsORO) {
+
+    NakedDhcpv4Srv srv(0);
+
+    ConstElementPtr x;
+    string config = "{ \"interfaces\": [ \"all\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "    \"option-data\": [ {"
+        "          \"name\": \"tftp-servers\","
+        "          \"space\": \"vendor-4491\","
+        "          \"code\": 2,"
+        "          \"data\": \"192.0.2.1, 192.0.2.2\","
+        "          \"csv-format\": True"
+        "        }],"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.0/25\" ],"
+        "    \"subnet\": \"192.0.2.0/24\", "
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"valid-lifetime\": 4000,"
+        "    \"interface\": \"" + valid_iface_ + "\" "
+        " } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(x = configureDhcp4Server(srv, json));
+    ASSERT_TRUE(x);
+    comment_ = isc::config::parseAnswer(rcode_, x);
+    ASSERT_EQ(0, rcode_);
+
+    boost::shared_ptr<Pkt4> dis(new Pkt4(DHCPDISCOVER, 1234));
+    // Set the giaddr to non-zero address as if it was relayed.
+    dis->setGiaddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get an advertise
+    Pkt4Ptr offer = srv.processDiscover(dis);
+
+    // check if we get response at all
+    ASSERT_TRUE(offer);
+
+    // We did not include any vendor opts in DISCOVER, so there should be none
+    // in OFFER.
+    ASSERT_FALSE(offer->getOption(DHO_VIVSO_SUBOPTIONS));
+
+    // 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));
+    vendor_oro->addValue(DOCSIS3_V4_TFTP_SERVERS); // Request option 33
+    OptionPtr vendor(new OptionVendor(Option::V4, 4491));
+    vendor->addOption(vendor_oro);
+    dis->addOption(vendor);
+
+    // Need to process SOLICIT again after requesting new option.
+    offer = srv.processDiscover(dis);
+    ASSERT_TRUE(offer);
+
+    // Check if thre is vendor option response
+    OptionPtr tmp = offer->getOption(DHO_VIVSO_SUBOPTIONS);
+    ASSERT_TRUE(tmp);
+
+    // The response should be OptionVendor object
+    boost::shared_ptr<OptionVendor> vendor_resp =
+        boost::dynamic_pointer_cast<OptionVendor>(tmp);
+    ASSERT_TRUE(vendor_resp);
+
+    OptionPtr docsis2 = vendor_resp->getOption(DOCSIS3_V4_TFTP_SERVERS);
+    ASSERT_TRUE(docsis2);
+
+    Option4AddrLstPtr tftp_srvs = boost::dynamic_pointer_cast<Option4AddrLst>(docsis2);
+    ASSERT_TRUE(tftp_srvs);
+
+    Option4AddrLst::AddressContainer addrs = tftp_srvs->getAddresses();
+    ASSERT_EQ(2, addrs.size());
+    EXPECT_EQ("192.0.2.1", addrs[0].toText());
+    EXPECT_EQ("192.0.2.2", addrs[1].toText());
+}
+
+// Test checks whether it is possible to use option definitions defined in
+// src/lib/dhcp/docsis3_option_defs.h.
+TEST_F(Dhcpv4SrvTest, vendorOptionsDocsisDefinitions) {
+    ConstElementPtr x;
+    string config_prefix = "{ \"interfaces\": [ \"all\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "    \"option-data\": [ {"
+        "          \"name\": \"tftp-servers\","
+        "          \"space\": \"vendor-4491\","
+        "          \"code\": ";
+    string config_postfix = ","
+        "          \"data\": \"192.0.2.1\","
+        "          \"csv-format\": True"
+        "        }],"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.50\" ],"
+        "    \"subnet\": \"192.0.2.0/24\", "
+        "    \"renew-timer\": 1000, "
+        "    \"rebind-timer\": 1000, "
+        "    \"valid-lifetime\": 4000,"
+        "    \"interface\": \"\""
+        " } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    // There is docsis3 (vendor-id=4491) vendor option 2, which is a
+    // tftp-server. Its format is list of IPv4 addresses.
+    string config_valid = config_prefix + "2" + config_postfix;
+
+    // There is no option 99 defined in vendor-id=4491. As there is no
+    // definition, the config should fail.
+    string config_bogus = config_prefix + "99" + config_postfix;
+
+    ElementPtr json_bogus = Element::fromJSON(config_bogus);
+    ElementPtr json_valid = Element::fromJSON(config_valid);
+
+    NakedDhcpv4Srv srv(0);
+
+    // This should fail (missing option definition)
+    EXPECT_NO_THROW(x = configureDhcp4Server(srv, json_bogus));
+    ASSERT_TRUE(x);
+    comment_ = isc::config::parseAnswer(rcode_, x);
+    ASSERT_EQ(1, rcode_);
+
+    // This should work (option definition present)
+    EXPECT_NO_THROW(x = configureDhcp4Server(srv, json_valid));
+    ASSERT_TRUE(x);
+    comment_ = isc::config::parseAnswer(rcode_, x);
+    ASSERT_EQ(0, rcode_);
+}
+
+
 }; // end of isc::dhcp::test namespace
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

src/bin/dhcp6/config_parser.cc

@@ -109,16 +109,16 @@ protected:
         } else if (option_space == "dhcp4") {
             isc_throw(DhcpConfigError, "'dhcp4' option space name is reserved"
                      << " for DHCPv4 server");
+        } else {
+            // Check if this is a vendor-option. If it is, get vendor-specific
+            // definition.
+            uint32_t vendor_id = SubnetConfigParser::optionSpaceToVendorId(option_space);
+            if (vendor_id) {
+                def = LibDHCP::getVendorOptionDef(Option::V6, vendor_id, option_code);
+            }
         }
 
-        // Check if this is a vendor-option. If it is, get vendor-specific
-        // definition.
-        uint32_t vendor_id = SubnetConfigParser::optionSpaceToVendorId(option_space);
-        if (vendor_id) {
-            def = LibDHCP::getVendorOptionDef(Option::V6, vendor_id, option_code);
-        }
-
-        return def;
+        return (def);
     }
 };
 

src/bin/dhcp6/dhcp6_srv.cc

@@ -705,8 +705,6 @@ Dhcpv6Srv::appendRequestedVendorOptions(const Pkt6Ptr& question, Pkt6Ptr& answer
         return;
     }
 
-    uint32_t vendor_id = vendor_req->getVendorId();
-
     // Let's try to get ORO within that vendor-option
     /// @todo This is very specific to vendor-id=4491 (Cable Labs). Other vendors
     /// may have different policies.
@@ -718,6 +716,8 @@ Dhcpv6Srv::appendRequestedVendorOptions(const Pkt6Ptr& question, Pkt6Ptr& answer
         return;
     }
 
+    uint32_t vendor_id = vendor_req->getVendorId();
+
     boost::shared_ptr<OptionVendor> vendor_rsp(new OptionVendor(Option::V6, vendor_id));
 
     // Get the list of options that client requested.

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -2157,10 +2157,13 @@ TEST_F(Dhcp6ParserTest, vendorOptionsCsv) {
 
     // Try to get the non-existing option from the non-existing
     // option space and  expect that option is not returned.
-    Subnet::OptionDescriptor desc2 = subnet->getVendorOptionDescriptor(5678, 38);
+    Subnet::OptionDescriptor desc2 = subnet->getVendorOptionDescriptor(5678, 100);
     ASSERT_FALSE(desc2.option);
 }
 
+/// @todo add tests similar to vendorOptionsCsv and vendorOptionsHex, but for
+///       vendor options defined in a subnet.
+
 // The goal of this test is to verify that the standard option can
 // be configured to encapsulate multiple other options.
 TEST_F(Dhcp6ParserTest, stdOptionDataEncapsulate) {

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -2022,8 +2022,6 @@ TEST_F(Dhcpv6SrvTest, docsisTraffic) {
 // Checks if server is able to handle a relayed traffic from DOCSIS3.0 modems
 TEST_F(Dhcpv6SrvTest, docsisVendorOptionsParse) {
 
-    NakedDhcpv6Srv srv(0);
-
     // Let's get a traffic capture from DOCSIS3.0 modem
     Pkt6Ptr sol = captureDocsisRelayedSolicit();
     EXPECT_NO_THROW(sol->unpack());
@@ -2035,10 +2033,10 @@ TEST_F(Dhcpv6SrvTest, docsisVendorOptionsParse) {
     boost::shared_ptr<OptionVendor> vendor = boost::dynamic_pointer_cast<OptionVendor>(opt);
     ASSERT_TRUE(vendor);
 
-    EXPECT_TRUE(vendor->getOption(1));
+    EXPECT_TRUE(vendor->getOption(DOCSIS3_V6_ORO));
     EXPECT_TRUE(vendor->getOption(36));
     EXPECT_TRUE(vendor->getOption(35));
-    EXPECT_TRUE(vendor->getOption(2));
+    EXPECT_TRUE(vendor->getOption(DOCSIS3_V6_DEVICE_TYPE));
     EXPECT_TRUE(vendor->getOption(3));
     EXPECT_TRUE(vendor->getOption(4));
     EXPECT_TRUE(vendor->getOption(5));
@@ -2046,7 +2044,7 @@ TEST_F(Dhcpv6SrvTest, docsisVendorOptionsParse) {
     EXPECT_TRUE(vendor->getOption(7));
     EXPECT_TRUE(vendor->getOption(8));
     EXPECT_TRUE(vendor->getOption(9));
-    EXPECT_TRUE(vendor->getOption(10));
+    EXPECT_TRUE(vendor->getOption(DOCSIS3_V6_VENDOR_NAME));
     EXPECT_TRUE(vendor->getOption(15));
 
     EXPECT_FALSE(vendor->getOption(20));
@@ -2061,9 +2059,9 @@ TEST_F(Dhcpv6SrvTest, docsisVendorORO) {
 
     // Let's get a traffic capture from DOCSIS3.0 modem
     Pkt6Ptr sol = captureDocsisRelayedSolicit();
-    EXPECT_NO_THROW(sol->unpack());
+    ASSERT_NO_THROW(sol->unpack());
 
-    // Check if the packet contain
+    // Check if the packet contains vendor options option
     OptionPtr opt = sol->getOption(D6O_VENDOR_OPTS);
     ASSERT_TRUE(opt);
 

src/lib/dhcp/docsis3_option_defs.h

@@ -35,6 +35,7 @@ const OptionDefParams DOCSIS3_V4_DEFS[] = {
 /// Number of option definitions defined.
 const int DOCSIS3_V4_DEFS_SIZE  = sizeof(DOCSIS3_V4_DEFS) / sizeof(OptionDefParams);
 
+/// @todo define remaining docsis3 v6 codes
 #define DOCSIS3_V6_ORO 1
 #define DOCSIS3_V6_DEVICE_TYPE 2
 #define DOCSIS3_V6_VENDOR_NAME 10
@@ -62,4 +63,4 @@ const int DOCSIS3_V6_DEFS_SIZE  = sizeof(DOCSIS3_V6_DEFS) / sizeof(OptionDefPara
 
 }; // anonymous namespace
 
-#endif // STD_OPTION_DEFS_H
+#endif // DOCSIS3_OPTION_DEFS_H

src/lib/dhcp/libdhcp++.cc

@@ -604,57 +604,7 @@ void LibDHCP::OptionFactoryRegister(Option::Universe u,
 
 void
 LibDHCP::initStdOptionDefs4() {
-    v4option_defs_.clear();
-
-    // Now let's add all option definitions.
-    for (int i = 0; i < OPTION_DEF_PARAMS_SIZE4; ++i) {
-        std::string encapsulates(OPTION_DEF_PARAMS4[i].encapsulates);
-        if (!encapsulates.empty() && OPTION_DEF_PARAMS4[i].array) {
-            isc_throw(isc::BadValue, "invalid standard option definition: "
-                      << "option with code '" << OPTION_DEF_PARAMS4[i].code
-                      << "' may not encapsulate option space '"
-                      << encapsulates << "' because the definition"
-                      << " indicates that this option comprises an array"
-                      << " of values");
-        }
-
-        // Depending whether the option encapsulates an option space or not
-        // we pick different constructor to create an instance of the option
-        // definition.
-        OptionDefinitionPtr definition;
-        if (encapsulates.empty()) {
-            // Option does not encapsulate any option space.
-            definition.reset(new OptionDefinition(OPTION_DEF_PARAMS4[i].name,
-                                                  OPTION_DEF_PARAMS4[i].code,
-                                                  OPTION_DEF_PARAMS4[i].type,
-                                                  OPTION_DEF_PARAMS4[i].array));
-
-        } else {
-            // Option does encapsulate an option space.
-            definition.reset(new OptionDefinition(OPTION_DEF_PARAMS4[i].name,
-                                                  OPTION_DEF_PARAMS4[i].code,
-                                                  OPTION_DEF_PARAMS4[i].type,
-                                                  OPTION_DEF_PARAMS4[i].encapsulates));
-
-        }
-
-        for (int rec = 0; rec < OPTION_DEF_PARAMS4[i].records_size; ++rec) {
-            definition->addRecordField(OPTION_DEF_PARAMS4[i].records[rec]);
-        }
-
-        // Sanity check if the option is valid.
-        try {
-            definition->validate();
-        } catch (const Exception& ex) {
-            // This is unlikely event that validation fails and may
-            // be only caused by programming error. To guarantee the
-            // data consistency we clear all option definitions that
-            // have been added so far and pass the exception forward.
-            v4option_defs_.clear();
-            throw;
-        }
-        v4option_defs_.push_back(definition);
-    }
+    initOptionSpace(v4option_defs_, OPTION_DEF_PARAMS4, OPTION_DEF_PARAMS_SIZE4);
 }
 
 void

src/lib/dhcp/libdhcp++.h

@@ -55,7 +55,13 @@ public:
     static OptionDefinitionPtr getOptionDef(const Option::Universe u,
                                             const uint16_t code);
 
-
+    /// @brief Returns vendor option definition for a given vendor-id and code
+    ///
+    /// @param u universe (V4 or V6)
+    /// @param vendor_id enterprise-id for a given vendor
+    /// @param code option code
+    /// @return reference to an option definition being requested
+    /// or NULL pointer if option definition has not been found.
     static OptionDefinitionPtr getVendorOptionDef(const Option::Universe u,
                                                   const uint32_t vendor_id,
                                                   const uint16_t code);
@@ -154,16 +160,46 @@ public:
                                       uint16_t type,
                                       Option::Factory * factory);
 
+    /// @brief returns v4 option definitions for a given vendor
+    ///
+    /// @param vendor_id enterprise-id of a given vendor
+    /// @return a container for a given vendor (or NULL if not option
+    ///         definitions are defined)
     static const OptionDefContainer*
     getVendorOption4Defs(uint32_t vendor_id);
 
+    /// @brief returns v6 option definitions for a given vendor
+    ///
+    /// @param vendor_id enterprise-id of a given vendor
+    /// @return a container for a given vendor (or NULL if not option
+    ///         definitions are defined)
     static const OptionDefContainer*
     getVendorOption6Defs(uint32_t vendor_id);
 
+    /// @brief Parses provided buffer as DHCPv6 vendor options and creates
+    ///        Option objects.
+    ///
+    /// Parses provided buffer and stores created Option objects
+    /// in options container.
+    ///
+    /// @param vendor_id enterprise-id of the vendor
+    /// @param buf Buffer to be parsed.
+    /// @param options Reference to option container. Options will be
+    ///        put here.
     static size_t unpackVendorOptions6(uint32_t vendor_id,
                                        const OptionBuffer& buf,
                                        isc::dhcp::OptionCollection& options);
 
+    /// @brief Parses provided buffer as DHCPv4 vendor options and creates
+    ///        Option objects.
+    ///
+    /// Parses provided buffer and stores created Option objects
+    /// in options container.
+    ///
+    /// @param vendor_id enterprise-id of the vendor
+    /// @param buf Buffer to be parsed.
+    /// @param options Reference to option container. Options will be
+    ///        put here.
     static size_t unpackVendorOptions4(uint32_t vendor_id, const OptionBuffer& buf,
                                        isc::dhcp::OptionCollection& options);
 
@@ -204,8 +240,10 @@ private:
     /// Container with DHCPv6 option definitions.
     static OptionDefContainer v6option_defs_;
 
+    /// Container for v4 vendor option definitions
     static VendorOptionDefContainers vendor4_defs_;
 
+    /// Container for v6 vendor option definitions
     static VendorOptionDefContainers vendor6_defs_;
 };
 

src/lib/dhcp/option_vendor.cc

@@ -32,20 +32,28 @@ OptionVendor::OptionVendor(Option::Universe u, OptionBufferConstIter begin,
 void OptionVendor::pack(isc::util::OutputBuffer& buf) {
     packHeader(buf);
 
+    // Store vendor-id
     buf.writeUint32(vendor_id_);
 
+    // The format is slightly different for v4
+    if (universe_ == Option::V4) {
+        // Store data-len1 (it's a length of following suboptions
+        buf.writeUint8(len() - getHeaderLen() - sizeof(uint32_t) - sizeof(uint8_t));
+    }
+
     packOptions(buf);
 }
 
 void OptionVendor::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) {
     if (distance(begin, end) < sizeof(uint32_t)) {
-        isc_throw(OutOfRange, "Truncated vendor-specific information option");
+        isc_throw(OutOfRange, "Truncated vendor-specific information option"
+                  << ", length=" << distance(begin, end));
     }
 
     vendor_id_ = isc::util::readUint32(&(*begin));
 
     OptionBuffer vendor_buffer(begin +4, end);
-    
+
     if (universe_ == Option::V6) {
         LibDHCP::unpackVendorOptions6(vendor_id_, vendor_buffer, options_);
     } else {
@@ -58,6 +66,11 @@ uint16_t OptionVendor::len() {
 
     length += sizeof(uint32_t); // Vendor-id field
 
+    // Data-len field exists in DHCPv4 vendor options only
+    if (universe_ == Option::V4) {
+        length += sizeof(uint8_t);  // data-len
+    }
+
     // length of all suboptions
     for (OptionCollection::iterator it = options_.begin();
          it != options_.end();

src/lib/dhcp/option_vendor.h

@@ -26,6 +26,7 @@ namespace isc {
 namespace dhcp {
 
 /// This class represents vendor-specific information option.
+/// As defined in RFC3925. The option formatting is slightly
 class OptionVendor: public Option {
 
 public:
@@ -56,10 +57,6 @@ public:
     /// byte after stored option.
     ///
     /// @param [out] buf buffer (option will be stored here)
-    ///
-    /// @throw isc::dhcp::InvalidDataType if size of a data field type is not
-    /// equal to 1, 2 or 4 bytes. The data type is not checked in this function
-    /// because it is checked in a constructor.
     void pack(isc::util::OutputBuffer& buf);
 
     /// @brief Parses received buffer
@@ -71,19 +68,16 @@ public:
     /// @param end iterator to end of option data (first byte after option end)
     ///
     /// @throw isc::OutOfRange if provided buffer is shorter than data size.
-    /// @throw isc::dhcp::InvalidDataType if size of a data field type is not
-    /// equal to 1, 2 or 4 bytes. The data type is not checked in this function
-    /// because it is checked in a constructor.
     virtual void unpack(OptionBufferConstIter begin, OptionBufferConstIter end);
 
-    /// @brief Set option value.
+    /// @brief Sets enterprise identifier
     ///
-    /// @param value new option value.
+    /// @param value vendor identifier
     void setVendorId(uint32_t vendor_id) { vendor_id_ = vendor_id; }
 
-    /// @brief Return option value.
+    /// @brief Returns enterprise identifier
     ///
-    /// @return option value.
+    /// @return enterprise identifier
     uint32_t getVendorId() const { return vendor_id_; }
 
     /// @brief returns complete length of option
@@ -101,4 +95,4 @@ private:
 } // isc::dhcp namespace
 } // isc namespace
 
-#endif // OPTION_INT_H
+#endif // OPTION_VENDOR_H

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -1052,7 +1052,7 @@ SubnetConfigParser::createSubnet() {
             // Add sub-options (if any).
             appendSubOptions(option_space, desc.option);
 
-            // thomson
+            // Check if the option space defines a vendor-option
             uint32_t vendor_id = optionSpaceToVendorId(option_space);
             if (vendor_id) {
                 // This is a vendor option

src/lib/dhcpsrv/dhcp_parsers.h

@@ -775,7 +775,8 @@ public:
 
     /// @brief tries to convert option_space string to numeric vendor_id
     ///
-    /// This will work if the option_space has format "vendor-1234".
+    /// This will work if the option_space has format "vendor-X", where
+    /// X can be any value between 1 and MAX_UINT32.
     /// This is used to detect whether a given option-space is a vendor
     /// space or not. Returns 0 if the format is different.
     /// @return numeric vendor-id (or 0 if the format does not match)

src/lib/dhcpsrv/option_space_container.h

@@ -42,7 +42,7 @@ public:
     /// @brief Adds a new item to the option_space.
     ///
     /// @param item reference to the item being added.
-    /// @param option_space name of the option space.
+    /// @param option_space name or vendor-id of the option space
     void addItem(const ItemType& item, const Selector& option_space) {
         ItemsContainerPtr items = getItems(option_space);
         items->push_back(item);
@@ -55,7 +55,7 @@ public:
     /// space an empty container is created and returned. However
     /// this container is not added to the list of option spaces.
     ///
-    /// @param option_space name of the option space.
+    /// @param option_space name or vendor-id of the option space.
     ///
     /// @return pointer to the container holding items.
     ItemsContainerPtr getItems(const Selector& option_space) const {
@@ -91,7 +91,7 @@ public:
 
 private:
 
-    /// A map holding container (option space name is the key).
+    /// A map holding container (option space name or vendor-id is the key).
     typedef std::map<Selector, ItemsContainerPtr> OptionSpaceMap;
     OptionSpaceMap option_space_map_;
 };

src/lib/dhcpsrv/subnet.h

@@ -180,12 +180,20 @@ public:
     void addOption(const OptionPtr& option, bool persistent,
                    const std::string& option_space);
 
+
+    /// @brief Adds new vendor option instance to the collection.
+    ///
+    /// @param option option instance.
+    /// @param persistent if true, send an option regardless if client
+    /// requested it or not.
+    /// @param vendor_id enterprise id of the vendor space to add an option to.
     void addVendorOption(const OptionPtr& option, bool persistent,
                          uint32_t vendor_id);
 
     /// @brief Delete all options configured for the subnet.
     void delOptions();
 
+    /// @brief Deletes all vendor options configured for the subnet.
     void delVendorOptions();
 
     /// @brief checks if the specified address is in pools
@@ -226,6 +234,11 @@ public:
     OptionContainerPtr
     getOptionDescriptors(const std::string& option_space) const;
 
+    /// @brief Return a collection of vendor option descriptors.
+    ///
+    /// @param vendor_id enterprise id of the option space.
+    ///
+    /// @return pointer to collection of options configured for a subnet.
     OptionContainerPtr
     getVendorOptionDescriptors(uint32_t vendor_id) const;
 
@@ -240,6 +253,13 @@ public:
     getOptionDescriptor(const std::string& option_space,
                         const uint16_t option_code);
 
+    /// @brief Return single vendor option descriptor.
+    ///
+    /// @param vendor_id enterprise id of the option space.
+    /// @param option_code code of the option to be returned.
+    ///
+    /// @return option descriptor found for the specified option space
+    /// and option code.
     OptionDescriptor
     getVendorOptionDescriptor(uint32_t vendor_id, uint16_t option_code);
 
@@ -453,11 +473,14 @@ private:
     typedef OptionSpaceContainer<OptionContainer,
         OptionDescriptor, std::string> OptionSpaceCollection;
 
+    /// A collection of vendor space option descriptors.
     typedef OptionSpaceContainer<OptionContainer,
         OptionDescriptor, uint32_t> VendorOptionSpaceCollection;
 
+    /// Regular options are kept here
     OptionSpaceCollection option_spaces_;
 
+    /// Vendor options are kept here
     VendorOptionSpaceCollection vendor_option_spaces_;
 };
 

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -250,6 +250,16 @@ TEST_F(DhcpParserTest, interfaceListParserTest) {
     EXPECT_TRUE(cfg_mgr.isActiveIface("eth2"));
 }
 
+// Checks whether option space can be detected as vendor-id
+TEST_F(DhcpParserTest, vendorOptionSpace) {
+    EXPECT_EQ(0, SubnetConfigParser::optionSpaceToVendorId(""));
+    EXPECT_EQ(0, SubnetConfigParser::optionSpaceToVendorId("dhcp4"));
+    EXPECT_EQ(0, SubnetConfigParser::optionSpaceToVendorId("vendor-"));
+    EXPECT_EQ(1, SubnetConfigParser::optionSpaceToVendorId("vendor-1"));
+    EXPECT_EQ(4491, SubnetConfigParser::optionSpaceToVendorId("vendor-4491"));
+    EXPECT_EQ(12345678, SubnetConfigParser::optionSpaceToVendorId("vendor-12345678"));
+}
+
 /// @brief Test Implementation of abstract OptionDataParser class. Allows
 /// testing basic option parsing.
 class UtestOptionDataParser : public OptionDataParser {