BIND 10 #1540: dhcp code refactor: Pkt6 and Options for DHCPv6

[BIND 10 Development]

#1540: dhcp code refactor: Pkt6 and Options for DHCPv6
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:         |  DHCP-20120319
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:14 stephen]:
 > Reviewed commit 485f7966f583f54cf2694f054de6accea9c19364
 >
 > I've pushed some small changes.
 Th
 >
 > '''src/bin/dhcp4/dhcp6_srv.cc'''[[BR]]
 > Dhcp6Srv::setServerID() - should use the isRangeZero() function added as
 part of this set of changes.
 Fixed.

 > '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 > Minor formatting changes have been made.
 >
 > openSockets4(): Does this not leak sockets?  "sock" is a local variable
 and appears to be discarded after receiving the value returned by
 openSocket(); it is not closed.
 No. Socket descriptor is returned for informational purposes only.
 openSockets4() calls openSocket4() that adds SocketInfo structure to
 appropriate interface. See
 {{{
 iface.addSocket(info);
 }}}
 in openSocket4(). This is a safety feature. Method that creates a socket
 stores information about it, so there is no way to leak sockets.

 Perhaps openSocket4() could be modified to return the whole structure, but
 the result would be the same. openSockets4() would just log that specific
 socket was create and would throw away the structure.

 > (Perhaps we should consider a "!SocketPtr" object - a shared_ptr to a
 socket that would close the socket when the last pointer reference it was
 destroyed?)
 >
 > openSockets6(): The logic here is very similar to openSockets4 and the
 common code could be abstracted into a separate method.
 See above. The same rule applies to v6. Note that structures are shared.
 We have a uniform structure to represent v4/v6/TCP/UDP sockets. This will
 come in handy, when we start supporting fancier features, like failover or
 bulk leasequery (works over TCP).

 > >> 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.
 > > Done.
 > Did you check that change in? "struct msghdr" and "struct iovec v" are
 still defined at the head of the function.
 Yes, I did. See commit 4fd6efcde0f9cb8d7d1513530324a389e32a978d (parent of
 the commit you are reviewing). cmsg and result are now in
 iface_mgr_linux.c. It is true that m and v could be moved closer to their
 use. Just did that.

 > BTW, the same considerations about locality of declaration apply to the
 V6 version.
 Fixed.
 >
 >
 > >> 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?
 > > Done.
 > The V4 stuff has been moved to the Linux-specific code, but the V6
 version of send() still has this problem.
 Fixed.

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