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

[BIND 10 Development]

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

 * owner:  UnAssigned => tmark


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.

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

 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.

 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.

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

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

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

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


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

 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"?

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

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

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


 I think that's it...

 ... Oh, er, and it's good start, nice work :-)

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