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]:
 > IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the
 data for v.iov_base instead of the C-style cast.
 That's not that simple. getData() returns const void* while iov_base
 structure has field of type void*. static_cast from const void* to void*
 does not work. I used const_cast<void*>.

 Using C++ style casts while dealing with system API is awkward. Around
 line 635 there is following conversion:

     struct in_pktinfo* pktinfo =(struct in_pktinfo *)CMSG_DATA(cmsg);

 CMSG_DATA is a macro defined in system headers. On my Linux box, it is
 defined as
 ((unsigned char *) ((struct cmsghdr *) (cmsg) + 1))
 so CMSG_DATA() is of unsigned char * type. 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.

 > IfaceMgr::send() - (V6 version) Remove the comment about ticket #1237.
 Also, can the Linux-specific part be put into a separate function
 (possibly in a separate file)?  It would serve to isolate the Linux-
 specific code from the rest of the function.
 We can do this, but I would very much like to avoid that. It would require
 putting msghdr and iovec structures as parameters. I'm not sure if those
 are defined in the same way on all supported systems (and if they are
 defined at all). That method would be mentioned in iface_mgr.h that is
 frequently used in many places. Instead of separating Linux code, we would
 spread it all around. The other alternative would be to pass msghdr and
 iovec pointers as void*, but that would be an ugly kludge.

 > IfaceMgr::receive4() - The "while" loop looks awkward with two "++s"
 increments.  Converting to "for" loop and inverting the test of the
 address family would simplify it.  Also, as this appears to be separate
 from the rest of the code, can't it be placed into a separate method?
 This code will be removed when support for receiving data on more than one
 interface (using select()) will be implemented. Changed to for loop. Added
 comment with reference to ticket #1555.

 > IfaceMgr::receive4() - a lot of the declarations look similar to that in
 other methods - is there anything that could be made common?
 Again, creating functions for them would require using some possibly Linux
 specific structures in header.

 We could define a function (not a member of the IfaceMgr class) that would
 have a scope limited to iface_mgr.cc, but that would be breaking down OO
 paradigm (and a good coding) for just a minor benefit.

 > IfaceMgr::receive4() - "pkt" should have the data type Pkt4Ptr (and
 should be declarated where it is initialized).
 >
 > IfaceMgr::receive4() - return variable should be of type Pkt4Ptr.
 Done.

 > IfaceMgr::receive4() - same comments made above concerning declaraing
 variables closer to their use apply.
 Done.
 >
 > IfaceMgr::receive4() - no need for a blank line after calling recvmsg()
 and the test on result.
 >
 > IfaceMgr::receive4() - suggest place the Linux-specific code in a
 separate function (in a separate file).
 See above regarding polluting headers with Linux-specific structures.

 > IfaceMgr::receive6() - most of the comments made for receive4()
 (including the conversion of the loop over the sockets from "while" to
 "for", and variable initialization) apply here.
 >
 > IfaceMgr::receive6() - the "no interfaces detected" message should be
 made only if "ifaces_.empty()" is true.  Currently it can be thrown if no
 interface has IPV6 capability (which is somewhat different).
 Updated the message.

 > IfaceMgr::receive6() - casts in the calls to setXxxxAddr should be
 C++-style.
 I think reinterpret_cast is the only suitable cast method.

 > IfaceMgr::getSocket() - in the method signature, "const Pkt6&" appears
 to be the more usual style than "Pkt6 const&".
 >
 > IfaceMgr::getSocket() - (both versions) in the "for" loop, as the
 conditions aren't too complicated, inverting and combining the conditions
 in a single "if" test and placing the "return" in the body avoids the need
 for "continue" statements.
 Done.

 >
 >
 > '''src/lib/dhcp/libdhcp++.{cc,h}'''[[BR]]
 > unpackOptions6() - should not be a space between "end" and ")" in the
 first "if" test.
 >
 > unpackOptions4(): using pre-increments is generally better than post-
 increments.
 That is true regarding objects. However, unpackOptions4 post-increments
 only plain integers, so there is no difference in performance. Increasing
 offset after byte was consumer is also more natural.

 > unpackOptions6()/unpackOptions4() - in the call to options.insert(), you
 could use std::make_pair as a less-verbose way of creating a pair to
 insert into "options".
 Great trick! I was not aware of the make_pair.

 > !OptionFactoryRegister() - should be no space preceding the "*" in the
 factory type.  Spaces needed around "!=" in the "if" statements.
 >
 > !OptionFactoryRegister() - throws an exception if the option type is
 greater than 254 - although "end" option has a value of 255.  If "end" can
 never be passed to this method, there should be a comment explaining why.
 Added appropriate comment. Also added exception in case of another special
 value 0 (that's padding option).


 > '''src/lib/dhcp/option.h'''[[BR]]
 > Typedef of !OptionBufferPtr does not need spaces after "<" and before
 ">".
 >
 > Typedef of !OptionCollection does not need to be split across two lines.
 Also, no need for a space before the ">".
 >
 > A comment should be added to the effect that it is assumed all over the
 code that !OptionBuffer is a vector.  (In several places an iterator over
 !OptionBuffer is dereferenced and assumed to point to a set of contiguous
 bytes that are interpreted as a data type such as uint16_t.  If the
 underlying !OptionBuffer data type were changed, this might not be true.)
 Added appropriate comment.

 > General: revisiting the Option class, I was struck by the separation of
 functions depending on "Universe".  What is the argument against an Option
 (abstract) base class with Option4 and Option6 derived classes?
 Hierarchy bloat and code duplication. Many soon to be implemented options,
 like integer arrays are shared between v4 and v6. Implementing them
 separate would need to code the same methods twice (or use ugly multiple
 inheritance).

 > '''src/lib/dhcp/option.cc'''[[BR]]
 > pack4() - since the constructor only rejects the V4 option type if type
 is greater than 255, there is a possibility that the Option object could
 hold an "end" option.  In this case, pack4() will attempt to add a length
 byte after it.
 Good catch. Also added check that 0 (PAD option) is invalid as well.
 >
 > pack4() - should add a sanity check for option size when writing the
 length byte for a V4 option - len() returns a uint16_t so could
 potentially overflow the field.
 Updated check in pack4()

 > pack() - "return" should have the argument enclosed in brackets.
 >
 > getOption()/delOption() - argument type should be uint16_t, not
 "unsigned short".
 Done.

 > addOption() - consider use of make_pair().
 Done.

 > '''src/lib/dhcp/option6_addrlst.cc'''[[BR]]
 > Remove comment "this wrapping is *ugly*..." ... unless you think it is
 still ugly, in which case complete the comment :-)
 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.

 > Option6AddrLst::unpack() - use V6ADDRESS_LEN in the "if" statement
 instead of 16. (More informative and consistent with use later on in the
 method.)
 >
 > Option6AddrLst::unpack() - would prefer the result of "distance % 16" to
 be explicitly compared against 0 (instead of using the implicit conversion
 to bool): the check is whether the remainder is non-zero and an explicit
 numerical check reinforces that.
 Done.

 > '''src/lib/dhcp/option6_ia.cc'''[[BR]]
 > Option6IA::unpack() - there is a warning if the distance between the
 start and end iterators is less than 12; what if it is greater than 12?
 Also, could this value be a symbolic constant (c.f. OPTION6_IAADR_LEN in
 Option6IAAddr::unpack())
 Nothing wrong happens if it is greater than 12. That only means that sub-
 options are
 present (which is usually the case for IA_NA and IA_PD as they are
 containers for addresses and prefixes). Distance equal 12 means no sub-
 options.

 > '''src/lib/dhcp/pkt4.h'''[[BR]]
 > The comments for the member variable data_ use double slashes instead of
 the triple slashes required by Doxygen and used for other member
 variables.
 Fixed.
 >
 > '''src/lib/dhcp/pkt6.h'''[[BR]]
 > getBuffer() header - should say "This buffer is only valid while the
 Pkt4 object is valid."
 It already said that. I think you meant "Pkt6" :-) Fixed.

 >
 > len() header - the header comment is not clear.  The "brief" explanation
 is that it returns the size of the packet, but the text implies that it
 returns the length of the options fields - and treats the header as one of
 them.  Also, why does options_ have to be set - if it is clear doesn't it
 mean that the packet has no options?
 Comment clarified.

 > A consistent style should be used for the inline expansions of one-line
 methods.  In some cases the expansion is on the same line as the
 declaration: in other cases it is split across three lines.
 I think I fixed them all.

 > Does ifindex_ deserve the "brief" line in its explanatory comments?
 I don't understand this comment. Do you want me to remove "@brief
 interface index" from
 description of int ifindex_ field? I wanted it to be there to explain
 (even when browsing
 Doxygen documentation for the whole class that shows only brief
 descriptions) that ifindex
 stands for "interface index", not conditional index ("if index").

 > There are apparently two ways of identifying the interface - the name
 and the index.  Are these truly independent or is there a unified way of
 accessing the relevant interface?
 Keeping them separated is really the way to go. Usually they are equally
 unique, but in some
 cases ifindex can change. It was the case in some older Ubuntu boxes that
 had race some kind of race condition. Also, in some systems (Windows XP)
 interface names can be really weird, like "Połączenie sieci
 bezprzewodowej" (pol. Wireless network link). Note spaces and national
 characters. It is also unfortunate that those names are language specific
 and change between languages. In such bizarre environments it is better to
 rely on interface indexes. There are also cases when interface can
 disappear and reappear (e.g. any USB interface) and there is no guarantee
 that it will get the same index. There are valid use cases, when we also
 want to refer non-existing interface by its name, e.g. when CPE device
 boots and wifi link initialization takes a long time. Also, depending on
 the context it is more convenient to use one identification or the other.
 When dealing with configuration, it is more likely that we will use
 interface names. On the other hand, socket operations more likely will use
 ifindex to refer specific interface.

 The bottom line is that we need both.

 > No need for spaces after the opening parenthesis and beflore the closing
 parenthesis in some EXPECT_EQ checks.
 I removed several instances of unnecessary spaces in various tests. I hope
 I found them all.

 > '''src/lib/dhcp/tests/iface_mgr_unittest.cc'''[[BR]]
 > sendReceive6 test - the loop setting the dummy payload should have
 spaces around binary operators.
 Done.

 > '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 > packOptions4 and packOptions6 tests - no need for the space before the
 ">" in the "opts.insert" lines.
 Used make_pair instead. Shorter and cleaner.

 > packOptions6 test - the EXPECT_NO_THROW test can be on a single line.
 Fixed. On a side note, I think EXPECT_NO_THROW macro is kind of pointless.
 If there is exception thrown during test, the test will fail anyway. The
 only benefit of using it is that the (failed) test will continue
 execution.

 >
 > No need for spaces after the opening parenthesis and before the closing
 parenthesis in some EXPECT_EQ checks.
 Done.

 > '''src/lib/dhcp/tests/option6_addrlst_unittest.cc'''[[BR]]
 > No need for spaces after the opening parenthesis and before the closing
 parenthesis in some EXPECT_EQ checks.
 >
 >
 > '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 > suboptions_pack test - "sub1" declaration does not need to sbe split
 across two lines.
 >
 > suboptions_unpack test - not sure what the cryptic comment "48 bytes"
 means.
 Clarified.

 > suboptions_unpack test - in the data declaration, binary operators
 should have a space around them.
 Done.

 > '''src/lib/dhcp/tests/option6_iaaddr_unittest.cc'''[[BR]]
 > basic test - in the initialization loop, binary operators should have a
 space around them.
 Done.

 > '''src/lib/dhcp/tests/option_unittest.cc'''[[BR]]
 > No need for spaces after the opening parenthesis and before the closing
 parenthesis in some EXPECT_EQ checks.
 Done.

 > '''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
 > constructor test - No space before the "*" in the declaration of pkt1.

 >
 > No need for spaces after the opening parenthesis and before the closing
 parenthesis in some EXPECT_EQ checks.
 >
 > capture() - C++ style is to put the "*" after the return type, not
 before the function name.
 >
 > capture() - can we line up the assignment of data elements?  However, it
 would be clearer to statically initialize the array.
 This code was actually automatically generated. Lined up assignments and
 added comment in iface_msg_unitest (in case we will ever use it again to
 convert captured packets into parsing tests).

 > packUnpack test - "3*Option" - should be spaces around the "*".
 Done.

 > addGetDelOptions test -  C++ style is to put the "*" after the return
 type, not before the function name (declaration of "parent".)  Also should
 be no space before the closing parenthesis.
 Done.

 > '''src/lib/util/buffer.h'''[[BR]]
 > The change appears OK, but tests/buffer_unittest.cc should be extended
 to check that the constructor using a vector works.
 Implemented. On a side note, I'm now 100% addicted to TDD. Buffer
 constructor actually didn't set position properly and test found that out.

 Also renamed some tests to reflect that they are for inputBuffer.

 > '''!ChangeLog'''[[BR]]
 > Text for the entry is OK.

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