BIND 10 #3292: Bindctl incapable to configure boolean options on Kea4

[BIND 10 Development]

#3292: Bindctl incapable to configure boolean options on Kea4
-------------------------------------+-------------------------------------
            Reporter:  wlodekwencel  |                        Owner:
                Type:  defect        |  marcin
            Priority:  medium        |                       Status:
           Component:  bind-ctl      |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  QA Defects
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  8             |                 CVSS Scoring:
         Total Hours:  15            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  3
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  2 => 3
 * owner:  tmark => marcin
 * totalhours:  12 => 15


Comment:

 src/lib/dhcp/option_definition.cc

 OptionDefinition::lexicalCastWithRangeCheck(const std::string& value_str)

 1. Shouldn't string comparisons for "true" and "false" be case
 insensitive?

 2. I think it would be better to create a separate method for converting
 booleans
 and let lexicalCastWithRangeCheck handle only integer types. You end up
 with two
 methods that are much cleaner and simpler to test, something like this:

 {{{
 bool
 OptionDefinition::convertToBool(const std::string& value_str)
     const {
     if (OptionDataTypeTraits<T>::type == OPT_BOOLEAN_TYPE) {
         if (boost::iequals(value_str,"true")) {
             return (true);

         } else if (boost::iequals(value_str, "false")) {
             return (false);
         }
     }

     uint8_t result = 0;
     try {
         result = boost::lexical_cast<uint8_t>(value_str);
     } catch (const boost::bad_lexical_cast&) {
         isc_throw(BadDataTypeCast, "unable to convert the value '"
                   << value_str << "' to boolean");
     }

     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.
     if (!OptionDataTypeTraits<T>::integer_type) {
         isc_throw(BadDataTypeCast,
                   "unable to do lexical cast to non-integer");
     }

     // 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.
     int64_t result = 0;
     try {
         result = boost::lexical_cast<int64_t>(value_str);
     } catch (const boost::bad_lexical_cast&) {
         isc_throw(BadDataTypeCast, "unable to convert the value '"
                   << value_str << "' to integer");
     }

     // Perform range checks for integer values only (exclude bool values).
     if (result > numeric_limits<T>::max() ||
         result < numeric_limits<T>::min()) {
             isc_throw(BadDataTypeCast, "unable to convert '"
                       << value_str << "' to numeric type. This value is "
                       " expected to be in the range of "
                       << numeric_limits<T>::min()
                       << ".." << numeric_limits<T>::max());
     }

     return (static_cast<T>(result));
 }
 }}}


 --------------------------------------------------------------------------


 option_definition_unittest.cc

 T_F(OptionDefinitionTest, boolValue)

 line 696: You assign option_v4 the value returned by the optionFactory()
 having passed in an option space of Option::V6.  While perhaps irrelevant
 to the test, it does look rather odd.  If the intent is to verify it under
 both V4 and V6 that's good but you should consider renaming the local
 variable to just "option_ptr".

 --------------------------------------------------------------------------
 option_definition_unittest.cc

 TEST_F(OptionDefinitionTest, boolTokenized)

 In this test a vector, "values", is passed into the optionFactory.  In
 each
 case there are two elements in the vector but only the first one is used.
 The second entry is never parsed and has no affect on the outcome of any
 of
 the tests.  To prove this I commented out each line that set values[1] to
 a
 value and all of the tests still pass.  These should be removed.  I spent
 a bit of time trying to understand the significance of these entries only
 to discover there is none.

 --------------------------------------------------------------------------

 src/bin/dhcp4/tests/config_parser_unittest.cc

 line 1860: "carrying a bolean"  I believe you mean "boolean" ;)

 Same interesting spelling in V6 version of this too.

 --------------------------------------------------------------------------


 cppcheck.sh is ok, and unittests for dhcp6,dhcp4, and libdhcp all pass
 with valgrind.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3292#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development