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

Comment (by marcin):

 Replying to [comment:17 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.

 Ok. I added some comment in class description.

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

 I renamed per your suggestion.

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

 Added @todo.

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

 Dropped.

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

 I don't think it is going to be larger. I think you may be right. I will
 create another ticket for this.

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

 For now I removed confusing const and I use index directly with comment.
 In the further implementation of perfdhcp we will be adding support for
 other message types (e.g. RENEW). This will be the opportunity to re-think
 the mapping between message type and index of vector that holds template
 buffers and many other "indexed" command options.
 >
 >
 > > > 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.

 Created #2250.

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

 Created ticket #2251.

 >
 > 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:21>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development