BIND 10 #991: Method to send a IPv4 packet to a client without an address

[BIND 10 Development]

#991: Method to send a IPv4 packet to a client without an address
-------------------------------------+-------------------------------------
            Reporter:  shane         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp4         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130411
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0.0           |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:9 tmark]:
 > Review Comments:
 >
 > 1. dhcp4_srv.cc -  Comment wording is wrong:
 > line 521 -  "...client requesting new lease. We scan end response to"
 > s/scan end/can send/

 Fixed.

 >
 > 2. dchp4_srv.cc -  else clause comment (line 781) says
 > "...On the other hand, if we fail to set remote
 > address to broadcast here, we can't unit-test the case when
 > server doesn't support direct responses to the client
 > which doesn't have address yet."
 >
 > Does this imply that answer-setRemoteAddr() can fail? If so should we be
 > checking for that failure?

 No. setRemoteAddress() is fail safe, because it simply assigns existing
 !IOAddress object to a class member. I modified the comment to better
 explain what I had in mind.

 >
 >
 > 3. dhcp4_srv_unittest.cc - in testDiscoverRequest(), there are 5 more
 tests
 > (lines 437-446) of response content following the call to
 messageCheck().  Why
 > aren't these in messageCheck() it seems like they belong there.

 Yes. I have moved them there.

 >
 > 4. dhcp4_srv_unittest.cc - You "repeat the test" on line 453. Won't this
 will
 > call processRequest for both a DISCOVER and a REQUEST type, regardless
 of the
 > value of msg_type?

 Good catch. Such things usually happen when there is a lot of copy-paste
 done.

 >
 > 5. pkt_filter, general -
 >
 > Doesn't this concept also apply to IPv6? The implementation for
 pkt_filter_inet.h
 > is IPv4 specific.  Currently, an IfaceMgr isn't instantiated only for 4
 or 6,
 > rather it supports both based on the send or receive methods invoked.
 It would
 > seem that if packet fitlers will be needed for v6: then either there
 should be
 > protocol-specific derivations of pkt_filter and IfaceMgr will need an
 > instance of each; or pkt_filter needs protocol specific sends/receives.

 Possibly, but I haven't done that for V6 intentionally:
 - Direct V4 traffic requires !''manual!'' construction of ip headers to
 respond to the client which doesn't have address. This is not the issue
 for IPv6 so implementation for IPv6 it is simpler and more generic (cross
 OSes). What we have today works well on either Linux or BSD.
 - There is a lot of changes to packet handling routines for V4. I don't
 want to do revolution in IPv6 domain while we have to do revolution in V4
 domain. If we want, we can do it later, once IPv4 Direct traffic works.

 >
 > 6. PktFilterInet::openSocket() - if IP_PKTINFO is supported we try set
 socket
 > option for it and throw if it fails.  What if IP_PKTINFO isn't defined?
 Isn't
 > that just as "fatal"?

 It is not. It may be undefined on some OSes and we still want the server
 to work. For sure it is not fatal, maybe just need another way to get the
 information about incoming packet (but this is out of scope in this
 ticket).

 >
 > 7. PktFilterInet::openSocket()  - exception message for SO_BROADCAST
 option failure says "Failed to set SO_BINDTODEVICE".

 Fixed.

 >
 > 8. Code does not compile under OS-X: (I suspect it won't compile under
 Solaris
 > either).
 >
 > libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../../.. -I../../../src/lib
 -I../../../src/lib -I/opt/local/include -I/opt/local/include
 -D__APPLE_USE_RFC_3542 -DOS_BSD -I../../../ext/asio
 -I../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wwrite-
 strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -Wno-missing-
 field-initializers -g -O2 -MT libb10_dhcp___la-iface_mgr_bsd.lo -MD -MP
 -MF .deps/libb10_dhcp___la-iface_mgr_bsd.Tpo -c iface_mgr_bsd.cc  -fno-
 common -DPIC -o .libs/libb10_dhcp___la-iface_mgr_bsd.o
 > iface_mgr_bsd.cc: In member function 'int
 isc::dhcp::IfaceMgr::openSocket4(isc::dhcp::Iface&, const
 isc::asiolink::IOAddress&, uint16_t, bool, bool)':
 > iface_mgr_bsd.cc:54: error: 'socket_handler_' was not declared in this
 scope
 > iface_mgr_bsd.cc:61: error: 'SO_BINDTODEVICE' was not declared in this
 scope
 >
 >
 > Under Debian everything builds, unit tests pass.  I even ran a test with
 perfdchp and every
 > thing works fine.

 It is fixed now.

 Proposed !ChangeLog enytry:
 {{{
 595.    [func]          marcin
         libdhcp++: abstracted methods which open sockets and send/receive
         DHCP4 packets to a separate class. Other classes will be derived
         from it to implement OS-specific methods of DHCPv4 packets
 filtering.
         The primary purpose for this change is to add support for Direct
         DHCPv4 response to a client which doesn't have an address yet on
         different OSes.
         (Trac #2991, git abcd)

 }}}

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