BIND 10 #1555: refactor: DHCP listening on more than one interface, timeout

[BIND 10 Development]

#1555: refactor: DHCP listening on more than one interface, timeout
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130717
         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 tomek):

 * owner:  tomek => marcin


Comment:

 General comment: Is there any specific use case that would make sense to
 mix wildcard and specific interface name? Wildcard implies all interfaces,
 including those that are explicitly mentioned.

 '''bind10-guide.xml'''
 Please update first example in section 17.2 ("Dhcp4/interface" =>
 "Dhcp4/interfaces") and in 18.2 ("Dhcp6/interface" => "Dhcp6/interfaces")

 Please move ControlledDhcpv4Srv::openActiveSockets to Dhcpv4Srv class.
 Please add @todo in its code to mention that it should be optimized and
 not close existing open sockets. We don't care about closing/reopening UDP
 and raw much, but some time in the future we'll have TCP sockets as well
 (for failover and for bulk leasequery).

 I like the concept of interfaces being active, but we'll need to expand it
 a bit in the future. Interfaces are sometimes marked as to be activated,
 but they are not physically ready. Dibbler supports such mode of
 operation. For example you can say that interface wifi0 should be
 configured, but it is not ready yet, so the server (or client)
 periodically monitors its state and starts configuration once physical
 layer is up and running (e.g. wifi association or Ethernet cable plugged).
 While the current approach is ok for now, it will eventually be evolved
 into a list of interfaces that should be activated. (a diff between
 currently activated and the list user specified).

 '''ctrl_dhcp4_srv.cc'''
 openActiveSockets(): You never check if iface_ptr is non-NULL. The code
 will crash if it is NULL.

 '''dhcp4_srv.h'''
 getPort() -  please add a comment that the server listens on port 67 and
 any other ports are used for testing only.

 '''dhcp4_srv_unittest.cc'''
 Dhcp4ParserTest.selectedInterfaces - make sure that test works on machines
 and systems that do not have ethX interfaces, e.g. FreeBSD.

 '''iface_mgr.cc'''
 Please study "if" clauses around lines 297 and 364. I was not aware of the
 comma operator. BTW I find this page very englightening:
 http://en.wikipedia.org/wiki/Comma_operator To be honest, I was not aware
 that such an operator even exists.

 activateAllIfaces(): I'm on a fence with this one. On one hand, we could
 expand * into list of all interfaces. On the other hand, there are cases
 where interfaces may appear later (e.g. ppp0), so it may be useful to keep
 the flag. When new interface appears later and the flag is set, we could
 then immediately mark it as active.

 '''cfgmgr.h'''
 The comment for activateAllIfaces() and deleteActiveIfaces() is wrong. It
 does not configure the server (i.e. the function does not cause the server
 to open sockets), it merely stores the list.

 Most DHCPv4 comments also apply to DHCPv6.

 In general, a very good work. Thank you for doing this.

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