[2312] Code cleanup in OptionCustom::createBuffers.

src/lib/dhcp/option_custom.cc

@@ -52,57 +52,87 @@ OptionCustom::createBuffers() {
 
     std::vector<OptionBuffer> buffers;
     OptionBuffer::iterator data = data_.begin();
-    if (definition_.getType() == OPT_RECORD_TYPE) {
+
+    OptionDataType data_type = definition_.getType();
+    if (data_type == OPT_RECORD_TYPE) {
+        // An option comprises a record of data fields. We need to
+        // get types of these data fields to allocate enough space
+        // for each buffer.
         const OptionDefinition::RecordFieldsCollection& fields =
             definition_.getRecordFields();
+
+        // Go over all data fields within a record.
         for (OptionDefinition::RecordFieldsConstIter field = fields.begin();
              field != fields.end(); ++field) {
+            // For fixed-size data type such as boolean, integer, even
+            // IP address we can use the utility function to get the required
+            // buffer size.
             int data_size = OptionDataTypeUtil::getDataTypeLen(*field);
+
+            // For variable size types (such as string) the function above
+            // will return 0 so we need to do a runtime check. Since variable
+            // length data fields may be laid only at the end of an option we
+            // consume the rest of this option. Note that validate() function
+            // in OptionDefinition object should have checked whether the
+            // data fields layout is correct (that the variable string fields
+            // are laid at the end).
             if (data_size == 0) {
-                if (*field == OPT_IPV4_ADDRESS_TYPE) {
-                    data_size = asiolink::V4ADDRESS_LEN;
-                } else if (*field == OPT_IPV6_ADDRESS_TYPE) {
-                    data_size = asiolink::V6ADDRESS_LEN;
-                } else {
-                    data_size = std::distance(data, data_.end());
-                }
+                data_size = std::distance(data, data_.end());
             }
+            // If we reached the end of buffer we assume that this option is
+            // truncated because there is no remaining data to initialize
+            // an option field.
             if (data_size == 0) {
                 isc_throw(OutOfRange, "option buffer truncated");
             }
+            // Store the created buffer.
             buffers.push_back(OptionBuffer(data, data + data_size));
+            // Proceed to the next data field.
             data += data_size;
         }
     } else {
-        OptionDataType data_type = definition_.getType();
+        // If option definition type is not a 'record' than definition
+        // type itself indicates what type of data is being held there.
         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;
-            }
-        }
+        // The check below will fail if the input buffer is too short
+        // for the data size being held by this option.
+        // Note that data_size returned by getDataTypeLen may be zero
+        // if variable length data is being held by the option but
+        // this will not cause this check to throw exception.
         if (std::distance(data, data_.end()) < data_size) {
             isc_throw(OutOfRange, "option buffer truncated");
         }
+        // For an array of values we are taking different path because
+        // we have to handle multiple buffers.
         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.
+            // have checked this already. Thus data_size must be greater than
+            // zero.
             assert(data_size > 0);
+            // Get equal chunks of data and store as collection of buffers.
+            // Truncate any remaining part which length is not divisible by
+            // data_size.
             do {
                 buffers.push_back(OptionBuffer(data, data + data_size));
                 data += data_size;
             } while (std::distance(data, data_.end()) >= data_size);
         } else {
+            // For non-arrays the data_size can be zero because
+            // getDataTypeLen returns zero for variable size data types
+            // such as strings. Simply take whole buffer.
             if (data_size == 0) {
                 data_size = std::distance(data, data_.end());
             }
-            buffers.push_back(OptionBuffer(data, data + data_size));
+            if (data_size > 0 && data_type != OPT_EMPTY_TYPE) {
+                buffers.push_back(OptionBuffer(data, data + data_size));
+            } else if (data_type != OPT_EMPTY_TYPE) {
+                isc_throw(OutOfRange, "option buffer truncated");
+            }
         }
     }
+    // If everything went ok we can replace old buffer set with new ones.
     std::swap(buffers_, buffers);
 }
 

src/lib/dhcp/option_data_types.cc

@@ -31,6 +31,10 @@ OptionDataTypeUtil::getDataTypeLen(const OptionDataType data_type) {
     case OPT_INT32_TYPE:
     case OPT_UINT32_TYPE:
         return (4);
+    case OPT_IPV4_ADDRESS_TYPE:
+        return (asiolink::V4ADDRESS_LEN);
+    case OPT_IPV6_ADDRESS_TYPE:
+        return (asiolink::V6ADDRESS_LEN);
     default:
         ;
     }

src/lib/dhcp/option_data_types.h

@@ -77,7 +77,7 @@ struct OptionDataTypeTraits {
 template<>
 struct OptionDataTypeTraits<OptionBuffer> {
     static const bool valid = true;
-    static const int len = sizeof(OptionBuffer);
+    static const int len = 0;
     static const bool integer_type = false;
     static const OptionDataType type = OPT_BINARY_TYPE;
 };
@@ -178,15 +178,19 @@ struct OptionDataTypeTraits<std::string> {
 class OptionDataTypeUtil {
 public:
 
-    /// @brief Get data type size.
+    /// @brief Get data type buffer length.
     ///
     /// This functionm returs the size of a particular data type.
     /// Values retured by this function correspond to the data type
-    /// sizes defined in OptionDataTypeTraits so they rather indicate
+    /// sizes defined in OptionDataTypeTraits (IPV4_ADDRESS_TYPE and
+    /// IPV6_ADDRESS_TYPE are exceptions here) so they rather indicate
     /// the fixed length of the data being written into the buffer,
     /// not the sizeof the particular data type. Thus for data types
     /// such as string, binary etc. for which the buffer length can't
     /// be determined this function returns 0.
+    /// In addition, this function returns the data sizes for
+    /// IPV4_ADDRESS_TYPE and IPV6_ADDRESS_TYPE as their buffer
+    /// representations have fixed data lengths: 4 and 16 respectively.
     ///
     /// @param data_type data type which size is to be returned.
     /// @return data type size or zero for variable length types.