BIND 10 #1959: Implement perfdhcp control logic

[BIND 10 Development]

#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  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      |
-------------------------------------+-------------------------------------

Comment (by marcin):

 Replying to [comment:9 tomek]:
 > This review is based on commit-id
 6efd035f6178974adb48ace599f6d2335c0afd7e (and all previous changes on
 trac1959 that are not present on master).
 >
 > Generic comment: CommandOptions class should be renamed to
 CommandParameters or CommandSwitches. Option has very specific meaning in
 DHCP context - something different that CommandOptions convey. This
 applies to many uses of that class, e.g. in sendRequest6() and similar
 methods, "options" object is really for DHCP options. It should be named
 "parameters" (or "params" if you prefer short names).
 >

 I am not fully convinced but I think we can do this as a part of code
 cleanup (ticket #1960).

 > '''tests/tools/perfdhcp/test_control.h'''
 >
 > Is the TestControlSocket class really needed? Why can't you use
 IfaceMgr::SocketInfo? It holds all the data, except interface information.
 If you need interface info, perhaps TestControlSocket could be derived
 from IfaceMgr::SocketInfo? Its definition would be simpler.

 I prefer classes with read-only access to variables and that's why I
 started implementing my own class here. However I admit that it is cleaner
 to reuse the existing code so I now derive from SocketInfo.

 >
 > Private default TestControlSocket constructor is not need. Since there
 is another constructor explicitly defined, the implicit default
 constructor with public access will not be created.

 Ok, removed.

 >
 > ~TestControlSocket(): There's TODO outstanding there - to close only the
 socket represented by this object, not all sockets in the whole IfaceMgr.
 Fortunately, that is very easy to do. The following code should do the
 trick:
 >
 > {{{
 > IfaceMgr::Iface* iface = IfaceMgr::instance().getIface(ifindex_);
 > if (iface) {
 >     iface->delSocket(socket_);
 > }
 > }}}

 Ok, I changed the destructor as suggested.

 >
 > Also, TODO should be written as \TODO or @TODO to mark it up on Doxygen
 todo list.

 Fixed.

 >
 > HW_ETHER_LEN constant: it is ok to keep it as 6 bytes for now, but we
 will need to extend the code one day to check if longer MAC addresses
 cause problems. The longest link-layer address I'm aware of is used in
 Infiniband. It is 20 bytes long. A simple @TODO will do the trick for now.

 I added todo statement.

 >
 > TestControlSocket keeps both interface name (iface_) and its index
 (ifindex_). Keeping both is redundant. If both are kept for the (minimal)
 performance reasons, it should be commented. Otherwise it would be simpler
 to keep a pointer to the interface object and use its getName() and
 getIndex(). The added benefit is an access to getFullName() method that is
 useful for logging purposes (it returns both in a readable format, e.g.
 "eth0/2"). It is also more extensive, if you ever decide that more
 information is needed in the future.
 >
 I left ifindex_ and now client class uses IfaceMgr::getIface to get the
 interface name.

 > The comment just after protected: It should be moved above protected.
 The general approach is that the comment appears before the piece of code
 that is being commented. It is a good point about using friends. I tried
 to use that approach in one ticket, but I later removed it to follow
 whatever others do. It might be useful to write a question about it to
 bind10-dev asking if people are ok with using "friends" approach. As the
 discussion on bind10-dev may be lengthy, carry on with this ticket. If the
 discussion reaches a conclusion that fried is the way to go, we will have
 lots of places to change the code anyway.

 Comment moved. That's going to be hard job to change all classes to use
 "friend" approach. Maybe this could be applied to new classes only.

 >
 > checkExitConditions() description: fulfiled => fulfilled.

 Corrected.

 >
 > factoryElapsedTime6(): buf parameter should be described more throughly
 - does it contain the whole option (including option header, 6 bytes in
 total) or just content of the option(2 bytes)? This also applies to many
 other factory methods.

 Comment added.

 >
 > This factory is supposed to create *DHCPv6* option. Why does it take
 universe parameter? If this parameter is necessary because of factory
 registration, then it should be commented on (and definitely not say V4 or
 V6 in the parameter description).

 The parameter is required for registration purpose. With current approach
 to register factory functions many parameters like this will be ignored. I
 updated comments at parameters which are ignored that they are ignored.

 >
 > factoryIana6(), factoryOptionRequestOption6(): Why those methods end
 with 6? There are no v4 equivalents of those options, so 6 is redundant in
 the name.

 The naming convention that I am following is use
 - factoryABC6 for functions that are used to create DHCPv6 options only
 - factoryDEF4 for functions that are used to create DHCPv4 options only
 - factoryGHI for functions that can be used for either DHCPv4 or v6
 options.

 The function names correspond to this. Although it may seem a little
 redundant the option name indicates which factory function is for which IP
 mode (for somebody who does not recognize it from the option name). I
 insist that we leave it as it is.

 >
 > Many factory methods throw exceptions if specified buffer is of invalid
 size. That should be reflected in the description with \throw.

 I belive it is. Can you point which one that throws exception is left
 without \throw?

 >
 > generateDuid(): typo in the description: clinets => clients (x2).
 uint8_t passed by reference, because this method sets the randomized
 value. It should be noted somewhere in the description that this is an
 output parameter and its initial value is ignored.
 >
 Comment updated.

 > generateMacAddress(): See generateDuid() comments.

 Comment updated.

 >
 > generateTransId(): should mention if the transid is 32bit value (DHCPv4)
 or (DHCPv6).

 Comment updated.
 >
 > getNextExchangesNum(): immediatelly => immediately.

 Fixed.
 >
 > getTemplateBuffer(): Wouldn't it be more correct to throw exception,
 when user requested non-existing buffer?
 It now throws exception. It does not change anything but it is more
 consistent with other functions at least.
 >
 > initTemplateFiles(): Where is the list of template files specified? It
 should briefly comment on it, as that info is not passed as parameter.

 I am not sure if I understand your point. The list of template files is
 given from command line. I updated comments to reflect this.

 >
 > openSocket(): Why do you need to set multicast() flag on the socket for
 DHCPv6? This flag is only needed for receiving multicast traffic. A client
 never never does that. With v4, things are a bit different, as the server
 response may be sent to broadcast address if certain conditions are met.

 The logic to set multicast option on the socket for multicast remote
 addresses is taken from previous perfdhcp version (implemented by
 Francis). I will double check if I can safely remove this code during the
 course of #1960.

 >
 > The comment for open socket should also mention if the sockets opened
 are registered in IfaceMgr or not. I presume they are, otherwise IfaceMgr
 wouldn't be able to use them to send and receive traffic.

 I added this comment but from the caller perspective it is of a less
 importance. This is more TestControl class internals that rely on this
 fact.

 >
 > There are many articles missing in the comments in test_control.h. As
 I've never learned to handle them correctly, I will not bother to guess
 when they're needed.

 I added one or two articles. I haven't found "many".

 >
 > receivePacket4(), receivePacket6(): It looks more natural when received
 packet is returned by the method, so unless there are specific reasons to
 do otherwise, I would prefer the prototype to be:
 > {{{
 > const dhcp::Pkt4Ptr receivePacket4(const TestControlSocket& socket);
 > }}}

 The packet is NOT returned here at all. It is the input parameter. The
 actual packet reception from the wire is done elsewhere. The
 receivePacketX functions simply do processing on received packet (e.g.
 update statistics on StatsMgr, calculate RTT etc.).
 >
 > On the other hand, if you want to keep it similar to sendXXX4/6()
 methods, I won't object, but please add [in] or [out] to the parameters
 passed by reference.

 I put the [in,out] where applicable. I did not put this where I pass const
 reference or where I don't pass reference at all.

 >
 > Also, both methods should throw if someone tries to use v4 socket to
 receive v6 packet (and vice versa). That would be easy to check if
 IfaceMgr::SocketInfo structures were used. If such a check is not desired,
 because it is already checked in receivePackets(), a warning comment
 should be added.
 >
 The code checks mode of operation (v4 or v6) on the very beginning and
 uses receive4 or receive6 methods accordingly. If the code is ok (I belive
 it is) it should never get v6 packet on v4 socket etc. I added warnings to
 receivePacketX functions that they don't do these sanity checks.

 > sendDiscover4(), sendSolicit6(), sendRequest4() and many similar:
 > - for parameters passed by reference, please specify the direction
 ([in], [out] or [in,out]). See
 http://www.stack.nl/~dimitri/doxygen/docblocks.html (search for [in]).
 Why would I need it here? I pass all variables by const reference. It is
 clear that they are [in] params.
 > - please supplement the descriptions with the information that sent
 packets are stored somewhere (in stats_mgr).
 Comment added?
 >
 > testDiags(): Find of => Find if

 Corrected.
 >
 > byte2Hex(), vector2Hex(): These are very generic methods. Similar
 methods exist in src/lib/util/encode/hex.h (and other headers in that
 directory). If there isn't one already defined, please move them there.

 My methods in their shape do not fit to src/lib/util/encode/hex.h or any
 other file there. In the same time, methods provided there do not provide
 separators. Trying to move my code there might be tricky and I prefer
 postponing it for now.

 >
 > getElapsedTime(). This method is sufficiently complex to move its body
 to .cc file. Also, wouldn't it be more appropriate to throw exception (or
 return 0xffffffff) when time is not specified? The reason for returning
 0xffffffff is to mark bogus packet exchanges. (there's definitely
 something wrong if packet was not timestamped).

 Moved to .cc file and exception is now thrown.

 >
 > factoryIana6(): The description should be very specific that the option-
 buffer should point to IA_NA *sub-options*. The code should have @todo
 somewhere stating that IAID, T1 and T2 are hardcoded. Perhaps .cc file is
 better for such @todo comment.

 Added comment.

 >
 > '''tests/tools/perfdhcp/test_control.cc'''
 >
 > General comment: Adding "using namespace XYZ;" would make many
 expressions shorter.

 But in many cases having namespaces is preferred because it clearly
 indicates which module the methods belong to.

 >
 > checkExitConditions():148. The check should be
 > {{{
 > if (options.getNumRequests().size() > 1) {
 > }}}
 > as a future compatibility for cases, when RENEW support will be added.

 Ok. I added this but if we ever enable renew support it will have to be
 modified anyway.

 >
 > The check for reaching maximum drops percentage can be skipped, if total
 drop is 0. A simple if around the whole section should do the trick.

 Does it really improve any performance here? I don't want this function to
 grow any bigger than necessary so I dropped this suggestion.

 >
 > factoryElapsedTime6(): As a sanity check we should probably verify that
 universe is v6 and option type is D6O_EeLAPSED_TIME. The same applies to
 other factory functions.

 The factory functions now ignore universe.

 >
 > factoryOptionRequestOption6(): The comment in .h file states that the
 buf should be empty and is ignored. There should be a check whether it is
 indeed empty. Or even better, it should add name server and domain search
 as it does now only if the buffer is empty, otherwise treat the buffer as
 list of options to request. You may mark this as @todo for a future work
 if you don't want to implement it now.

 I changed the comment. The buffer is ignored.

 >
 > factoryRequestList4(): See factoryOptionRequestOption6().

 Same here.
 >
 > generateMacAddress(),generate: The comment states that the random number
 must be in range 0..clients_num, but the code will result in
 0..clinets_num - 1. Either the code or comment should be updated.

 I updated the comment.
 >
 > generateDuid(): please use STL copy function to copy mac_addr to duid,
 instead of copying byte by byte.

 Change to std::copy.

 >
 > getNextExchangesNum(): If you want to make sure that at least one packet
 goes out, you should use
 > {{{
 > if (due_exchanges == 0) {
 >   due_exchanges == 1;
 > }
 > }}}
 > or any fancy way of doing exactly that. Simply increasing it by one may
 overshoot and in the worst case double due_exchanges value.

 Ok. I added this condition.

 >
 > getRcvdPacketsNum(), getSentPacketsNum(), getTemplateBuffer(): Some
 people would say that the second return should be in else clause. I
 personally like the style you used as it keeps the code less indented.

 So we are on the same side. Great :-)

 >
 > initPacketTemplates(): ++it does not have to be on a separate line.
 >
 It is not anymore.

 > There are many cases were the code for stats_mgr 4 and 6 are repeated.
 Have you considered making stats_mgr4 and stats_mgr6 derived from a common
 ancestor and using a single pointer? That would simplify the code a lot.
 o
 This sounds reasonably. Initially I did not think that there will be so
 many distinct invocations to StatsMgr<Pkt4> and StatsMgr<Pkt6>. In the
 future we may reconsider this but not as a part of this ticket. I will
 take a look at this as a part of #1960.

 >
 > openSocket(): Please use DHCP4_{SERVER|CLIENT}_PORT. I have no idea why
 68 is "wrong" there. It should work.

 This will be handled in #1960.

 >
 > run(): Diagnostics is => Diagnostics are

 Fixed.

 >
 > When options.isSeeded() is called you seed PRNG with the specified seed.
 Otherwise you should seed it with current time (or if it is already seeded
 somewhere, add a comment to point out specific location).

 I now seed from current time.

 >
 > ourselfs => ourselves

 Fixed.

 >
 > template_idx is always zero. Are there any cases when additional
 templates may be used? If not, please add an appropriate comment.

 The reason to define the constant is that everybody knows that 0 is
 template index. I do another constant when I get the template for request
 packet. I just avoid using unnamed '0's in the code. I added some comment
 there.

 >
 > Why "interrupted" is printed when 'e' flag is set? It seems there may be
 other reasons to exit the main loop.

 The other reasons (like fatal) error are also printed. Fatal error for
 example is printed in main.cc.


 >
 > sendRequest4(): What happens if there is on server-id in the ADVERTISE
 message and the exception is thrown? Perhaps we should let the test
 continue, but increase some form of "RFC3315 violations" counter. The run
 will ultimately fail, but it is often interesting not abort after first
 fail, but let the server continue its work.

 This will be a part of #1960.

 >
 > "Set elapsed time" needs one more indent space.

 Fixed.
 >
 > sendRequest6(): Elapsed time is calculated wrong. In DHCPv4 secs field
 is specified in number of seconds since address acquisition has started.
 However, in DHCPv6 it specifies how  much time elapsed since client
 initiated *current* transaction (i.e. for REQUEST the time is measured
 from first transmission of REQUEST message, not first SOLICIT).

 Ok. I now set elapsed time == 0 for DHCPv6 requests.

 >
 > There is no check if received ADVERTISE is sane or not. First, the code
 should check if there is STATUS_CODE option in the message with non-zero
 status. Then it should check for non-zero STATUS_CODE opion in the IA_NA
 option. Finally it should check if there's IAADDR option in IA_NA. If you
 don't want to implement this now, just adding appropriate @TODO will be
 sufficient for now.

 We will need some other ticket for this to guarantee robustness of this
 code but also in many other places.

 >
 > updateSendDue(): Is the code really that fast that it usually send out
 packeted within microsecond of when it was supposed to be sent? I would
 add a little delta when comparing now and send_due_ to avoid cases when
 things are happening at the boundary between n-th and n-th microsecond.
 Some time later, this could be made a configurable parameter ("allowed
 send delay" or "send delay tolerance"), but it is ok to add @todo for now
 (if you think it is a good idea to implement such a parameter).

 In fact, I have checked that it is often that we exceed due time and
 latesend counter is increased. First I would like to implement milisecond
 timeout in ifaceMgr as it will change a lot the timings in main loop.
 After that I will take a look how it affects the section you're poiting
 at.

 >
 > This concludes test_control.{cc|h}. I will continue looking at the
 remainder of the changes tomorrow.

 Thanks for this part of review.

 >
 > Note to self: the review will resume from diff line 3892.

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