BIND 10 #992: DHCP IPv4 server as a dummy BIND 10 component

[BIND 10 Development]

#992: DHCP IPv4 server as a dummy BIND 10 component
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111221
                  Component:  dhcp4  |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0.0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:9 stephen]:
 > Reviewed commit d8cd199a66645341270081a7f409c557e596099b
 >
 > '''src/bin/dhcp4/dhcp4.spec'''[[BR]]
 > Just a comment that we will eventually need a list of interfaces.
 However it is fine for now.
 >
 > '''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
 > Dhcp4Srv contructor: port argument needs Doxygen entry in the header.
 Added.

 > processXxx(): I feel that the arguments should be passed by reference to
 avoid the overhead of a reference count increase during the class.
 (Although I know Jinmei disagrees with me on this one.)
 Added references. Note that this should not be const reference as there is
 a possibility that packets will be modified, e.g. if incoming packet does
 not have PRL, it would be useful to add empty PRL for easier processing.

 > shutdown member variable should be shutdown_
 Done.

 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > run(): In the longer term we need to address how to shutdown the server
 if it is in the receive4() call when the shutdown request is received.
 Yes. receive4() and receive6() also will take a timeout parameter that
 specifies how long reception attempt should take. The most obvious use for
 this is lease expiration. Exact way of handling shutdown (or any other
 control sequence) is to be determined. Perhaps receive4() and receive6()
 will be called in a separate thread? That requires architectural
 discussion.

 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 > The comment should be '"naked" DHCP4 server', not '"naked" Interface
 Manager'
 >
 > In NakedDhcpv4Srv(), if you don't change the function signatures you
 don't need to duplicate base class methods.  (In this case, the derived
 class receives the parameters by value and passes them to the base class
 by reference; if it were to received the parameters by reference the
 methods would be no-op and could be omitted.)
 I have to. Although signatures are the same, access specifiers are
 different. Naked server has them public, so they can be used in tests.

 > "basic" test: there is no need to check is srv is non-null before
 attempting the deletion.
 Fixed.

 > processXxx tests: Do we want to do an ASSERT_NO_THROW around the "delete
 src"?
 Although we did this in many tests, I think it is redundant. Test will
 fail anyway if exception is thrown in code that was not explicitly
 expected (EXPECT_THROW or ASSERT_THROW). As this is the last statement in
 every test, it does not make much sense to add it. I we should do
 something opposite eventually, i.e. remove EXPECT_NO_THROW() in many
 tests.

 > '''src/bin/dhcp4/tests/dhcp4_unittests.cc'''[[BR]]
 > "return result;" should be "return (result);" according to the coding
 guidelines.
 Done.

 > '''src/lib/dhcp/iface_mgr.*'''[[BR]]
 > I didn't review these files or tests as they receintly appeared in
 another ticket that I reviewed and I understand there is a lot of cross-
 over between tickets at the moment.
 Yes, that is true. This code went through significant changes recently.
 Perhaps after end of Dec. release, we could deal with some tickets that
 address rewriting Pkt6 to use vector<uint8_t> and then re-review updated
 IfaceMgr again?

 Added ChangeLog entry.

 Lease review.

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