BIND 10 #1959: Implement perfdhcp control logic
BIND 10 Development
do-not-reply at isc.org
Mon Sep 3 18:27:52 UTC 2012
#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):
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).
'''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.
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.
~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_);
}
}}}
Also, TODO should be written as \TODO or @TODO to mark it up on Doxygen
todo list.
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.
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.
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.
checkExitConditions() description: fulfiled => fulfilled.
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.
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).
factoryIana6(), factoryOptionRequestOption6(): Why those methods end with
6? There are no v4 equivalents of those options, so 6 is redundant in the
name.
Many factory methods throw exceptions if specified buffer is of invalid
size. That should be reflected in the description with \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.
generateMacAddress(): See generateDuid() comments.
generateTransId(): should mention if the transid is 32bit value (DHCPv4)
or (DHCPv6).
getNextExchangesNum(): immediatelly => immediately.
getTemplateBuffer(): Wouldn't it be more correct to throw exception, when
user requested non-existing buffer?
initTemplateFiles(): Where is the list of template files specified? It
should briefly comment on it, as that info is not passed as parameter.
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 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.
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.
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);
}}}
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.
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.
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]).
- please supplement the descriptions with the information that sent
packets are stored somewhere (in stats_mgr).
testDiags(): Find of => Find if
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.
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).
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.
'''tests/tools/perfdhcp/test_control.cc'''
General comment: Adding "using namespace XYZ;" would make many expressions
shorter.
checkExitConditions():148. The check should be
{{{
if (options.getNumRequests().size() > 1) {
}}}
as a future compatibility for cases, when RENEW support will be added.
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.
factoryElapsedTime6(): As a sanity check we should probably verify that
universe is v6 and option type is D6O_ELAPSED_TIME. The same applies to
other factory functions.
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.
factoryRequestList4(): See factoryOptionRequestOption6().
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.
generateDuid(): please use STL copy function to copy mac_addr to duid,
instead of copying byte by byte.
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.
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.
initPacketTemplates(): ++it does not have to be on a separate line.
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.
openSocket(): Please use DHCP4_{SERVER|CLIENT}_PORT. I have no idea why 68
is "wrong" there. It should work.
run(): Diagnostics is => Diagnostics are
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).
ourselfs => ourselves
template_idx is always zero. Are there any cases when additional templates
may be used? If not, please add an appropriate comment.
Why "interrupted" is printed when 'e' flag is set? It seems there may be
other reasons to exit the main loop.
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.
"Set elapsed time" needs one more indent space.
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).
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.
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).
This concludes test_control.{cc|h}. I will continue looking at the
remainder of the changes tomorrow.
Note to self: the review will resume from diff line 3892.
--
Ticket URL: <http://bind10.isc.org/ticket/1959#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list