[2490] Added support for string data field within an option.

src/lib/dhcp/option_definition.cc

@@ -64,8 +64,8 @@ T OptionDefinition::DataTypeUtil::lexicalCastWithRangeCheck(const std::string& v
         isc_throw(BadDataTypeCast, "unable to do lexical cast to non-integer and"
                   << " non-boolean data type");
     }
-    // We use the 64-bit value here because it has greater range than
-    // any other type we use here and it allows to detect some out of
+    // We use the 64-bit value here because it has wider range than
+    // any other type we use here and it allows to detect out of
     // bounds conditions e.g. negative value specified for uintX_t
     // data type. Obviously if the value exceeds the limits of int64
     // this function will not handle that properly.
@@ -97,9 +97,15 @@ void
 OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
                                               const OptionDataType type,
                                               OptionBuffer& buf) {
+    // We are going to write value given by value argument to the buffer.
+    // The actual type of the value is given by second argument. Check
+    // this argument to determine how to write this value to the buffer.
     switch (type) {
     case OPT_BINARY_TYPE:
         {
+            // Binary value means that the value is encoded as a string
+            // of hexadecimal deigits. We need to decode this string
+            // to the binary format here.
             OptionBuffer binary;
             try {
                 util::encode::decodeHex(value, binary);
@@ -107,11 +113,17 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
                 isc_throw(BadDataTypeCast, "unable to cast " << value
                           << " to binary data type: " << ex.what());
             }
+            // Decode was successful so append decoded binary value
+            // to the buffer.
             buf.insert(buf.end(), binary.begin(), binary.end());
             return;
         }
     case OPT_BOOLEAN_TYPE:
         {
+            // We encode the true value as 1 and false as 0 on 8 bits.
+            // That way we actually waist 7 bits but it seems to be the
+            // simpler way to encode boolean.
+            // @todo Consider if any other encode methods can be used.
             bool bool_value = lexicalCastWithRangeCheck<bool>(value);
             if (bool_value) {
                 buf.push_back(static_cast<uint8_t>(1));
@@ -122,11 +134,15 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
         }
     case OPT_INT8_TYPE:
         {
+            // Buffer holds the uin8_t values so we need to cast the signed
+            // value to unsigned but the bits values remain untouched.
             buf.push_back(static_cast<uint8_t>(lexicalCastWithRangeCheck<int8_t>(value)));
             return;
         }
     case OPT_INT16_TYPE:
         {
+            // Write the int16 value as uint16 value is ok because the bit values
+            // remain untouched.
             int16_t int_value = lexicalCastWithRangeCheck<int16_t>(value);
             buf.resize(buf.size() + 2);
             writeUint16(static_cast<uint16_t>(int_value), &buf[buf.size() - 2]);
@@ -160,6 +176,9 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
         }
     case OPT_IPV4_ADDRESS_TYPE:
         {
+            // The easiest way to get the binary form of IPv4 address is
+            // to create IOAddress object from string and use its accessors
+            // to retrieve the binary form.
             asiolink::IOAddress address(value);
             if (!address.getAddress().is_v4()) {
                 isc_throw(BadDataTypeCast, "provided address " << address.toText()
@@ -167,6 +186,7 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
             }
             asio::ip::address_v4::bytes_type addr_bytes =
                 address.getAddress().to_v4().to_bytes();
+            // Increase the buffer size by the size of IPv4 address.
             buf.resize(buf.size() + addr_bytes.size());
             std::copy_backward(addr_bytes.begin(), addr_bytes.end(),
                                buf.end());
@@ -181,16 +201,39 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
             }
             asio::ip::address_v6::bytes_type addr_bytes =
                 address.getAddress().to_v6().to_bytes();
+            // Incresase the buffer size by the size of IPv6 address.
             buf.resize(buf.size() + addr_bytes.size());
             std::copy_backward(addr_bytes.begin(), addr_bytes.end(),
                                buf.end());
             return;
         }
+    case OPT_STRING_TYPE:
+        if (value.size() > 0) {
+            // Increase the size of the storage by the size of the string.
+            buf.resize(buf.size() + value.size());
+            // Assuming that the string is already UTF8 encoded.
+            std::copy_backward(value.c_str(), value.c_str() + value.length(),
+                               buf.end());
+            return;
+        }
+    case OPT_FQDN_TYPE:
+        {
+            // FQDN implementation is not terribly complicated but will require
+            // creation of some additional logic (maybe object) that will parse
+            // the fqdn into labels.
+            isc_throw(isc::NotImplemented, "write of FQDN record into option buffer"
+                      " is not supported yet");
+            return;
+        }
     default:
+        // We hit this point because invalid option data type has been specified
+        // This may be the case because 'empty' or 'record' data type has been
+        // specified. We don't throw exception here because it will be thrown
+        // at the exit point from this function.
         ;
     }
-    isc_throw(isc::NotImplemented, "write of string, FQDN record into option buffer"
-              " is not supported yet");
+    isc_throw(isc::BadValue, "attempt to write invalid option data field type"
+              " into the option buffer: " << type);
 
 }
 

src/lib/dhcp/tests/option_definition_unittest.cc

@@ -149,7 +149,7 @@ TEST_F(OptionDefinitionTest, validate) {
     EXPECT_THROW(opt_def6.validate(), isc::BadValue);
 }
 
-TEST_F(OptionDefinitionTest, factoryAddrList6) {
+TEST_F(OptionDefinitionTest, ipv6AddressArray) {
     OptionDefinition opt_def("OPTION_NIS_SERVERS", D6O_NIS_SERVERS,
                              "ipv6-address", true);
 
@@ -200,7 +200,7 @@ TEST_F(OptionDefinitionTest, factoryAddrList6) {
 
 // This test checks that a vector of strings, holding IPv6 addresses,
 // can be used to create option instance with the optionFactory function.
-TEST_F(OptionDefinitionTest, factoryTokenizedAddrList6) {
+TEST_F(OptionDefinitionTest, ipv6AddressArrayTokenized) {
     OptionDefinition opt_def("OPTION_NIS_SERVERS", D6O_NIS_SERVERS,
                              "ipv6-address", true);
 
@@ -242,7 +242,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedAddrList6) {
     EXPECT_TRUE(std::equal(addrs.begin(), addrs.end(), addrs_returned.begin()));
 }
 
-TEST_F(OptionDefinitionTest, factoryAddrList4) {
+TEST_F(OptionDefinitionTest, ipv4AddressArray) {
     OptionDefinition opt_def("OPTION_NAME_SERVERS", D6O_NIS_SERVERS,
                              "ipv4-address", true);
 
@@ -290,7 +290,7 @@ TEST_F(OptionDefinitionTest, factoryAddrList4) {
 
 // This test checks that a vector of strings, holding IPv4 addresses,
 // can be used to create option instance with the optionFactory function.
-TEST_F(OptionDefinitionTest, factoryTokenizedAddrList4) {
+TEST_F(OptionDefinitionTest, ipv4AddressArrayTokenized) {
     OptionDefinition opt_def("OPTION_NIS_SERVERS", DHO_NIS_SERVERS,
                              "ipv4-address", true);
 
@@ -332,7 +332,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedAddrList4) {
     EXPECT_TRUE(std::equal(addrs.begin(), addrs.end(), addrs_returned.begin()));
 }
 
-TEST_F(OptionDefinitionTest, factoryEmpty) {
+TEST_F(OptionDefinitionTest, empty) {
     OptionDefinition opt_def("OPTION_RAPID_COMMIT", D6O_RAPID_COMMIT, "empty");
 
     // Create option instance and provide empty buffer as expected.
@@ -355,7 +355,7 @@ TEST_F(OptionDefinitionTest, factoryEmpty) {
     EXPECT_EQ(0, option_v4->getData().size());
 }
 
-TEST_F(OptionDefinitionTest, factoryBinary) {
+TEST_F(OptionDefinitionTest, binary) {
     // Binary option is the one that is represented by the generic
     // Option class. In fact all options can be represented by this
     // class but for some of them it is just natural. The SERVERID
@@ -402,7 +402,7 @@ TEST_F(OptionDefinitionTest, factoryBinary) {
                            buf.begin()));
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedBinary) {
+TEST_F(OptionDefinitionTest, binaryTokenized) {
     OptionDefinition opt_def("OPTION_FOO", 1000, "binary", true);
 
     // Prepare some dummy data (serverid): 0, 1, 2 etc.
@@ -449,7 +449,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedBinary) {
 }
 
 
-TEST_F(OptionDefinitionTest, factoryIA6) {
+TEST_F(OptionDefinitionTest, recordIA6) {
     // This option consists of IAID, T1 and T2 fields (each 4 bytes long).
     const int option6_ia_len = 12;
 
@@ -487,7 +487,7 @@ TEST_F(OptionDefinitionTest, factoryIA6) {
      );
 }
 
-TEST_F(OptionDefinitionTest, factoryIAAddr6) {
+TEST_F(OptionDefinitionTest, recordIAAddr6) {
     // This option consists of IPV6 Address (16 bytes) and preferred-lifetime and
     // valid-lifetime fields (each 4 bytes long).
     const int option6_iaaddr_len = 24;
@@ -531,7 +531,7 @@ TEST_F(OptionDefinitionTest, factoryIAAddr6) {
      );
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedIAAddr6) {
+TEST_F(OptionDefinitionTest, recordIAAddr6Tokenized) {
     // This option consists of IPV6 Address (16 bytes) and preferred-lifetime and
     // valid-lifetime fields (each 4 bytes long).
     OptionDefinition opt_def("OPTION_IAADDR", D6O_IAADDR, "record");
@@ -562,7 +562,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedIAAddr6) {
     );
 }
 
-TEST_F(OptionDefinitionTest, factoryIntegerInvalidType) {
+TEST_F(OptionDefinitionTest, integerInvalidType) {
     // The template function factoryInteger<> accepts integer values only
     // as template typename. Here we try passing different type and
     // see if it rejects it.
@@ -574,7 +574,7 @@ TEST_F(OptionDefinitionTest, factoryIntegerInvalidType) {
     );
 }
 
-TEST_F(OptionDefinitionTest, factoryUint8) {
+TEST_F(OptionDefinitionTest, uint8) {
     OptionDefinition opt_def("OPTION_PREFERENCE", D6O_PREFERENCE, "uint8");
 
     OptionPtr option_v6;
@@ -597,7 +597,7 @@ TEST_F(OptionDefinitionTest, factoryUint8) {
     // @todo Add more cases for DHCPv4
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedUint8) {
+TEST_F(OptionDefinitionTest, uint8Tokenized) {
     OptionDefinition opt_def("OPTION_PREFERENCE", D6O_PREFERENCE, "uint8");
 
     OptionPtr option_v6;
@@ -622,7 +622,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedUint8) {
 }
 
 
-TEST_F(OptionDefinitionTest, factoryUint16) {
+TEST_F(OptionDefinitionTest, uint16) {
     OptionDefinition opt_def("OPTION_ELAPSED_TIME", D6O_ELAPSED_TIME, "uint16");
 
     OptionPtr option_v6;
@@ -648,7 +648,7 @@ TEST_F(OptionDefinitionTest, factoryUint16) {
     // @todo Add more cases for DHCPv4
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedUint16) {
+TEST_F(OptionDefinitionTest, uint16Tokenized) {
     OptionDefinition opt_def("OPTION_ELAPSED_TIME", D6O_ELAPSED_TIME, "uint16");
 
     OptionPtr option_v6;
@@ -669,7 +669,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedUint16) {
 
 }
 
-TEST_F(OptionDefinitionTest, factoryUint32) {
+TEST_F(OptionDefinitionTest, uint32) {
     OptionDefinition opt_def("OPTION_CLT_TIME", D6O_CLT_TIME, "uint32");
 
     OptionPtr option_v6;
@@ -696,7 +696,7 @@ TEST_F(OptionDefinitionTest, factoryUint32) {
     // @todo Add more cases for DHCPv4
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedUint32) {
+TEST_F(OptionDefinitionTest, uint32Tokenized) {
     OptionDefinition opt_def("OPTION_CLT_TIME", D6O_CLT_TIME, "uint32");
 
     OptionPtr option_v6;
@@ -715,8 +715,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedUint32) {
     // @todo Add more cases for DHCPv4
 }
 
-
-TEST_F(OptionDefinitionTest, factoryUint16Array) {
+TEST_F(OptionDefinitionTest, uint16Array) {
     // Let's define some dummy option.
     const uint16_t opt_code = 79;
     OptionDefinition opt_def("OPTION_UINT16_ARRAY", opt_code, "uint16", true);
@@ -759,7 +758,7 @@ TEST_F(OptionDefinitionTest, factoryUint16Array) {
     );
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedUint16Array) {
+TEST_F(OptionDefinitionTest, uint16ArrayTokenized) {
     // Let's define some dummy option.
     const uint16_t opt_code = 79;
     OptionDefinition opt_def("OPTION_UINT16_ARRAY", opt_code, "uint16", true);
@@ -783,7 +782,7 @@ TEST_F(OptionDefinitionTest, factoryTokenizedUint16Array) {
 }
 
 
-TEST_F(OptionDefinitionTest, factoryUint32Array) {
+TEST_F(OptionDefinitionTest, uint32Array) {
     // Let's define some dummy option.
     const uint16_t opt_code = 80;
 
@@ -827,7 +826,7 @@ TEST_F(OptionDefinitionTest, factoryUint32Array) {
     );
 }
 
-TEST_F(OptionDefinitionTest, factoryTokenizedUint32Array) {
+TEST_F(OptionDefinitionTest, uint32ArrayTokenized) {
     // Let's define some dummy option.
     const uint16_t opt_code = 80;
 
@@ -854,6 +853,26 @@ TEST_F(OptionDefinitionTest, factoryTokenizedUint32Array) {
     EXPECT_EQ(1111, values[3]);
 }
 
+TEST_F(OptionDefinitionTest, utf8StringTokenized) {
+    // Let's create some dummy option.
+    const uint16_t opt_code = 80;
+    OptionDefinition opt_def("OPTION_WITH_STRING", opt_code, "string");
+    
+    std::vector<std::string> values;
+    values.push_back("Hello World");
+    values.push_back("this string should not be included in the option");
+    OptionPtr option_v6;
+    EXPECT_NO_THROW(
+        option_v6 = opt_def.optionFactory(Option::V6, opt_code, values);
+    );
+    ASSERT_TRUE(option_v6);
+    ASSERT_TRUE(typeid(*option_v6) == typeid(Option));
+    std::vector<uint8_t> data = option_v6->getData();
+    std::vector<uint8_t> ref_data(values[0].c_str(), values[0].c_str()
+                                  + values[0].length());
+    EXPECT_TRUE(std::equal(ref_data.begin(), ref_data.end(), data.begin()));
+}
+
 TEST_F(OptionDefinitionTest, recognizeFormat) {
     // IA_NA option format.
     OptionDefinition opt_def1("OPTION_IA_NA", D6O_IA_NA, "record");