BIND 10 #2955: DHCP-DDNS : Create DHCP-DDNS server class

[BIND 10 Development]

#2955: DHCP-DDNS : Create DHCP-DDNS server class
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130523
         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 tmark):

 * owner:  tmark => stephen


Comment:

 > Reviewed commit b70a49c18a153f40aaf129d6d3f25039e052e887
 >
 > '''src/bin/d2/Makefile.am'''[[BR]]
 > Suggest devoting a separate line to d_process.h - it fits in better with
 the idea of one line per module.

 Done.

 >
 > '''src/bin/d2/d2_messages.mes'''[[BR]]
 > Although we said we could use D2 in the source code, I don't think it's
 a good idea to use it in messages to the user, especially if the process
 will appear as something line b10-dhcp-ddns.  In this case, instead of D2
 a prefix for the messages of DHCPDDNS_ would be appropriate.  D2PRC is
 probably OK, although DHCPDDNS_PRC_ would be better.
 >

 The "D2_" prefix is being replaced in #2956, with D2CTL_. Logs emanating
 at the controller level would be D2CTL_ and those at the process level
 would be D2PRC_.

 Although not checked in yet, it looks like this at run time:

 2013-05-24 13:29:39.346 DEBUG [b10-dchp-ddns/35050] D2CTL_STARTING
 controller starting, pid: 35050
 2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddns/35050] D2CTL_INIT_PROCESS
 initializing application proces
 2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddns/35050] D2CTL_STANDALONE
 skipping message queue, running standalone
 2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddns/35050] D2CTL_RUN_PROCESS
 starting application proces event loop
 2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddsn/35050] D2PRC_RUN_ENTER
 process has entered the event loop.

 Since the module name is b10-dchp-ddns, having that again in the log is
 not much benefit.  I'd like to stick with the D2CTL_ and D2PRC_
 for now.

 > Message files should not include a ":" between the message identifier
 and the message text. Neither should the message end with a full-stop. The
 first token on the line is the identifier, everything else is the rest of
 the message.  The formatting is determined by the logging code.

 Not sure why I stuck the colons in there. They've been removed along with
 the full stops.

 >
 > D2_STARTING: perhaps better phrased as "DHCP DDNS client starting".
 Also, look at the other BIND 10 servers: although "starting" messages are
 debug messages, there is usually an informational "started" message.
 >

 Replaced with D2CTL_STARTING.

 > D2_SHUTDOWN: s/normal shutting down/normal shutdown/ Also note that the
 text description says "debug" but the message is logged at INFO severity.
 >

 Replaced with D2CTL_STOPPING.

 > D2PRC_SHUTDOWN:  s/normal shutting down/normal shutdown/
 >
 > D2PRC_SHUTDOWN: I think we should expand the description (and the
 preceding one) to explain the difference between the service and the
 process.  (And it's OK for a debug message to be fairly technical and say
 something like "This message is issued at a lower-level in the code than
 the DHCPDDNS_SHUTDOWN message: it indicates the shutdown command has been
 successfully transmitted to the D2Process object associated with the
 service.")

 Text should be clearer between the two.

 >
 > D2PRC_FAILED: This message has an argument passed to it, so the message
 text should include the argument placeholder "%1".

 Nice catch.

 >
 > '''src/bin/d2/d2_process.h'''[[BR]]
 > init(): as this is a concrete class, I would think that init actually
 does something rather than "likely" do something.

 It will eventually do stuff. There is a lot of concrete to pour yet.

 >
 >
 > '''src/bin/d2/d_process.h'''[[BR]]
 > Having both DProcess and D2Process are a bit confusing.  As the former
 is an abstract class for the latter, D2ProcessBase is perhaps better.
 However, as this is intended to be the base class for a managed
 asynchronous application, perhaps a better name is something like
 !AsynchAppManager, !AsynchAppManagerBase or something along those lines.
 If the name is changed, the name of the exception should also be changed
 to reflect it.
 >

 I thought about calling it an something like DApplication etc, but then
 figured the people would holler about it not being called a "Module"since
 it will plug into BIND10.   I thought about calling it a "DModule" but
 then figured Tomek would holler about using it outside of BIND10, so in
 the end a settled on DProcess.  I have renamed it DProcessBase to help
 distinguish it from D2Process.

 > Header typos:
 > s/asyncrhonous/asynchronous/
 > s/terms fo management/terms of management/
 >
 > run(): where are EXIT_SUCCESS and EXIT_FAILURE defined?  If only two
 status codes can be returned, perhaps it is better declared "bool"?
 >

 EXIT_SUCCESS and EXIT_FAILURE are apparently defined in <cstdlib>. I had
 to look them up myself. They're used in the dhcp4 and 6
 servers, so I thought, "Oh, I better use those".  In actuality, I am going
 to alter init, run, and shutdown to be voids and throw
 if there is an error, as part of #2956.  It is cleaner for the controller
 to handle them that way.

 > Rather than make the elements protected, a "getXxxx()" method is better:
 it allows access to the elements, but hides the implementation from all
 classes (including derived ones).
 >

 Done.

 > '''src/bin/d2/tests/d2_process_unittests.cc'''[[BR]]
 > Test normalShutdown and fatalErrorShutdown:
 > * spaces needed around multiplication: s/2*1000/2 * 1000/.
 > * suggest that the comment for setting up the timer mentions that the
 argument is expected to be milliseconds. (My first thought was that it
 wopuld be a long test!)
 > * watch out for the test on elapsed.seconds().  Can you guarantee that
 specifying a 2,000 millisecond wait will always result in a wait equal to
 or longer than that figure?  It would perhaps be better to get the elapsed
 milliseconds and verify that it is in the range (say) 1,995 to 2,100.

 No, I don't think I'd gaurantee it.  I did think about that before but it
 worked numerous times so I moved on.  However, in the interests of not
 having intermittent unit test failures, I have taken your suggestion and
 structured the tests to test within a range.

 >
 > '''!ChangeLog'''[[BR]]
 > This change will need a !ChangeLong entry, which will need approval.
 >

 How's this?

 6xx     [func]      tmark
     Created the initial, bare-bones implementation of DHCP-DDNS service
     process class, D2Process, and the abstract class from which it
 derives,
     DProcessBase. D2Process will provide the DHCP-DDNS specific event loop
     and business logic.
     (Trac #2955, git TBD)

 >
 > I think that's it...
 >
 > ... Oh, er, and it's good start, nice work :-)


 Why thank you, very kindly sir.

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