BIND 10 #1540: dhcp code refactor: Pkt6 and Options for DHCPv6
BIND 10 Development
do-not-reply at isc.org
Tue Feb 7 12:33:40 UTC 2012
#1540: dhcp code refactor: Pkt6 and Options for DHCPv6
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: | Status: reviewing
enhancement | Milestone: Sprint-
Priority: major | DHCP-20120206
Component: dhcp | Resolution:
Keywords: | Sensitive: 0
Defect Severity: N/A | Sub-Project: DHCP
Feature Depending on Ticket: | Estimated Difficulty: 0
Add Hours to Ticket: 0 | Total Hours: 0
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by stephen):
* owner: stephen => tomek
Comment:
'''General'''[[BR]]
The use of PktXPtr makes the code clearer, but we should also have a
ConstPktXPtr data type - a pointer to a const object. (This approach is
used elsewhere in BIND 10, e.g. RRsetPtr and ConstRRsetPtr.) Note that
this also applies to !OptionPtr as well.
Although this review will mention places where a ConstPktXPtr can be used,
those comments can be ignored for now: it is suggested that their
introduction be deferred to a separate ticket - introducing "const" is
likely to require a lot of changes.
'''src/bin/dhcp4/dhcp4_srv.{cc,h}'''[[BR]]
Dhcp4Srv::run() - query should be declared and initialised on one line.
Dhcp4Srv::run() - query could probably be a ConstPkt4Ptr, in which case a
number of the processXxxx methods need to be modified to accept a
ConstPkt4Ptr. (In fact, the argument to these is likely to be of data
type {{{const ConstPkt4Ptr&}}} - a const pointer to a const object.)
Dhcp4Srv::copyDefaultFields() - "question" argument should be {{{const
ConstPkt4Ptr&}}}.
Many methods - Pointers to options are created. It is likely that these
could be const options, i.e. !ConstOptionPtrs.
'''src/bin/dhcp4/dhcp6_srv.{cc,h}'''[[BR]]
Comments made above about const pointers apply.
Dhcp6Srv::run() - query should be declared and initialised on one line.
Dhcp6Srv::setServerID() - check the Ethernet type (you're not in a hotel
now :-)
Dhcp6Srv::setServerID() - for loop: spaces around binary operators please.
Dhcp6Srv::processRequest()/processSolicit() - why the blank line before
the creation of "reply" - the remainder of the processXxxx() methods do
not have it.
Dhcp6Srv::processRebind(): Creation of "reply" does not need to be split
across multiple lines.
'''src/bin/dhcp6/tests/dhcp6_srv_unittest.{cc,h}'''[[BR]]
Solicit_basic test - the "for" loop at the head of the function should
included braces around the (single) statement.
Solicit_basic test - in the initialization of "clientid", there should be
spaces around the "+".
'''src/lib/dhcp/iface_mgr.cc'''[[BR]]
IfaceMgr::send() - (V4 version) Shouldn't the argument have a data type of
Pkt4Ptr?
IfaceMgr::send() - (V4 version) a number of the local variables could be
declared closer to where they are first used: in the case of "cmsg" and
"result", the declaration could be combined with initialization.
IfaceMgr::send() - (V4 version) although the compiler apparently accepts
it for the purposes of calculating a sizeof(), it feels uncomfortable to
dereference pktinfo before initializing it. Instead of
"sizeof(*pktinfo)", "sizeof(in6_pktinfo)" seems safer. Or swap this
statement with the one before it?
IfaceMgr::send() - (V4 version) Should use "static_cast<in6_pktinfo*>"
(instead of the C-style cast) to set pktinfo.
IfaceMgr::send() - (V6 version) Shouldn't the argument have a data type of
Pkt6Ptr?
IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the data
for v.iov_base instead of the C-style cast.
IfaceMgr::send() - (V6 version) Remove the comment about ticket #1237.
Also, can the Linux-specific part be put into a separate function
(possibly in a separate file)? It would serve to isolate the Linux-
specific code from the rest of the function.
IfaceMgr::receive4() - The "while" loop looks awkward with two "++s"
increments. Converting to "for" loop and inverting the test of the
address family would simplify it. Also, as this appears to be separate
from the rest of the code, can't it be placed into a separate method?
IfaceMgr::receive4() - a lot of the declarations look similar to that in
other methods - is there anything that could be made common?
IfaceMgr::receive4() - "pkt" should have the data type Pkt4Ptr (and should
be declarated where it is initialized).
IfaceMgr::receive4() - return variable should be of type Pkt4Ptr.
IfaceMgr::receive4() - same comments made above concerning declaraing
variables closer to their use apply.
IfaceMgr::receive4() - no need for a blank line after calling recvmsg()
and the test on result.
IfaceMgr::receive4() - suggest place the Linux-specific code in a separate
function (in a separate file).
IfaceMgr::receive6() - most of the comments made for receive4() (including
the conversion of the loop over the sockets from "while" to "for", and
variable initialization) apply here.
IfaceMgr::receive6() - the "no interfaces detected" message should be made
only if "ifaces_.empty()" is true. Currently it can be thrown if no
interface has IPV6 capability (which is somewhat different).
IfaceMgr::receive6() - casts in the calls to setXxxxAddr should be
C++-style.
IfaceMgr::getSocket() - in the method signature, "const Pkt6&" appears to
be the more usual style than "Pkt6 const&".
IfaceMgr::getSocket() - (both versions) in the "for" loop, as the
conditions aren't too complicated, inverting and combining the conditions
in a single "if" test and placing the "return" in the body avoids the need
for "continue" statements.
'''src/lib/dhcp/libdhcp++.{cc,h}'''[[BR]]
unpackOptions6() - should not be a space between "end" and ")" in the
first "if" test.
unpackOptions4(): using pre-increments is generally better than post-
increments.
unpackOptions6()/unpackOptions4() - in the call to options.insert(), you
could use std::make_pair as a less-verbose way of creating a pair to
insert into "options".
!OptionFactoryRegister() - should be no space preceding the "*" in the
factory type. Spaces needed around "!=" in the "if" statements.
!OptionFactoryRegister() - throws an exception if the option type is
greater than 254 - although "end" option has a value of 255. If "end" can
never be passed to this method, there should be a comment explaining why.
'''src/lib/dhcp/option.h'''[[BR]]
Typedef of !OptionBufferPtr does not need spaces after "<" and before ">".
Typedef of !OptionCollection does not need to be split across two lines.
Also, no need for a space before the ">".
A comment should be added to the effect that it is assumed all over the
code that !OptionBuffer is a vector. (In several places an iterator over
!OptionBuffer is dereferenced and assumed to point to a set of contiguous
bytes that are interpreted as a data type such as uint16_t. If the
underlying !OptionBuffer data type were changed, this might not be true.)
General: revisiting the Option class, I was struck by the separation of
functions depending on "Universe". What is the argument against an Option
(abstract) base class with Option4 and Option6 derived classes?
'''src/lib/dhcp/option.cc'''[[BR]]
pack4() - since the constructor only rejects the V4 option type if type is
greater than 255, there is a possibility that the Option object could hold
an "end" option. In this case, pack4() will attempt to add a length byte
after it.
pack4() - should add a sanity check for option size when writing the
length byte for a V4 option - len() returns a uint16_t so could
potentially overflow the field.
pack() - "return" should have the argument enclosed in brackets.
getOption()/delOption() - argument type should be uint16_t, not "unsigned
short".
addOption() - consider use of make_pair().
'''src/lib/dhcp/option6_addrlst.cc'''[[BR]]
Remove comment "this wrapping is *ugly*..." ... unless you think it is
still ugly, in which case complete the comment :-)
Option6AddrLst::unpack() - use V6ADDRESS_LEN in the "if" statement instead
of 16. (More informative and consistent with use later on in the method.)
Option6AddrLst::unpack() - would prefer the result of "distance % 16" to
be explicitly compared against 0 (instead of using the implicit conversion
to bool): the check is whether the remainder is non-zero and an explicit
numerical check reinforces that.
'''src/lib/dhcp/option6_ia.cc'''[[BR]]
Option6IA::unpack() - there is a warning if the distance between the start
and end iterators is less than 12; what if it is greater than 12? Also,
could this value be a symbolic constant (c.f. OPTION6_IAADR_LEN in
Option6IAAddr::unpack())
'''src/lib/dhcp/pkt4.h'''[[BR]]
The comments for the member variable data_ use double slashes instead of
the triple slashes required by Doxygen and used for other member
variables.
'''src/lib/dhcp/pkt6.h'''[[BR]]
getBuffer() header - should say "This buffer is only valid while the Pkt4
object is valid."
len() header - the header comment is not clear. The "brief" explanation
is that it returns the size of the packet, but the text implies that it
returns the length of the options fields - and treats the header as one of
them. Also, why does options_ have to be set - if it is clear doesn't it
mean that the packet has no options?
A consistent style should be used for the inline expansions of one-line
methods. In some cases the expansion is on the same line as the
declaration: in other cases it is split across three lines.
Does ifindex_ deserve the "brief" line in its explanatory comments?
There are apparently two ways of identifying the interface - the name and
the index. Are these truly independent or is there a unified way of
accessing the relevant interface?
No need for spaces after the opening parenthesis and beflore the closing
parenthesis in some EXPECT_EQ checks.
'''src/lib/dhcp/tests/iface_mgr_unittest.cc'''[[BR]]
sendReceive6 test - the loop setting the dummy payload should have spaces
around binary operators.
'''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
packOptions4 and packOptions6 tests - no need for the space before the ">"
in the "opts.insert" lines.
packOptions6 test - the EXPECT_NO_THROW test can be on a single line.
No need for spaces after the opening parenthesis and before the closing
parenthesis in some EXPECT_EQ checks.
'''src/lib/dhcp/tests/option6_addrlst_unittest.cc'''[[BR]]
No need for spaces after the opening parenthesis and before the closing
parenthesis in some EXPECT_EQ checks.
'''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
suboptions_pack test - "sub1" declaration does not need to sbe split
across two lines.
suboptions_unpack test - not sure what the cryptic comment "48 bytes"
means.
suboptions_unpack test - in the data declaration, binary operators should
have a space around them.
'''src/lib/dhcp/tests/option6_iaaddr_unittest.cc'''[[BR]]
basic test - in the initialization loop, binary operators should have a
space around them.
'''src/lib/dhcp/tests/option_unittest.cc'''[[BR]]
No need for spaces after the opening parenthesis and before the closing
parenthesis in some EXPECT_EQ checks.
'''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
constructor test - No space before the "*" in the declaration of pkt1.
No need for spaces after the opening parenthesis and before the closing
parenthesis in some EXPECT_EQ checks.
capture() - C++ style is to put the "*" after the return type, not before
the function name.
capture() - can we line up the assignment of data elements? However, it
would be clearer to statically initialize the array.
packUnpack test - "3*Option" - should be spaces around the "*".
addGetDelOptions test - C++ style is to put the "*" after the return
type, not before the function name (declaration of "parent".) Also should
be no space before the closing parenthesis.
'''src/lib/util/buffer.h'''[[BR]]
The change appears OK, but tests/buffer_unittest.cc should be extended to
check that the constructor using a vector works.
'''!ChangeLog'''[[BR]]
Text for the entry is OK.
--
Ticket URL: <http://bind10.isc.org/ticket/1540#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list