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:  major  |  DHCP-20120220
                  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      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:6 stephen]:
 > '''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.
 That is true, but actual usefulness of this in case of DHCP would be very
 limited. There's good reason for that. Once hooks are implemented,
 callouts code can modify practically any part of packet or option at any
 stage. So can't really say "from now on this object is read-only". We can
 use const OptionPtr though (i.e. make a promise that specific function
 will not modify the pointer).

 > '''src/bin/dhcp4/dhcp4_srv.{cc,h}'''[[BR]]
 > Dhcp4Srv::run() - query should be declared and initialised on one line.
 Done.

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

 > Dhcp6Srv::setServerID() - check the Ethernet type (you're not in a hotel
 now :-)
 > Dhcp6Srv::setServerID() - for loop: spaces around binary operators
 please.
 Actually written this method for real. Now it tries to find suitable
 interface to generate DUID and generates proper DUID-LLT (that is
 recommended type). If that fails (running on OS that does not have
 interface detection implemented or on a box with really weird interfaces),
 it falls back to DUID-EN and uses ISC's enterprise number + random values.
 >
 > Dhcp6Srv::processRebind(): Creation of "reply" does not need to be split
 across multiple lines.
 Done. Also marked all skeleton methods with appropriate TODO.

 > '''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 "+".
 Done.

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

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

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