[2312] Initialize custom option's buffers when option is not a record.

src/lib/dhcp/option_custom.cc

@@ -61,10 +61,10 @@ OptionCustom::createBuffers() {
     definition_.validate();
 
     std::vector<OptionBuffer> buffers;
+    OptionBuffer::iterator data = data_.begin();
     if (definition_.getType() == OPT_RECORD_TYPE) {
         const OptionDefinition::RecordFieldsCollection& fields =
             definition_.getRecordFields();
-        OptionBuffer::iterator data = data_.begin();
         for (OptionDefinition::RecordFieldsConstIter field = fields.begin();
              field != fields.end(); ++field) {
             int data_size = OptionDataTypeUtil::getDataTypeLen(*field);
@@ -73,22 +73,45 @@ OptionCustom::createBuffers() {
                     data_size = asiolink::V4ADDRESS_LEN;
                 } else if (*field == OPT_IPV6_ADDRESS_TYPE) {
                     data_size = asiolink::V6ADDRESS_LEN;
-                } else if (*field == OPT_STRING_TYPE ||
-                           *field == OPT_FQDN_TYPE ||
-                           *field == OPT_BINARY_TYPE) {
-                    data_size = std::distance(data, data_.end());
                 } else {
-                    isc_throw(InvalidDataType, "invalid option data type"
-                              << " used in the option record");
+                    data_size = std::distance(data, data_.end());
                 }
             }
-            if (std::distance(data, data_.end()) < data_size ||
-                data_size == 0) {
+            if (data_size == 0) {
                 isc_throw(OutOfRange, "option buffer truncated");
             }
             buffers_.push_back(OptionBuffer(data, data + data_size));
             data += data_size;
         }
+    } else {
+        OptionDataType data_type = definition_.getType();
+        int data_size = OptionDataTypeUtil::getDataTypeLen(data_type);
+        if (data_size == 0) {
+            if (data_type == OPT_IPV4_ADDRESS_TYPE) {
+                data_size = asiolink::V4ADDRESS_LEN;
+            } else if (data_type == OPT_IPV6_ADDRESS_TYPE) {
+                data_size = asiolink::V6ADDRESS_LEN;
+            }
+        }
+        if (std::distance(data, data_.end()) < data_size) {
+            isc_throw(OutOfRange, "option buffer truncated");
+        }
+        if (definition_.getArrayType()) {
+            // We don't perform other checks for data types that can't be
+            // used together with array indicator such as strings, empty field
+            // etc. This is because OptionDefinition::validate function should
+            // have checked this already.
+            assert(data_size > 0);
+            do {
+                buffers_.push_back(OptionBuffer(data, data + data_size));
+                data += data_size;
+            } while (std::distance(data, data_.end()) >= data_size);
+        } else {
+            if (data_size == 0) {
+                data_size = std::distance(data, data_.end());
+            }
+            buffers_.push_back(OptionBuffer(data, data + data_size));
+        }
     }
 }
 
@@ -105,7 +128,6 @@ OptionCustom::pack4(isc::util::OutputBuffer& buf) {
     // @todo write option data here
 
     LibDHCP::packOptions(buf, options_);
-    return;
 }
 
 void
@@ -116,7 +138,6 @@ OptionCustom::pack6(isc::util::OutputBuffer& buf) {
     // @todo write option data here.
 
     LibDHCP::packOptions(buf, options_);
-    return;
 }
 
 void

src/lib/dhcp/option_definition.cc

@@ -209,8 +209,10 @@ OptionDefinition::addRecordField(const OptionDataType data_type) {
         isc_throw(isc::InvalidOperation, "'record' option type must be used"
                   " to add data fields to the record");
     }
-    if (data_type >= OPT_UNKNOWN_TYPE) {
-        isc_throw(isc::BadValue, "attempted to add invalid data type to the record");
+    if (data_type >= OPT_RECORD_TYPE ||
+        data_type == OPT_ANY_ADDRESS_TYPE ||
+        data_type == OPT_EMPTY_TYPE) {
+        isc_throw(isc::BadValue, "attempted to add invalid data type to the record.");
     }
     record_fields_.push_back(data_type);
 }
@@ -321,18 +323,24 @@ OptionDefinition::validate() const {
     std::ostringstream err_str;
     if (name_.empty()) {
         // Option name must not be empty.
-        err_str << "option name must not be empty";
+        err_str << "option name must not be empty.";
     } else if (name_.find(" ") != string::npos) {
         // Option name must not contain spaces.
-        err_str << "option name must not contain spaces";
+        err_str << "option name must not contain spaces.";
     } else if (type_ >= OPT_UNKNOWN_TYPE) {
         // Option definition must be of a known type.
-        err_str << "option type value " << type_ << " is out of range";
-    } else if (type_ == OPT_STRING_TYPE && array_type_) {
-        // Array of strings is not allowed because there is no way
-        // to determine the size of a particular string and thus there
-        // it no way to tell when other data fields begin.
-        err_str << "array of strings is not a valid option definition";
+        err_str << "option type value " << type_ << " is out of range.";
+    } else if (array_type_) {
+        if (type_ == OPT_STRING_TYPE) {
+            // Array of strings is not allowed because there is no way
+            // to determine the size of a particular string and thus there
+            // it no way to tell when other data fields begin.
+            err_str << "array of strings is not a valid option definition.";
+        } else if (type_ == OPT_BINARY_TYPE) {
+            err_str << "array of binary values is not a valid option definition.";
+        } else if (type_ == OPT_EMPTY_TYPE) {
+            err_str << "array of empty value is not a valid option definition.";
+        }
     } else if (type_ == OPT_RECORD_TYPE) {
         // At least two data fields should be added to the record. Otherwise
         // non-record option definition could be used.
@@ -342,8 +350,8 @@ OptionDefinition::validate() const {
                     << " least 2 fields.";
         } else {
             // If the number of fields is valid we have to check if their order
-            // is valid too. We check that string data fields are not laid before
-            // other fields. But we allow that they are laid at the end of
+            // is valid too. We check that string or binary data fields are not
+            // laid before other fields. But we allow that they are laid at the end of
             // an option.
             const RecordFieldsCollection& fields = getRecordFields();
             for (RecordFieldsConstIter it = fields.begin();
@@ -354,6 +362,17 @@ OptionDefinition::validate() const {
                             << " of other types.";
                     break;
                 }
+                if (*it == OPT_BINARY_TYPE &&
+                    it < fields.end() - 1) {
+                    err_str << "binary data field can't be laid before data fields"
+                            << " of other types.";
+                }
+                /// Empty typy is not allowed within a record.
+                if (*it == OPT_EMPTY_TYPE) {
+                    err_str << "empty data type can't be stored as a field in an"
+                            << " option record.";
+                    break;
+                }
             }
         }
 

src/lib/dhcp/tests/option_custom_unittest.cc

@@ -70,6 +70,43 @@ TEST_F(OptionCustomTest, constructor) {
           ASSERT_THROW(opt_def1.validate(), isc::Exception); */
 }
 
+TEST_F(OptionCustomTest, setData) {
+    OptionDefinition opt_def("OPTION_FOO", 1000, "string");
+
+    OptionBuffer buf;
+    writeString("hello world!", buf);
+    OptionCustom option(opt_def, Option::V6, buf.begin(), buf.end());
+    ASSERT_TRUE(option.valid());
+
+    std::string value;
+    ASSERT_NO_THROW(option.readString(0, value));
+
+    EXPECT_EQ("hello world!", value);
+}
+
+TEST_F(OptionCustomTest, setArrayData) {
+    OptionDefinition opt_def("OPTION_FOO", 1000, "uint32", true);
+
+    std::vector<uint32_t> values;
+    values.push_back(71234);
+    values.push_back(12234);
+    values.push_back(54362);
+    values.push_back(1234);
+
+    OptionBuffer buf;
+    for (int i = 0; i < values.size(); ++i) {
+        writeInt<uint32_t>(values[i], buf);
+    }
+    OptionCustom option(opt_def, Option::V6, buf.begin(), buf.begin() + 13);
+    ASSERT_TRUE(option.valid());
+
+    for (int i = 0; i < 3; ++i) {
+        uint32_t value = 0;
+        ASSERT_NO_THROW(value = option.readInteger<uint32_t>(i));
+        EXPECT_EQ(values[i], value);
+    }
+}
+
 TEST_F(OptionCustomTest, setRecordData) {
     OptionDefinition opt_def("OPTION_FOO", 1000, "record");
     ASSERT_NO_THROW(opt_def.addRecordField("uint16"));

src/lib/dhcp/tests/option_definition_unittest.cc

@@ -458,56 +458,6 @@ TEST_F(OptionDefinitionTest, binary) {
                            buf.begin()));
 }
 
-// The purpose of this test is to verify that definition can be
-// creates for the option that holds binary data and that the
-// binary data can be specified in 'comma separated values'
-// format.
-TEST_F(OptionDefinitionTest, binaryTokenized) {
-    OptionDefinition opt_def("OPTION_FOO", 1000, "binary", true);
-
-    // Prepare some dummy data (serverid): 0, 1, 2 etc.
-    OptionBuffer buf(16);
-    for (int i = 0; i < buf.size(); ++i) {
-        buf[i] = i;
-    }
-    std::vector<std::string> hex_data;
-    hex_data.push_back("00010203");
-    hex_data.push_back("04050607");
-    hex_data.push_back("08090A0B0C0D0E0F");
-
-    // Create option instance with the factory function.
-    // If the OptionDefinition code works properly than
-    // object of the type Option should be returned.
-    OptionPtr option_v6;
-    ASSERT_NO_THROW(
-        option_v6 = opt_def.optionFactory(Option::V6, 1000, hex_data);
-    );
-    // Expect base option type returned.
-    ASSERT_TRUE(typeid(*option_v6) == typeid(Option));
-    // Sanity check on universe, length and size. These are
-    // the basic parameters identifying any option.
-    EXPECT_EQ(Option::V6, option_v6->getUniverse());
-    EXPECT_EQ(4, option_v6->getHeaderLen());
-    ASSERT_EQ(buf.size(), option_v6->getData().size());
-
-    // Get data from the option and compare against reference buffer.
-    // They are expected to match.
-    EXPECT_TRUE(std::equal(option_v6->getData().begin(),
-                           option_v6->getData().end(),
-                           buf.begin()));
-
-    // Repeat the same test scenario for DHCPv4 option.
-    OptionPtr option_v4;
-    ASSERT_NO_THROW(option_v4 = opt_def.optionFactory(Option::V4, 214, hex_data));
-    EXPECT_EQ(Option::V4, option_v4->getUniverse());
-    EXPECT_EQ(2, option_v4->getHeaderLen());
-    ASSERT_EQ(buf.size(), option_v4->getData().size());
-
-    EXPECT_TRUE(std::equal(option_v6->getData().begin(),
-                           option_v6->getData().end(),
-                           buf.begin()));
-}
-
 // The purpose of this test is to verify that definition can be created
 // for option that comprises record of data. In this particular test
 // the IA_NA option is used. This option comprises three uint32 fields.