BIND 10 #2597: DHCP server must store a server ID

[BIND 10 Development]

#2597: DHCP server must store a server ID
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130122
         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 => stephen


Comment:

 Replying to [comment:4 stephen]:
 > Reviewed commit 695d9fc3bc135de080369b0b7213aca3c264216c
 >
 > '''!ChangeLog'''[[BR]]
 > Suggest a slight change:
 > {{{
 > b10-dhcp4/b10-dhcp6: The DHCP server now generates a server identifier
 > the first time it is run.  The identifier is preserved in a file across
 > server restarts.
 > }}}
 Used as suggested, with slight change. DHCPv6 generated its DUID before.
 This change brings the storage part.

 > '''doc/guide/bind10-guide.xml'''[[BR]]
 > '''src/bin/dhcp4/dhcp4_messages.mes'''[[BR]]
 > '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 > Some changes made and pushed.
 Thanks.

 > '''src/bin/dhcpX/dhcpX_srv.{cc,h}'''[[BR]]
 > I feel that it would be more logical for the server ID to be a separate
 class.  It would be easier to test, as well as removing the minutiae of
 server ID manipulation from the DHCP server class.
 There is a class for DUID. I agree that we should have OptionDUID class as
 well, but I'm not sure if we want to stuff the DUID generation in there.
 This is a generic class that can be used by many entities, like relay,
 requestor or dhcpperf. None of those tools would require DUID generation
 routines, merely parsing/building received options. On the other hand
 adding such routines there would induce dependency of requiring to link
 with IfaceMgr and its interface detection routines.

 This code will never be trivial to test, so you may gain just a little
 bit, but not much. As for the minutiae of server ID manipulation, I
 believe it is in exactly the right place. That is part of the server
 duties to manage its own server identifier. Even the identifier name says
 so.


 > The DhcpvXSrv constructors call the methods that create the server ID.
 Some of these can throw exceptions.  At the very least, the constructor
 should catch the exception, print out an error message and re-throw the
 exception.
 The whole constructor is encapsulated with large catch at the end. See
 lines
 dhcp6_srv.cc:105
 dhcp4_srv.cc:90

 Both server identifiers are essential for the server operation. There is
 not viable recovery path. If the server generation fails, we shut down.

 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > loadServerID()/generateServerID(): IOAddress has been extended to
 include an isV4() method (saves comparing the family against AF_INET).
 Updated to use isV4().

 > '''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
 > Description of the SERVER_ID_FILE talks about a DUID: copy/paste error?
 Indeed copy/paste error. Updated.

 > '''src/lib/dhcpsrv/cfgmgr.cc'''
 > !CfgMgr constructor:
 > Suggest the comment is strengthened to "must be set" and add something
 like "Note: the definition of DHCP_DATA_DIR needs to include the quotation
 marks".
 Done.

 Changes pushed in. Unless there are no new comments, I think it is ready
 for merge.

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