BIND 10 #2902: Direct DHCPv4 traffic on Linux - Linux Packet Filter (LPF)

[BIND 10 Development]

#2902: Direct DHCPv4 traffic on Linux - Linux Packet Filter (LPF)
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130523
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 9a49846cb4456b17526c30799df03c9a06c8a318

 '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 Dhcp4Srv::adjustRemoteAddr(): the test for the broadcast flag is
 currently:
 {{{
         bool bcast_flag = (question->getFlags() >> 0xF) ? true : false;
 }}}
 I'm not clear why there is a right-shift. As the method returns a
 unit16_t, and as the broadcast flag is defined in
 [http://tools.ietf.org/html/rfc2131#section-2 RFC 2131] as the left-most
 bit, the test would be better done as:
 {{{
         bool bcast_flag = ((question->getFlags() & 0x8000) != 0);
 }}}
 (In fact, as bcast_flag is a bool, the comparison is not strictly needed
 as the compiler should convert the numeric value to true/false as
 appropriate.  However I think it does make things clearer.)

 What about defining a symbol in Pkt4 (called something along the lines of
 FLAG_BROADCAST_MASK) and using it in the code instead of 0x8000?  Strictly
 speaking, although the fact that a broadcast flag is important to the
 packet filter code, the numerical value of the bit mask needed to obtain
 the information does not need to be hardcoded anywhere other than in Pkt4.

 Dhcp4Srv::adjustRemoteAddr(): the change made here fixes the issue raised
 in #2760.  When you close this ticket, please close that one as well.


 '''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
 Nothing to do with this ticket, but as the file has been modified... The
 general convention for the "public/protected/private" scope statements is
 to be flush against the left margin.  The "protected" statement is: the
 others are not.


 '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 NamedDhcpv4Srv constructor: with the constructor now fully doxygen-
 documented, the "port" argument should be documented as well.

 testDiscoverRequest: (trivial) in the loops to set the source and
 destination addresses, the source HW address loop uses an increment of
 "i++", the destination address loop uses "++i".

 testDiscoverRequest: typo - s/interprented/interpreted/

 adjustRemoteAddressSelect(): resetting the !IfaceMgr packet filter should
 be done in the test fixture (Dhcpv4SrvTest) destructor.  This guarantees
 it will always be done, regardless of how the tests exits. (As it is, the
 packet filter is changed, an ASSERT_NO_THROW test is done, then the packet
 filter is reset.  The ASSERT_NO_THROW test could fail, which would lead to
 the test exiting without executing the reset code.)


 '''src/lib/dhcp/pkt_filter_lpf.cc'''[[BR]]
 > Space added. What do you mean by split macro from its body?
 I mean "MACRO (BODY)" as opposed to "MACRO(BODY)".  However, that's been
 corrected.

 In the BPF program, with the introduction of symbolic constants, the part
 of the comments describing the offset arithmetic (which explained what the
 various numbers were) are now no-longer needed. However, they can be left.


 '''!ChangeLog entry'''[[BR]]
 Suggest a change of wording:
 {{{
 6XX.    [func]*         marcin
         b10-dhcp4: Added the ability for the server to respond to a
 directly
         connected client which does not yet have an IP address.  On Linux,
 the
         server will unicast the response to the client's hardware address
 and
         the 'yiaddr' (the client's new IP address). Sending a response to
 the
         unicast address prevents other (not interested) hosts from
 receiving
         the server response. This capability is not yet implemented on
 non-Linux
         Operating Systems where, in all cases, the server responds to the
         broadcast address. The logic conforms to section 4.1 of RFC 2131.
         (Trac #2902, git abc)
 }}}
 (As an aside, if the client doesn't have an IP address yet, I don't see
 why RFC 2131 mandates that the response also goes to the yiaddr.)

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