BIND 10 #1186: libdhcp implementation - DHCPv6 part

[BIND 10 Development]

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

Comment (by stephen):

 I've re-reviewed the code and have entered my comments as replies to each
 of the updates you made to the ticket.


 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 >> To check whether rsp holds a valid pointer, the simpler "if (rsp) {"
 can be used.
 > I think that is against BIND9 conding standards that is also used in
 DHCP...
 BIND 9 is written in C, so such a check is a test on zero/non-zero (i.e.
 false/true).  BIND 10 uses C++, and the C++0x standard mentions
 {{{operator unspecified-bool-type() const;}}} which provides a conversion
 of an object to bool (and so is the correct type for evaluation in a
 boolean context).  We are not using C++0x, but Boost provides such a
 conversion operation and this conversion is used when checking if a
 shared_ptr is empty.


 '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 getServerID(): this returns a reference to a shared_ptr within Dhcpv6Srv.
 But that reference is only valid while Dhcpv6Srv remains in existence.
 Unless there is a guarantee that Dhcpv6Srv remains in existence while the
 reference is valid, it should return a shared_ptr by value.  (But if
 Dhcpv6Srv guaranteed to remain in existence, there is no need for
 serverid_ to be a shared_ptr - it could be a scoped_ptr (avoiding the
 overhead of reference counting) and Dhcpv6Srv could return a "bare"
 pointer.)

 If a class derived from boost::noncopyable, there is no need to define the
 copy constructor and assignment operator.

 It is not the BIND 10 standard layout to indent code in namespaces.

 All Doxygen headers should describe their arguments.

 '''src/bin/dhcp6/iface_mgr.cc'''[[BR]]
 >> getFillName(): return (name_ + "/"); is simpler than creating a
 temporary ostringstream object.
 > No, it is not. It is not possible to add integer to strings.
 You're right, I overlooked the ifindex_ variable.

 getPlainMac(): Should be spaces around binary operators in the "for" and
 "if" statements.

 >> In IfaceMgr constructor, control_buf_ could be declared as a
 boost::scoped_ptr<> ...
 > scoped_ptr is not the proper type here ...
 My fault, use boost::scoped_array<> instead.  (This is better than
 shared_ptr<> if there is no need to shared the underlying buffer - it
 avoids the need to manipulate reference counters.)

 Can remove the method headers from the .cc file if they are already in the
 .h file. The typo "nad" has been moved to the .h file :-)

 '''src/bin/dhcp6/iface_mgr.h'''[[BR]]
 Same comment as before about the indentation with respect to namespaces.

 >> send() should take a reference to a shared pointer as its argument.
 Also, if the argument is unchanged, it should be const.
 > I prefer to keep it as separate copy of shared pointer. We may decide to
 use async send sometime later.
 If async send is used, we can always copy the pointer if we need to.
 Ignoring possible compiler optimisation, this would mean two copies: one
 copy when the shared_ptr is passed by value to the method, and one copy of
 the method's argument into the location where the pointer is stored for
 the duration of the asynchronous call.  Passing the shared_ptr by
 reference eliminates the first copy.

 '''src/bin/dhcp6/tests/iface_mgr_unittest.cc'''[[BR]]
 > I politely refuse to implement this
 :-) As the code is only temporary, leave it for now.

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