BIND 10 #3173: Add support to perfdhcp to acquire IPv6 prefixes (Prefix Delegation)

[BIND 10 Development]

#3173: Add support to perfdhcp to acquire IPv6 prefixes (Prefix Delegation)
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  perfdhcp      |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:3 tmark]:
 > command_options.cc
 >
 > Question: Why 'e' for the new parameter letter? Why not 'l' for lease
 type or 't' for type?  There doesn't seem to anything particularly
 mnemonic about 'e'.
 > (At least not in English ;) )

 Check the perfdhcp documentation or type ./perfdhcp --help. There is a
 load of parameters already used (including L, l, t and T). I just don't
 know if there is any letter remaining which would be more meaningful.
 Apparently, I will have to start using full words instead of letters.

 >
 >
 ----------------------------------------------------------------------------
 >
 > Question: Why are template files restricted to address-only?
 >
 > +    check(!getTemplateFiles().empty() && (getLeaseType() !=
 ADDRESS_ONLY),
 > +          "template files may be only used with '-e address-only'");
 >
 >

 Because of the time constraints. It shouldn't really be a problem to allow
 !''prefix-only!''. It would require changing the command line description
 to indicate that the !''-I<ip-offset>!'' may indicate the IA_PD instead of
 IA_NA. It would also require some additional unit testing - which involves
 creation of the test template files etc. So, it is doable but I don't want
 to spend time on something that is not needed before November.

 Note, that use of templates for combination of IA_NA and IA_PD
 (!''address-and-prefix!'') will not be trivial because it would require
 reorganization of the command line (or maybe implementation of the config
 file?) for templates (need to specify where IA_NA AND IA_PD are). I have
 taken a shortcut and disabled templates for anything else than simple IPv6
 address assignment because we will need to think how to better organize
 templates anyway.

 >
 ----------------------------------------------------------------------------
 > command_options.cc
 >
 > Should this method:
 >
 > {{{
 > std::string CommandOptions::LeaseType::toText()
 > }}}
 >
 > throw rather than return a string that says "unknown value" or
 something?
 > Usually things like this are used for log messages and such.  It can be
 > inconvenient if they throw.

 My thinking was that if toText() gets the wrong enum value it must be
 something badly wrong with the code. In theory, this should not happen. It
 could only happen when someone has added a new lease-type enumerator and
 forgot to extend the toText() function to support it. In such case I
 prefer to throw an exception so as the implementer finds out that he
 hasn't extended toText() function. If I just return !''unknown!'' it is
 possible that he will not notice that.

 >
 >
 ----------------------------------------------------------------------------
 > command_options_test.cc
 >
 > TEST(LeaseTypeTest, toText)
 >
 > Does not test the case of an invalid value.

 The problem is that !''LeaseType!'' is an enumeration. The valid values
 are: ADDRESS, PREFIX and ADDRESS_AND_PREFIX. There are no other values I
 could pass to this function because they are not valid enumerators. Even
 if I cast the integer value (say 3) to pass to this function the behavior
 of the compiler is implementation-dependent, which means that I can either
 get value of 3 passed to the function which will result in an exception,
 or I can get anything else (say 2 - which will be valid). Therefore, the
 behavior of this test will not be deterministic. It will depend on the
 platform.

 >
 >
 ----------------------------------------------------------------------------
 >
 > test_control.h
 >
 > I think this comment:
 >  {{{
 >     /// \todo In the future it is planned to add support for the
 perfdhcp to
 >     /// request address and prefix in the same DHCP message (request
 both
 >     /// IA_NA and IA_PD options). In this case, this function will have
 to
 >     /// be extended to copy both IA_NA and IA_PD options.
 > }}}
 >
 > is obsolete. The method already copies address, prefix, or both.

 Good catch! Initially I implemented IA_NA or IA_PD (never both). Then I
 updated the code to support both and the comment indeed has become
 obsolete.

 >
 >
 ----------------------------------------------------------------------------
 >
 > test_control_unittest.cc
 >
 > The two new tests, Packet6ExchangePrefixDelegation and
 Packet6ExchangeAddressAndPrefix do not really explain what exactly is
 being tested.  I think a little additional commentary for each would be
 helpful.

 I added some commentary.

 >
 >
 ----------------------------------------------------------------------------
 >
 > test_control_unittest.cc
 >
 > I do not see a packet exchange test for command line  "-e address_only".

 There is one new test for this now.

 >
 >
 --------------------------------------------------------------------------
 >
 > I still get one cppcheck error:
 >
 > {{{
 > tests/tools/perfdhcp/tests/stats_mgr_unittest.cc:91: check_fail:
 Function parameter 'xchg_type' should be passed by reference.
 (performance,passedByValue)
 > }}}
 >

 Corrected.

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