BIND 10 #2956: DHCP-DDNS: Create server controller class

[BIND 10 Development]

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

Comment (by stephen):

 Reviewed commit b222f7faadc7ed7898a936a2b97b2a385c6a2a1b

 This is the first part of the review.

 '''General'''[[BR]]
 Well done: you've put a lot of thought into this and it provides a good
 framework for all new BIND 10 processes.  After the DHCP/DDNS work is
 completed, I'd like to try to get this framework fully documented and
 included in the BIND 10 manual for programmers.  I think we should also
 try moving the existing processes onto this framework so as to reduce the
 amount of BIND 10 code.  This of course requires collaboration with the
 BIND 10 DNS folks.

 '''src/bin/d2/d2.spec'''[[BR]]
 The modified file's command description is "Shut down the stats httpd". It
 should read "Shut down the DHCP-DDNS process".

 '''src/bin/d2/d2_controller.h'''[[BR]]
 Class brief description: I would suggest stating "Process Controller for
 the DHCP-DDNS (D2) Process".  We need to think about future maintainers
 who may be confused as to what "D2" means.

 To maintain the integrity of the singleton, not only must the constructor
 be declared private, but the copy constructor needs to be declared private
 as well.

 '''src/bin/d2/d2_log.{cc,h}'''[[BR]]
 Since the service name is now defined in an extern variable, we may as
 well set it to the final name - which appears to be something line b10
 -dhcp-ddns.

 '''src/bin/d2/d2_messages.mes'''[[BR]]
 General: "it's" is short for "it is".  "its" is used to indicate a
 possession of "it".

 D2CTL_DISCONNECT_FAIL: explanation should be extended to state what is the
 effect of this and will now happen.  Will the process exit normally?

 (Just an aside, the tool tools/reorder_message_file.py will take a message
 file and order the entries in alphabetical order of message ID.)

 '''src/bin/d2/d2_process.cc'''[[BR]]
 D2Process::run(): in the "isc_throw" statement, the "(" should be flush
 with "isc_throw". In addition, in isc_throw, instead of writing
 {{{
 std::string("xxx") + ex.what()
 }}}
 the second argument can be put as
 {{{
 "xxx" << ex.what()
 }}}

 and the second argument should start immediately below the first (at the
 moment it's offset by a few characters).  I


 '''src/bin/d2/d_controller.h'''[[BR]]
 Header comments for DControllerBase uses both io_service (the class) and
 io_service_ (the variable).  I think the former should be used in all
 cases.

 Header comments for DControllerBase should expand the description of BIND
 10 callbacks - in what circumstances are they used?

 DControllerBase::launch(): is there a need to return all these status
 codes?  This is more a question of general error-handling philosophy
 rather than a particular issue with the code.  Returning an error code is
 useful in cases where:

 * the language does not support exceptions.
 * the code will be part of a library and the API requires information to
 be returned via an error code (e.g. a C++ library called from C).
 * a non-unusual condition can occur and the caller must be notified: in
 these cases, an exception may be too much of an overhead.

 In this case, the language does have exceptions and we do use them as a
 matter of course. For this reason, I suggest that launch() return a void
 and throw an exception be thrown in case of error.

 Just continuing the point about error-handling philosophy, the list of
 error codes reflect mostly fatal conditions which will cause the process
 to terminate.  So the reason to distinguish error conditions in the return
 status is to allow the caller to log the problem before exiting.  As this
 code is part of the server (as opposed to a general-purpose library) there
 is no reason not to allow the server to log the problem where it ocurs.

 DControllerBase::executeCommand(): strictly speaking the return status is
 isc::d2::xxxx, not D2::xxx.  (Note that I'm not suggesting exceptions here
 - we expect a commands to occasionally fail and the result of the command
 needs to be returned to the originator of the command: for this reason, a
 status return code is acceptable.)

 DControllerBase::createProcess() header: suggest that the term
 "application process object" is used rather than "application process".
 When I first read it, I thought that it was going to create a new process
 on the system.

 DControllerBase::getControllerName(): I think the de-facto standard in
 BIND 10 is to return a "std::string" instead of a "const std::string&"
 (e.g. look at the values returned by class methods in the src/lib/dns
 directory).  Admittedly this is a debatable point and so I'm not pushing
 for a change - a problem only arises if a caller explicitly declares a
 reference to the returned value and then deletes the containing object.

 DControllerBase::getXxxx()/isXxxx(): most of these methods can be declared
 "const".

 DControllerBase::usage(): "&" should be attached to the "std::string".

 Member variables: Descriptions should have a blank line before them and no
 blank line after.

 '''src/bin/d2/d_controller.cc'''[[BR]]
 setController(): although the usage is OK, it's slightly risky to pass a
 raw pointer to the method which then sets it in a shared pointer: it is
 feasible that the caller could delete it.  Either pass in a "const
 DControllerBasePtr&" variable or extend the header comments to explain
 what is done and what should not be done.

 setController(): if a raw pointer is kept, the value can be set in
 controller using the smart pointer's "reset" method - there is no need to
 create temporary object.

 launch(): in the "initLogger" does the first test need to be "verbose_ &&
 stand_alone_"?  Setting it to just "verbose_" will mean that the
 conditional "#if 1" can be removed.

 launch() - see comments above regarding return status codes.

 parseArgs(): should be a space after the comma in the isc_throw calls.

 parseArgs(): there is no need to create the "tmp" string when calling
 isc_throw - it is a macro and the string stream can be includes as the
 arguments, e.g.
 {{{
 isc_throw(InvalidUsage, "unsupported option: [" << static_cast<char>(opt
 opt) <<
                         "] " << (!optarg ? "" : optarg));
 }}}
 (Also, note use of "static_cast<char>()" for casting instead of "(char)".

 initProcess(): no space between "isc_throw" and "(".

 commandHandler(): declaration of the second argument should be immediately
 below the first.

 shutdown(): regarding the comments in shutdown() about not being sure
 about calling io_service::stop(), I would take it out but provide a method
 that does call it and require that the process class call it at the
 appropriate time - either in its shutdown() method or when the last I/O
 has completed. (As an aside, this is more flexible - the process can
 request a shutdown at any time.)

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