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 tomek):

 Replying to [comment:11 marcin]:
 > > 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.
 That is a reasonable approach. Nevertheless, you should ask about the
 concept on bind10-dev.

 > 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.
 Sounds reasonable. Please add a comment about that naming scheme.

 > > 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?
 I thought that factory methods create specialized objects, e.g.
 factoryIana6 creates instances of Option6IA, not Option classes. Since
 they are using generic class, \throw are not needed.

 > > 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.
 Thanks. I see that the method is now called initPacketTemplates(). I like
 the new name better.

 > > 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".
 Here's an example (a comment to the openSocket() method):
 {{{
     /// Method opens socket and binds it to local address. Function will
     /// use either interface name, local address or server address
     /// to create a socket, depending on what is available (specified
     /// from the command line). If socket can't be created for any
     /// reason, exception is thrown.
 }}}
 I think it should be:
 opens socket => opens a socket
 to local address => to the local address
 If socket can't => If the socket can't
 exception is thrown => an exception is thrown

 Anyway, I don't think it is productive to spend much time on it. It may
 sound a bit rough for native speakers, but they will understand it.

 > > 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.).
 Uh oh. So receivePacket4 does not receive a packet? So rename it!
 processReceivedPacket4 sounds reasonable...

 > > 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.
 Ok. A simple @todo will do the trick for now.

 > > 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.
 I'm not sure how many times per second it is called. It was only a
 suggestion to consider, so it is ok to drop it.

 > 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.
 1960 seems to be growing bigger and bigger. Will it be ultimately larger
 than #1959? Perhaps the first thing to do in 1960 would be to split it
 into smaller chunks?

 > > 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.
 This should be a const static member of the class then.


 > > 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.
 Yes. Please create one.

 > > 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.
 Ok. That seems like a worthy candidate for a separate ticket. Please
 create one with a pointer to this discussion.

 I have removed many comments that you agreed with or just marked as one.
 It will make reading this at least a bit easier.

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