BIND 10 #1186: libdhcp implementation - DHCPv6 part

[BIND 10 Development]

#1186: libdhcp implementation - DHCPv6 part
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  stephen
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  DHCP 2011
                   Priority:  major  |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:  878    |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by stephen):

 = Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 1 =

 As a large number of files have been altered, the review is in two parts.
 The first part concerns the files in src/bin/dhcp6 (and associated tests).

 Notes:
 * It has been noted that there is a TODO to convert the files to use BIND
 10 logging.  Therefore no comments have been made about this.

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 The #include of 'io_address.h' should be {{{#include
 "asiolink/io_address.h"}}}


 In Dhcpv6Srv::run(), there is the following construct:
 {{{
 if (rsp != boost::shared_ptr<Pkt6>()) {
 }}}
 To check whether rsp holds a valid pointer, the simpler {{{if (rsp) {}}}
 can be used.

 getServerID(): Presumably there is a getServerID() as a counterpart to
 setServerID()?  (If not, why not access serverid_ directly?) If so, it
 would make sense to have getServerID() return a reference to serverid_ to
 save on the copying of the shared pointer.  Also, getServerID() is
 sufficiently trivial that it can be coded inline in the header.

 In setServerID(), need to check that srvid[3] should really be 6.

 Does setServerID() need to return anything?  If the code fails to set the
 server ID, would an exception be a better way of signalling it?

 in processSolicit(), the code contains two declarations of the form
 {{{Option * tmp}}}.  This should be {{{Option* tmp}}} (as the current
 layout looks at first glance like a multiplication).  Also, should be no
 space between the ">" and "(" in the dynamic_cast statement.

 Should include a 'TODO:' comment about changing them in the code for the
 hard-wired times and address.

 '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 To avoid a copy of the input variable, the {{{solicit}}} argument to many
 of the methods should be passed by reference.

 All methods in Dhcpv6Srv should have a Doxygen header.

 The class Dhcp6Srv should have a header explaining the purpose of the
 class.

 The convention in BIND 10 is to use nested namespaces rooted at "isc" for
 different components.  Suggest that the class be put into isc::dhcp.

 Declaring the test class as a "friend" in the production code is unusual
 (in BIND 10) but valid.  This should be discussed on bind10-dev as to
 whether we want to make this a standard.  However, if the test class is a
 friend, why are methods protected?

 To prevent copying, the class should be derived from
 {{{boost::noncopyable}}}. (In fairness, although this was decided back in
 February, is has only today been included into the coding guidelines.)

 All member variables should include a Doxygen comment stating their
 purpose.

 '''src/bin/dhcp6/iface_mgr.cc'''
 (A few things that may have been picked up in the review of #868 but
 caught my eye when reviewing these changes.)

 instance(): the {{{if}}} statement should contain braces, even if the
 contents are only one statement.

 Iface constructor.  For setting the mac_ array to zero, memset is OK
 (although some authorities prefer std::fill).  However the length should
 be {{{sizeof(mac_)}}} and not a hard-coded 20.

 getFillName(): {{{return (name_ + "/");}}} is simpler than creating a
 temporary ostringstream object.

 getPlainMac(): I think that setting the fill, width and radix once outside
 the {{{for}}} loop should be OK. (Also, if mac_ is declared as
 {{{uint8_t}}}, there should be no need for the cast to an {{{int}}} when
 output each element of it.)

 In !IfaceMgr constructor, {{{control_buf_}}} could be declared as a
 {{{boost::scoped_ptr<>}}} to provide automatic destruction in the case
 that anything else in the constructor throws an exception. (Would then
 remove the deallocation of {{{control_buf_}}} from the destructor.)

 Header for openSocket.  The BIND 10 convention is to place the headers in
 the .h file.  Also, the first line of the header contains a typo "nad"
 instead of "and".

 receive(): without checking where this is called from, would not throwing
 an exception (instead of returning an empty packet) be a better way of
 signalling that the receive has failed?

 '''src/bin/dhcp6/iface_mgr.h'''
 See comment in dhcp6_srv.h about namespaces.

 All methods - including those in the contained Iface struct - should have
 Doxygen headers.

 Remove the note that {{{struct Iface}}} could be a class as well - in this
 context, a {{{struct}}} is fine.

 {{{mac_}}} is best declared as {{{uint8_t}}}. (It is not being used as a
 {{{char}}}.)

 Should remove the final comment in {{{struct Iface}}} as there is no next
 field.

 Suggest grouping the variables and the method declarations in {{{struct
 Iface}}} instead of putting the latter in the middle of the former.

 {{{Iface * getIface(...)}}} should be {{{Iface* getIface(...)}}} (looks
 like a multiplication).  This appliease to other similar declarations in
 the file.

 send() should take a reference to a shared pointer as its argument.  Also,
 if the argument is unchanged, it should be {{{const}}}.

 All member variables should include a Doxygen comment stating their
 purpose.

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 Basic test.  The comment says that "an attempt to bind this socket will
 fail" yet the test is {{{EXPECT_NO_THROW}}} - which indicates that no
 exception will be thrown when the test succeeds.

 Should be a space after the comma in the {{{TEST_F}}} line for
 Solicit_basic.

 Inconsistency in declaration: both {{{sol}}} and {{{ia}}} are of the form
 {{{boost::shared_ptr<xyz>}}}, yet the former is initialized using the
 construct "ptr x = ptr(new Xyz(...))" and the latter "ptr x(new
 Xyz(...))".

 Would help to include a comment explaining what options are being set in
 the packet.

 See above for comments on the testing whether a shared pointer points to a
 null object.

 In the Solicit_basic test, the contents of the ASSERT_TRUE macros can
 simply be the shared_ptr in question - the implicit conversion to bool
 will return true if the pointed is non-null.


 '''src/bin/dhcp6/tests/iface_mgr_unittest.cc'''[[BR]]
 In the test loDetect, can you guarantee that the default directory when
 running the test is writeable and that "interfaces.txt" will be created?
 In fact, removing "interfaces.txt" and instead using an environment
 variable might be safer.  (Which will require an update to the !IfaceMgr
 code.)

 In the "sockets" test, the socket numbers are perhaps best put as
 constants at the head of the file (in case they need changing at some time
 in the future).

 In test sendReceive, the {{{ASSERT_TRUE}}} condition can be just
 {{{rcvPkt}}} (see above).

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