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-20111011
                  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:3 stephen]:
 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > The #include of 'io_address.h' should be {{{#include
 "asiolink/io_address.h"}}}
 Done. In dhcp6_srv.cc and other files in src/bin/dhcp6 and src/lib/dhcp.

 > In Dhcpv6Srv::run(), there is the following construct:
 > {{{
 > if (rsp != boost::shared_ptr<Pkt6>()) {
 > }}}
 > 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.
 It seems that is ok in BIND10, though, so I changed it to use simpler
 form.

 > getServerID(): Presumably there is a getServerID() as a counterpart to
 setServerID()?  (If not, why not access serverid_ directly?) If so, it
 would make sense to have getServerID() return a reference to serverid_ to
 save on the copying of the shared pointer.  Also, getServerID() is
 sufficiently trivial that it can be coded inline in the header.
 Done.

 > In setServerID(), need to check that srvid[3] should really be 6.
 Certainly not. While 6 is the most common hardware type (Ethernet), we
 must not limit ourselves to this hardware type. In particular, Infiniband
 is represented by hardware type 32, which is a valid type. Anyway, there's
 explicit statement in RFC3315 that says that DUID content MUST be treated
 as opaque value. If user desires to have it composed with values other
 than 6, it is legal.

 > Does setServerID() need to return anything?  If the code fails to set
 the server ID, would an exception be a better way of signalling it?
 Changed as suggested. Once fully implemented, setServerID() will throw
 exception if it fails to load old DUID and there are no suitable
 interfaces for generation of new DUID.

 > in processSolicit(), the code contains two declarations of the form
 {{{Option * tmp}}}.  This should be {{{Option* tmp}}} (as the current
 layout looks at first glance like a multiplication).  Also, should be no
 space between the ">" and "(" in the dynamic_cast statement.
 Done.

 > Should include a 'TODO:' comment about changing them in the code for the
 hard-wired times and address.
 Added TODO explaining that this method should be rewritten once
 LeaseManager is implemented.

 > '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 > To avoid a copy of the input variable, the {{{solicit}}} argument to
 many of the methods should be passed by reference.
 No. In current state of implementation, packet is consumed immediately and
 is not stored anywhere. That is likely to change, however. Note that
 leasequery protocol may ask about details about relays, so it may be
 useful to keep at last some parts of message. Also, if we decide to
 implement distributed processing, we need separate instance of
 shared_pointer.

 > All methods in Dhcpv6Srv should have a Doxygen header.
 Done.

 > The class Dhcp6Srv should have a header explaining the purpose of the
 class.
 Done.
 > The convention in BIND 10 is to use nested namespaces rooted at "isc"
 for different components.  Suggest that the class be put into isc::dhcp.
 Done. Note that library, dhcp6 and upcoming dhcp4 components will all use
 a single (isc::dhcp) namespace.

 > Declaring the test class as a "friend" in the production code is unusual
 (in BIND 10) but valid.  This should be discussed on bind10-dev as to
 whether we want to make this a standard.  However, if the test class is a
 friend, why are methods protected?
 Use of a friend was a quick hack, rather than permanent solution.
 Implemented derived class that exposes Dhcpv6Srv and removed friend
 statement.

 > To prevent copying, the class should be derived from
 {{{boost::noncopyable}}}. (In fairness, although this was decided back in
 February, is has only today been included into the coding guidelines.)
 Done.

 > All member variables should include a Doxygen comment stating their
 purpose.
 Done.

 > '''src/bin/dhcp6/iface_mgr.cc'''
 > (A few things that may have been picked up in the review of #868 but
 caught my eye when reviewing these changes.)
 >
 > instance(): the {{{if}}} statement should contain braces, even if the
 contents are only one statement.
 Done.

 > Iface constructor.  For setting the mac_ array to zero, memset is OK
 (although some authorities prefer std::fill).  However the length should
 be {{{sizeof(mac_)}}} and not a hard-coded 20.
 Using std::fill is a overkill. modified memset should do the trick. Also
 added MAX_MAC_LEN const.

 > getFillName(): {{{return (name_ + "/");}}} is simpler than creating a
 temporary ostringstream object.
 No, it is not. It is not possible to add integer to strings. using
 stringstream is the preferred way of converting integers to strings in
 C++. Note that the format is eth0/2 (interface-name, a slash followed by
 integer representing interface index).

 > getPlainMac(): I think that setting the fill, width and radix once
 outside the {{{for}}} loop should be OK. (Also, if mac_ is declared as
 {{{uint8_t}}}, there should be no need for the cast to an {{{int}}} when
 output each element of it.)
 Note: As some manipulator affect only the next token (and I fail to
 remember which ones they are) I chose to set the before every token.

 To answer this comment: Yes and no. uint8_t is still printed as char, so
 cast to int is required, at least with g++ 4.5.2. Some stream manipulators
 apply to next token only. In particular that is true for width(). This
 page has nice table about applicability of stream manipulators:
 http://www.fredosaurus.com/notes-cpp/io/omanipulators.html

 Cast to (int) stays, fill() moved out of loop.

 > In !IfaceMgr constructor, {{{control_buf_}}} could be declared as a
 {{{boost::scoped_ptr<>}}} to provide automatic destruction in the case
 that anything else in the constructor throws an exception. (Would then
 remove the deallocation of {{{control_buf_}}} from the destructor.)
 scoped_ptr is not the proper type here as it will call delete to destroy
 pointer object. This is an array, so delete [] operator should be called.
 Note that deleting array type with delete results in undefined behavior.
 It just happens to work in g++.

 The better template would be shared_array<>. Do you want me to use it?

 > Header for openSocket.  The BIND 10 convention is to place the headers
 in the .h file.  Also, the first line of the header contains a typo "nad"
 instead of "and".
 >
 > receive(): without checking where this is called from, would not
 throwing an exception (instead of returning an empty packet) be a better
 way of signalling that the receive has failed?
 No. Once LeaseManager is implemented, this method will get a timeout timer
 ("try to receive something for 10 seconds. After that we need to expire a
 lease"). Not receiving anything will be a proper event.

 > '''src/bin/dhcp6/iface_mgr.h'''
 > See comment in dhcp6_srv.h about namespaces.
 Done.
 >
 > All methods - including those in the contained Iface struct - should
 have Doxygen headers.
 Done.

 > Remove the note that {{{struct Iface}}} could be a class as well - in
 this context, a {{{struct}}} is fine.
 Done.

 > {{{mac_}}} is best declared as {{{uint8_t}}}. (It is not being used as a
 {{{char}}}.)
 Done.

 > Should remove the final comment in {{{struct Iface}}} as there is no
 next field.
 This was an explanation why there is no such field. Removed as suggested.

 > Suggest grouping the variables and the method declarations in {{{struct
 Iface}}} instead of putting the latter in the middle of the former.
 Done.

 > {{{Iface * getIface(...)}}} should be {{{Iface* getIface(...)}}} (looks
 like a multiplication).  This appliease to other similar declarations in
 the file.
 Done.

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

 > All member variables should include a Doxygen comment stating their
 purpose.
 Done.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 > Basic test.  The comment says that "an attempt to bind this socket will
 fail" yet the test is {{{EXPECT_NO_THROW}}} - which indicates that no
 exception will be thrown when the test succeeds.
 See IfaceMgr::IfaceMgr() comment. This exception is caught prematurely. It
 should stay that way until proper interface detection is implemented. I
 really don't want to engineer a work-around for a test that tests stub
 implementation that is going away in 2 months.

 > Should be a space after the comma in the {{{TEST_F}}} line for
 Solicit_basic.
 Added.

 > Inconsistency in declaration: both {{{sol}}} and {{{ia}}} are of the
 form {{{boost::shared_ptr<xyz>}}}, yet the former is initialized using the
 construct "ptr x = ptr(new Xyz(...))" and the latter "ptr x(new
 Xyz(...))".
 Done.

 > Would help to include a comment explaining what options are being set in
 the packet.
 Added a comment about content of "sent" message and expected content of
 "received" message.
 Also added several new checks.

 > See above for comments on the testing whether a shared pointer points to
 a null object.
 >
 > In the Solicit_basic test, the contents of the ASSERT_TRUE macros can
 simply be the shared_ptr in question - the implicit conversion to bool
 will return true if the pointed is non-null.
 Done.

 > '''src/bin/dhcp6/tests/iface_mgr_unittest.cc'''[[BR]]
 > In the test loDetect, can you guarantee that the default directory when
 running the test is writeable and that "interfaces.txt" will be created?
 In fact, removing "interfaces.txt" and instead using an environment
 variable might be safer.  (Which will require an update to the !IfaceMgr
 code.)
 I politely refuse to implement this. This is a temporary workaround that
 is expected to go away in less than 2 months. It is not perfect, I agree.
 However, at this stage of development, write access to current directory
 during running tests is required. Added comment that explain that.

 Let's not over-engineer a workaround. We risk making it a feature.

 > In the "sockets" test, the socket numbers are perhaps best put as
 constants at the head of the file (in case they need changing at some time
 in the future).
 These are just random numbers. Well, almost. DHCPv6 operates on 546 and
 547 ports. I used normal value+10000 to have the ability to run as non-
 root user. Defining constants would be confusing. Tests would use
 something like DEFAULT_DHCPV6_PORT_PLUS_10000 + 1. It would be more
 confusing than directly using numbers. They are not expected to be unified
 among different tests. On the contrary, using different values would
 increase test coverage.

 > In test sendReceive, the {{{ASSERT_TRUE}}} condition can be just
 {{{rcvPkt}}} (see above).
 Done.

 Commit-id: 8fdcf2aa39b5fdb224f5e57f9605090409abc4a1

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