BIND 10 #1186: libdhcp implementation - DHCPv6 part

[BIND 10 Development]

#1186: libdhcp implementation - DHCPv6 part
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111026
                  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 tomek):

 Replying to [comment:13 stephen]:
 > >> 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.
 Ok. I think the code uses simple "if (pointer) {" code. If I missed
 anything, please do let me know.

 > '''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.)
 Fixed.

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

 > It is not the BIND 10 standard layout to indent code in namespaces.
 Fixed for dhcp6_srv.h and iface_mgr.h. Code in src/lib/dhcp seems to
 follow this guideline already. Also note that there is nothing in
 CodingGuidelines about indent (or lack of thereof) regarding namespaces.
 I'll ask on jabber and update CodingGuidelines if I get approval.

 > All Doxygen headers should describe their arguments.
 Added descriptions for missing arguments.

 > '''src/bin/dhcp6/iface_mgr.cc'''[[BR]]
 > getPlainMac(): Should be spaces around binary operators in the "for" and
 "if" statements.
 Done.

 > >> 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.)
 I did as you asked, but I don't like it. We gained nothing (this buffer
 was freed correctly), but code is more complicated now (need to use
 &control_buf[0] instead of simpler control_buf_) and is more difficult to
 debug (gcc can't display scoped_array<char> as easily as char *). That is
 unfortunate, as code around sendmsg and recvmsg are murky enough.

 > 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 :-)
 Redundant headers removed.

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

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

 Commit-ids: d8d0a731ce43167d98b7c7e855b8a31b9deacdfb,
 cb59e65da8d1c9498d1dc8845e277567814fe400 and
 a7605f27d5575c6fbeb325705220039936c81e53.

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