BIND 10 #1708: Integrate DHCP6 process startup into BIND 10

[BIND 10 Development]

#1708: Integrate DHCP6 process startup into BIND 10
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:         |  DHCP-20120730
  medium                             |            Resolution:
                  Component:  dhcp6  |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:6 stephen]:
 > Reviewed commit f788c425a50d504d7d23d3945b67f2ad25513495
 >
 > '''src/bin/dhcp4/main.cc'''[[BR]]
 > main(): Why does the ControlledDhcp4Srv have to be created via "new"?
 It can be created as an automatic object, which removes the need to delete
 the server when run() returns.
 new/delete gives you better control over when object is deleted. But
 there's nothing this flexibility could be useful for in this particular
 context, so changed to automatic object. I did the same for dhcpv6 object.

 > Suggest boost::lexical_cast be used to interpret the port number - the
 current use of strtol with a null value for endptr will not catch all
 invalid values.
 I have never used lexical_casts before. Nice!

 Also added 2 extra tests for each component checking if this lexical_cast
 really works. It seems it does :)

 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > Constructor: IFaceMgr::instance() is called and if there is a failure,
 the shutdown_ variable is set and an error is logged. Why is the condition
 then ignored (the code immediately goes on to call countIfaces() on the
 created instance).  And if there are no interfaces, another error is
 recorded, shutdown_ is set, yet the error is ignored and the code attempts
 to open sockets.  Why are the errors ignored?
 That is a side effect (or a leftover, if you prefer) of the issue with
 detecting interfaces on non-Linux systems. It used to be that those
 platforms usually returned 0 interfaces unless someone edited
 interfaces.txt file which was usually not the case.

 The idea was to go with graceful shutdown (create service and then shut it
 down immediately) rather than fail. This would in turn allow gentler
 clean-up (like notifying other DHCP components, closing leasequery or
 failover sockets etc). While there's no clear benefit at this stage of
 development, we will save some time of not requiring refactoring later.

 But you are right. It does not make sense to continue. I have modified the
 code to abort on the first error it encounters. Also updated, dhcpv4 code.

 > '''src/bin/dhcp6/ctrl_dhcp6_srv.cc'''[[BR]]
 > Note: This code is very similar to that in
 src/bin/dhcp6/ctrl_dhcp4_srv.cc (from ticket #1651).
 Yes, those classes are almost identical.

 >
 > '''src/bin/dhcp6/main.cc'''[[BR]]
 > main(): Why does the software start up a ControlledDhcpv6Srv object
 then, after run() returns and the server is deleted, start up a Dhcp6Srv
 object?
 Where do you see Dhcpv6Srv started after ControlledDhcpv6Srv is created
 and run()?

 > main(): Why does the ControlledDhcp4Srv have to be created via "new"?
 It can be created as an automatic object, which removes the need to delete
 the server when run() returns.
 Done.

 > Suggest boost::lexical_cast be used to interpret the port number - the
 current use of strtol with a null value for endptr will not catch all
 invalid values.
 Done.

 > dhcp6 and dhcp4 main() are practically identical.  As they depend on a
 type (the type of the server), they could be combined into a single
 templated function.
 Agree. Unification is good in general, but the code may start looking
 differently once we grow more features, e.g. failover in v4, prefix
 delegation or bulk leasequery in v6, etc. We should also consider if
 common code will not bring up dependency hell (like common code could
 possibly require v6 failover methods in dhcpv4 implementation). These are
 things that should be avoided. Nevertheless, unless there are serious
 reasons to do otherwise, unification is the answer. See new ticket #2146.

 > '''Overall'''[[BR]]
 > I suggest we open a new ticket to refactor the common control code in
 dhcp4 and dhcp6 into a single set of functions/classes (and tests).
 Done, see ticket #2146.

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