BIND 10 #1959: Implement perfdhcp control logic

[BIND 10 Development]

#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20120917
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  perfdhcp                           |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 '''tests/tools/perfdhcp/tests/command_options_helper.h'''
 represinting => representing

 CommandOptionsHelper seems like a useful class that could be shared
 between other components. dhcp4 and dhcp6 come to mind, but I'm sure there
 are others. You should investigate, if it is feasible to move it to
 src/lib/testutils.

 In the following code in CommandOptionsTest.Base:
 {{{
     // "t" is invalid character in DUID
     EXPECT_THROW(process("perfdhcp -6 -l ethx -b "
                          "duiD=010101010101010101t110111F14 all"),
                  isc::InvalidParameter);
 }}}
 is the duid capitalization (duiD) made on purpose?

 Is the parsing case-insensitive? I assume it is, but there seem to be no
 test for it. For parameters longer than single character, please check at
 least couple combinations, -Duid= -DuId= -DUID= etc.

 What about odd length DUID, e.g. 01234? Is it padded with a leading zero?
 Please test it.

 What about too short DUIDs, e.g. 12? That should be allowed for server
 testing purposes, but a warning should be added that DUID not-conformant
 to 3315 is used. If it is not allowed in the current code, an appropriate
 @TODO should be added. (it is minor and time should not be spent of
 implementing such a support now if it is missing).

 Please make sure that you test CommandOptionsTest.Interface on several
 non-linux systems. IfaceMgr detects interfaces on Linux only. On other
 systems it uses very simple approach: it tries to find interface index of
 "lo" interface. If that fails, it repeats with "lo0". This allows
 detecting loopback interfaces on all systems, except Windows. That is a
 bizarre OS that do not feature loopback concept. Fortunately, I think
 nobody pays any attention to this OS in BIND10 context.

 We do have plans to implement interface detection on other OSes, but such
 a ticket is not currently planned. Do you think it would be useful to
 implement it soon? There is a getifaddrs() interface that seems to be
 supported on Linux, all BSDs and surprisingly even Solaris 11.

 I don't like the fact that if no interfaces are detected that the test
 does nothing. In my opinion it should fail in such case.

 '''tests/tools/perfdhcp/tests/test_control_unittest.cc'''
 Let me just say that the trick with using in NamedTestControl is
 brilliant!

 getLocalLoopback(): Loopback interfaces should have
 IfaceMgr::Iface::flag_loopback_ set to true.

 matchRequestedOptions(): layed => laid

 unequalOctetPosition(): the comment for parameter random_size lacks
 parameter name.
 The comment for randomization range suggests that it is always 6. If that
 is the case, why not making is a fixed value? And it is not really needed
 anyway.

 If I understand the method correctly, in its current it will never return
 anything above 2. The first iteration will reduce the value to something
 in 0..255 range. The second iteration will set unequal_pos to 2 and break
 the loop. For that reason it doesn't work for 65537 and above (returns 2
 instead of 3).

 The code for this method is surprisingly convoluted. If I understand it
 correctly, this method should return number of bytes that have to be
 different to accommodate specified number of  clients. So what you really
 want is:

 log256(clients_num - 1);

 Here are 2 proposals to do this:
 {{{
     if (clients_num < 2) // degenerated case. There's nothing to randomize
         return 0;

     return 1 + log(clients_num - 1)/log(256);
 }}}
 It is very short, but has the disadvantage of using floating point
 arithmetic. Another code that is similar to the code under review would
 be:
 {{{
     if (!clients_num)
         return 0;
     clients_num--;

     int cnt = 0;
     while (clients_num) {
         clients_num >>= 8;
         ++cnt;
     }

     return cnt;
 }}}
 The advantage here is not using floating point calculations. As this is a
 test code, I personally prefer simplicity over performance to avoid
 possible bugs.

 On a related note, how does this method correlate to what is happening in
 generateDuid()? If you have 256 clients, those clients will have unique
 DUID only if the generateMacAddress() manages to allocate 256 unique
 numbers.

 I would like to see a test that generates 256 DUIDs and verifies that they
 are indeed unique. They should differ only on a single byte, right?

 testDuid(): The conditions checked in testDuid() are suspicious. Why the
 test passes if the total_dist is 0? It should only pass if all X DUIDs are
 different.

 Something along the lines:
 {{{
 Duid duid[clients_num];
 // fill in DUIDs
 for (i = 0; i < clients_num; ++i) {
     for (j = i + 1; j<clients_num; ++j) {
         if (duid[i] == duid[j]))
             FAIL();
     }
 }
 }}}

 The same comment applies to testMacAddress().

 testPkt4Exchange(): reponses => responses

 createOffcePkt4(), creatAdvertisePkt6(): doxygen descriptions are missing.

 TestControlTest.Options4,6: Those tests are generic to all options. Most
 of the test code is generic, only the call to registerOptionFactories() is
 specific to TestControl. You should consider splitting those tests and
 moving generic parts to src/lib/dhcp/ somewhere.

 TestControlTest.Options6: How does the code for requestes_options work? It
 uses matchRequestedOptions() that compares the requests on byte by byte
 basis. DHCPv6 options are  coded on 2 bytes. matchRequestdOptions6() is
 needed.

 matchRequestedOptions(): break in the loop should be removed. This will
 detect option code duplicates.

 TestControlTest.Options6: TODO should look like this /// @todo to be
 picked up by Doxygen TODO list generator.

 TestControlTest.PacketTemplates: The test should verify that the templates
 were read correctly.

 TestControlTest.RateControl: Checks for xchgs_num should be a bit more
 thorough. This is very broad subject, so I proposed to add TODO and don't
 spend too much time on it now.

 Ok. That's it. End of the review.

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