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-20130731
         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:

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

 Fair enough.

 > > 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.
 It shouldn't, but let's be on the safe side. Please add if (!iface) throw
 something; and be done with it.

 > > '''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.
 Yes, we did. It's an interesting trick and will increase our chances in
 the obfuscated C code contest. :)

 > 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.
 Ok, let the code stay as it is.

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

 > Done for DHCPv6 either. At this point, I again regret that we don't have
 shared code here.
 Although the code looks similar, there are subtle differences that would
 make working with the common code a pain or at least would require a lot
 of careful thought. And since the Pkt4 and Pkt6 do not share common base
 class, you couldn't really share much mode.

 > 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)
 >
 > }}}
 Looks good. Please add two if (!iface) in the code and then it is ready
 for merge (assuming you checked that the tests that use ethX names work on
 systems that don't have such interface names).

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