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-20120305
  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:11 stephen]:
 > > The only way to cast that to struct in_pktinfo* is to use
 reinterpret_cast, which is only slightly better than pure C cast. I would
 be less readable, though. I left C style cast for now. Let me know if you
 prefer reinterpret_cast instead.
 > I would prefer reinterpret_cast, but Jinmei is opposed.  We had a
 discussion about just this point on #1593 recently.  We ended up using a
 separate function to perform the conversion, inside which there was a
 construct of the form
 > {{{
 > static_cast<desired_type*>(static_cast<void*>(pointer));
 > }}}
 > For consistency, I think we should do the casting here using a similar
 function.
 In my opinion that is overly bloated, but I did as you suggested. Added
 new header file for pktinfo conversions. There is also in_pktinfo
 (compared to currently used in6_pktinfo) structure in Linux and possibly
 other systems, so I kept name of the file only similar to actual structure
 (pktinfo_util.h rather than in6_pktinfo_util.h).

 > > Also, can the Linux-specific part be put into a separate function
 (possibly in a separate file)?
 Done. There are two outstanding ifdef(OS_LINUX) instances. One of them is
 about specific socket bindings (we will remove it once we start supporting
 BSD family) and the second
 one will be remove once we have OS-specific files. (iface_mgr_bsd.cc and
 iface_mgr_solaris.cc).

 > os_setup_interface is in a separate file and it is this file that
 betterholds the operating-specific stuff. (In this way for example, there
 would be no need to reference the Linux-specific CMSG stuff within the
 general functions.)
 CMSG and sendmsg/recvmsg is defined in POSIX (or at least it is supported
 everywhere). The generic mechanism allows definition of additional
 messages that are Linux specific.

 > > My sense of good taste (or lack of thereof) must have changed
 recently. I don't consider calling 4 methods
 (getAddress().to_v6().to_bytes().data()) to get an actual value of address
 from address object ugly anymore.
 > It's called getting old.  You develop a taste for the sophisticated
 things in life - good food, fine wine, and the use of four methods to get
 an address. :-)
 You are mistaken, sir. Women are getting old. Gentlemen are just gaining
 experience. :-)

 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > run(): change is OK, but suggest either:
 > // Client's query and server's response
 I chose this one as it make the lines shorter.
 >
 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > setServerID(): the comment about the DUID being generated and stored is
 fine.  I think we should create a separate ticket to abstract the DUID
 into a separate class.  That way we can extend the code to add other DUID
 types without affecting operation of the server.
 Created ticket #1768. That will be two classes, actually: one for DUID
 itself and second for  representing DUID as option.

 > setServerID(): (Comment) "all those conditions".  Do you mean "All the
 following checks"?  (Also, being pedantic, please start comments with
 capital letters.  It is easier to read them they follow the rules of
 English.)
 >
 > setServerID(): (Comment) "mac at least 6 bytes".  Should be "MAC: at
 least 6 bytes".  Also, it would be more descriptive to put that comment
 into a header with the declaration of "const size_t MIN_MAC_LEN = 6".  (I
 note that there is already MAX_MAC_LEN symbol.)
 Added.

 > setServerID(): with regards to checking the array for all zeroes, this
 would seem to be of general usefulness and could be abstracted into a
 separate function (perhaps in a separate "util/range_utilities.h").
 That is true. I did as you suggested, but I think we could also start
 using lambda expressions. I didn't put much thought into this part of the
 code as it is typically done only once during whole life of a server
 installation (not even during every startup). Temporarily allocating
 couple of bytes just once is not an issue.


 > setServerID(): The fillling in of random numbers should be done in a
 separate function.  It will make it easier to update. (I'm not sure than
 rand() has particularly good randomisation properties on all operating
 systems.)  Also, if you are declaring a "standard", again it would be best
 in a separate function.
 My comment about standard was more or less a joke. RFC says that DUID-EN
 should be followed by company defined data.

 Nevertheless, having a way to generate random content may be useful. Added
 range_utilities.h as suggested. I also noted that headers in src/lib/util/
 are named *_utilities.h, but in src/lib/util/io/ follow different pattern
 and use *_util.h instead. That is somewhat strange, especially that there
 are two new headers added in this ticket. I decided to use _utilities.h
 pattern.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 > ''"This test will start failing after 2030.  Hopefully we will be on
 BIND12 by then."''[[BR]]
 > Optimist!
 >
 > If the isRangeZero() function above is written (and separately tested),
 it can be used in this test.
 Done.

  > '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 > stubdetectIfaces(): call to "if_nametoindex" has spurious blank space
 inside the parentheses.
 Done.
 >
 > OpenSocketX(): May want to consider defining the port as "const
 uint16_t" (and in the header file as well).  It doesn't change meaning,
 but it is a guarantee that "port" is not modified inside the method.
 Done.
 >
 > send(): as "const_cast" is unusual, a description why there is a need to
 write to what is considered to be an unmodifiable buffer is needed.
 Added explanation.

 > send(): "int result = sendmsg(..." is incorrectly indented.
 Fixed.

 > receive6(): I must have missed this last time round, but:
 >  * Declare the objects closer to where they are used.
 >  * Why is "v" not initialized to zero before being used?
 >  * Use "static_cast<void*>()" instead of "(void*)".
 Done.
 >
 > '''src/lib/dhcp/iface_mgr.h'''[[BR]]
 > getIFaces() actually returns a reference to the container collection
 stored inside !IfaceMgr.  The documentation should indicate that the
 reference only valid for as long as the !IfaceMgr object is valid.
 Added comment.

 > '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 > unpackOptions6(): should be spaces around binary operators.
 Added missing spaces.

 > !OptionFactoryRegister(): typo "consumed", not "consumer".
 Fixed.
 >
 > '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 > packOptions4/6 tests: EXPECT_THROW should have no spaces either side of
 the parentheses.
 >
 > '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 > suboptions_unpack tests: In the "ASSERT_EQ", the expected value should
 be the first parameter.
 >
 > '''src/lib/dhcp/tests/option_unittest.cc'''BR]]
 > v4basic_test: there is no need to check if "opt" is defined before
 calling "delete".
 >
 > '''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
 > Thanks for lining up the initialization of data.  Now for a spaces
 around the "=" signs?
 >
 > packUnpack test: should use "static_cast<const uint8_t*>() instead of a
 C-style cast.

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