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:  tmark
                Type:  defect        |                       Status:
            Priority:  medium        |  reviewing
           Component:  bind-ctl      |                    Milestone:  DHCP-
            Keywords:                |  QA Defects
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  8             |              Defect Severity:  N/A
         Total Hours:  17            |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  2
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

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


Comment:

 Replying to [comment:7 tmark]:
 > 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?
 >

 It may be. I changed that.

 > 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));
 > }
 > }}}
 >

 Ok. I split the function as suggested.

 >
 >
 --------------------------------------------------------------------------
 >
 >
 > 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".

 In fact, there is no reason to assign a returned value to anything, as
 this test verifies that the exception is thrown and should never get round
 to returning a value.

 >
 >
 --------------------------------------------------------------------------
 > 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.

 The function is supposed to ignore the second value and I thought that
 thanks to explicitly setting it in the test, we will check that it really
 does. However, I now removed it for clarity. Also, there is no real use
 case for too many parameters, so it should be ok.

 >
 >
 --------------------------------------------------------------------------
 >
 > 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.
 >

 Fixed.

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

 Thank you, QA Team.

 >

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