[3292] Addressed review comments.

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -1857,7 +1857,7 @@ TEST_F(Dhcp4ParserTest, optionDataInSingleSubnet) {
     testOption(*range.first, 23, foo2_expected, sizeof(foo2_expected));
 }
 
-// The goal of this test is to check that the option carrying a bolean
+// The goal of this test is to check that the option carrying a boolean
 // value can be configured using one of the values: "true", "false", "0"
 // or "1".
 TEST_F(Dhcp4ParserTest, optionDataBoolean) {

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -2198,7 +2198,7 @@ TEST_F(Dhcp6ParserTest, optionDataInMultipleSubnets) {
                sizeof(user_class_expected));
 }
 
-// The goal of this test is to check that the option carrying a bolean
+// The goal of this test is to check that the option carrying a boolean
 // value can be configured using one of the values: "true", "false", "0"
 // or "1".
 TEST_F(Dhcp6ParserTest, optionDataBoolean) {

src/lib/dhcp/option_definition.cc

@@ -399,33 +399,47 @@ OptionDefinition::haveVendor6Format() const {
     return  (getType() == OPT_UINT32_TYPE && !getEncapsulatedSpace().empty());
 }
 
+bool
+OptionDefinition::convertToBool(const std::string& value_str) const {
+    // Case insensitve check that the input is one of: "true" or "false".
+    if (boost::iequals(value_str, "true")) {
+        return (true);
+
+    } else if (boost::iequals(value_str, "false")) {
+        return (false);
+
+    }
+
+    // The input string is neither "true" nor "false", so let's check
+    // if it is not an integer wrapped in a string.
+    int result;
+    try {
+       result = boost::lexical_cast<int>(value_str);
+
+    } catch (const boost::bad_lexical_cast&) {
+        isc_throw(BadDataTypeCast, "unable to covert the value '"
+                  << value_str << "' to boolean data type");
+    }
+    // The boolean value is encoded in DHCP option as 0 or 1. Therefore,
+    // we only allow a user to specify those values for options which
+    // have boolean fields.
+    if (result != 1 && result != 0) {
+        isc_throw(BadDataTypeCast, "unable to convert '" << value_str
+                  << "' to boolean data type");
+    }
+    return (static_cast<bool>(result));
+}
+
 template<typename T>
 T
 OptionDefinition::lexicalCastWithRangeCheck(const std::string& value_str)
     const {
-    // Lexical cast in case of our data types make sense only
-    // for uintX_t, intX_t and bool type.
-    if (!OptionDataTypeTraits<T>::integer_type &&
-        OptionDataTypeTraits<T>::type != OPT_BOOLEAN_TYPE) {
+    // The lexical cast should be attempted when converting to an integer
+    // value only.
+    if (!OptionDataTypeTraits<T>::integer_type) {
         isc_throw(BadDataTypeCast,
-                  "unable to do lexical cast to non-integer and"
-                  << " non-boolean data type");
-    }
-
-    // The lexical cast will not handle conversion of the string holding
-    // "true" or "false". Therefore we will do the conversion on our own.
-    // If the string value doesn't match any of "true" or "false", it
-    // is possible that "0" or "1" has been specified, which we are
-    // ok with. For conversion of "0" or "1" we will try lexical cast
-    // below.
-    if (OptionDataTypeTraits<T>::type == OPT_BOOLEAN_TYPE) {
-        if (value_str == "true") {
-            return (true);
-
-        } else if (value_str == "false") {
-            return (false);
-
-        }
+                  "must not convert '" << value_str
+                  << "' to non-integer data type");
     }
 
     // We use the 64-bit value here because it has wider range than
@@ -433,22 +447,15 @@ OptionDefinition::lexicalCastWithRangeCheck(const std::string& value_str)
     // 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.
-    // Note that with the conversion below we also handle boolean
-    // values specified as "0" or "1".
     int64_t result = 0;
     try {
         result = boost::lexical_cast<int64_t>(value_str);
+
     } catch (const boost::bad_lexical_cast&) {
-        // Prepare error message here.
-        std::string data_type_str = "boolean";
-        if (OptionDataTypeTraits<T>::integer_type) {
-            data_type_str = "integer";
-        }
         isc_throw(BadDataTypeCast, "unable to convert the value '"
-                  << value_str << "' to " << data_type_str
-                  << " data type");
+                  << value_str << "' to integer data type");
     }
-    // Perform range checks for integer values only (exclude bool values).
+    // Perform range checks.
     if (OptionDataTypeTraits<T>::integer_type) {
         if (result > numeric_limits<T>::max() ||
             result < numeric_limits<T>::min()) {
@@ -458,12 +465,6 @@ OptionDefinition::lexicalCastWithRangeCheck(const std::string& value_str)
                       << numeric_limits<T>::min()
                       << ".." << numeric_limits<T>::max());
         }
-    // The specified value is of the boolean type and we are checking
-    // that it has successfuly converted to 0 or 1. Any other numeric
-    // value is not accepted to represent boolean value.
-    } else if (result != 1 && result != 0) {
-        isc_throw(BadDataTypeCast, "unable to convert '" << value_str
-                  << "' to boolean data type");
     }
     return (static_cast<T>(result));
 }
@@ -484,8 +485,7 @@ OptionDefinition::writeToBuffer(const std::string& value,
         // That way we actually waste 7 bits but it seems to be the
         // simpler way to encode boolean.
         // @todo Consider if any other encode methods can be used.
-        OptionDataTypeUtil::writeBool(lexicalCastWithRangeCheck<bool>(value),
-                                      buf);
+        OptionDataTypeUtil::writeBool(convertToBool(value), buf);
         return;
     case OPT_INT8_TYPE:
         OptionDataTypeUtil::writeInt<uint8_t>

src/lib/dhcp/option_definition.h

@@ -573,19 +573,38 @@ private:
         return (type == type_);
     }
 
+    /// @brief Converts a string value to a boolean value.
+    ///
+    /// This function converts the value represented as string to a boolean
+    /// value. The following conversions are acceptable:
+    /// - "true" => true
+    /// - "false" => false
+    /// - "1" => true
+    /// - "0" => false
+    /// The first two conversions are case insensitive, so as conversions from
+    /// strings such as "TRUE", "trUE" etc. will be accepted. Note that the
+    /// only acceptable integer values, carried as strings are: "0" and "1".
+    /// For other values, e.g. "2", "3" etc. an exception will be thrown
+    /// during conversion.
+    ///
+    /// @param value_str Input value.
+    ///
+    /// @return boolean representation of the string specified as the parameter.
+    /// @throw isc::dhcp::BadDataTypeCast if failed to perform the conversion.
+    bool convertToBool(const std::string& value_str) const;
+
     /// @brief Perform lexical cast of the value and validate its range.
     ///
     /// This function performs lexical cast of a string value to integer
-    /// or boolean value and checks if the resulting value is within a
-    /// range of a target type. Note that range checks are not performed
-    /// on boolean values. The target type should be one of the supported
-    /// integer types or bool.
+    /// value and checks if the resulting value is within a range of a
+    /// target type. The target type should be one of the supported
+    /// integer types.
     ///
     /// @param value_str input value given as string.
-    /// @tparam T target type for lexical cast.
+    /// @tparam T target integer type for lexical cast.
     ///
-    /// @return cast value.
-    /// @throw BadDataTypeCast if cast was not successful.
+    /// @return Integer value after conversion from the string.
+    /// @throw isc::dhcp::BadDataTypeCast if conversion was not successful.
     template<typename T>
     T lexicalCastWithRangeCheck(const std::string& value_str) const;
 

src/lib/dhcp/tests/option_definition_unittest.cc

@@ -693,8 +693,7 @@ TEST_F(OptionDefinitionTest, boolValue) {
 
     // Try to provide zero-length buffer. Expect exception.
     EXPECT_THROW(
-        option_v4 = opt_def.optionFactory(Option::V6, DHO_IP_FORWARDING,
-                                          OptionBuffer()),
+        opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING, OptionBuffer()),
         InvalidOptionValue
     );
 
@@ -711,10 +710,8 @@ TEST_F(OptionDefinitionTest, boolTokenized) {
 
     OptionPtr option_v4;
     std::vector<std::string> values;
-    // Provide two boolean values. It is expected that only the first
-    // one will be used.
+    // Specify a value for the option instance being created.
     values.push_back("true");
-    values.push_back("false");
     ASSERT_NO_THROW(
         option_v4 = opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING,
                                           values);
@@ -727,7 +724,6 @@ TEST_F(OptionDefinitionTest, boolTokenized) {
 
     // Repeat the test but for "false" value this time.
     values[0] = "false";
-    values[1] = "true";
     ASSERT_NO_THROW(
         option_v4 = opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING,
                                           values);
@@ -739,7 +735,6 @@ TEST_F(OptionDefinitionTest, boolTokenized) {
 
     // Check if that will work for numeric values.
     values[0] = "0";
-    values[1] = "1";
     ASSERT_NO_THROW(
         option_v4 = opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING,
                                           values);
@@ -751,7 +746,6 @@ TEST_F(OptionDefinitionTest, boolTokenized) {
 
     // Swap numeric values and test if it works for "true" case.
     values[0] = "1";
-    values[1] = "0";
     ASSERT_NO_THROW(
         option_v4 = opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING,
                                           values);
@@ -764,14 +758,12 @@ TEST_F(OptionDefinitionTest, boolTokenized) {
     // A conversion of non-numeric value to boolean should fail if
     // this value is neither "true" nor "false".
     values[0] = "garbage";
-    values[1] = "false";
     EXPECT_THROW(opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING, values),
       isc::dhcp::BadDataTypeCast);
 
     // A conversion of numeric value to boolean should fail if this value
     // is neither "0" nor "1".
     values[0] = "2";
-    values[1] = "0";
     EXPECT_THROW(opt_def.optionFactory(Option::V4, DHO_IP_FORWARDING, values),
       isc::dhcp::BadDataTypeCast);