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:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp          |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130731
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Replying to [comment:6 tomek]:
 > 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.

 IMHO, it is useful if you have existing configuration with explicit
 interface names listed and you want to enable all temporarily. You can do
 it by simply adding an asterisk to the list (to enable all) and remove it
 if you want to fall back to earlier configuration.

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

 Done

 >
 > '''ctrl_dhcp4_srv.cc'''
 > 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).

 If they are not physically ready, they will not be opened - IfaceMgr
 doesn't open sockets on interfaces which are not running. I believe, we
 should have some extended support for the validation of the interfaces
 being input by the user - in particular, we could check that the interface
 name is valid. I guess, that was not the objective of this ticket so I
 simply took the simplest approach.

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

 I know, but it shouldn't be NULL because interface should be always
 present. Note that the code iterates over the collection of existing
 interfaces and for each of them calls getIface. The getIface wouldn't
 return NULL for the interface that we already found existent.

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

 Added for DHCPv4 and v6.

 >
 > '''dhcp4_srv_unittest.cc'''
 > Dhcp4ParserTest.selectedInterfaces - make sure that test works on
 machines and systems that do not have ethX interfaces, e.g. FreeBSD. This
 test is independent from the IfaceMgr so it should't be an issue. But yes.
 I will run the test on our build bot before I merge.
 >
 > '''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.

 We both have learned something. Obviously it was a typo. Thanks for
 catching this.

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

 I like the flexibility that the asterisk give with respect to addition of
 new interfaces. If we ever decide that this design is wrong it will be
 little effort to change it.

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

 Well, it depends what you mean by "configure". The !CfgMgr is a class
 which holds the server 's current configuration. So, changing the state of
 the !CfgMgr could be called configuration. But, since it is likely to be
 misinterpreted, I changed the comments.

 >
 > Most DHCPv4 comments also apply to DHCPv6.

 Done for DHCPv6 either. At this point, I again regret that we don't have
 shared code here.

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

 Thanks

 Proposed !ChangeLog entry:
 {{{
 6XX.    [func]          marcin
         b10-dhcp4, b10-dhcp6: Implemented selection of the interfaces
         that server listens on, using Configuration Manager. It is
         possible to specify interface names explicitly or use asterisk
         to specify that server should listen on all available interfaces.
         Sockets are reopened according to the new configuration as
         soon as it is applied.
         (Trac #1555, git abc)

 }}}

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