BIND 10 #2956: DHCP-DDNS: Create server controller class
BIND 10 Development
do-not-reply at isc.org
Wed Jun 5 19:04:22 UTC 2013
#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
-------------------------------------+-------------------------------------
Changes (by tmark):
* owner: tmark => stephen
Comment:
PART I COMMENTS REPLY
Replying to [comment:6 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.
Thank you very much. Much thought did go into this and I actually did
wonder why this type of framework didn't already exist for BIND10. To me
it would have
been an essential part of that effort.
>
> '''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".
Gut and paste error. Fixed.
>
> '''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.
>
D2Controller derives from DControllerBase which derives from
boost::noncopyable.
> '''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.
>
Ok. Will also rename the files BIND10 looks for accordingly (such as spec
file).
> '''src/bin/d2/d2_messages.mes'''[[BR]]
> General: "it's" is short for "it is". "its" is used to indicate a
possession of "it".
>
Oops. Bad fingers! Bad!
> D2CTL_DISCONNECT_FAIL: explanation should be extended to state what is
the effect of this and will now happen. Will the process exit normally?
>
Done.
> (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.)
Will reorder as part of ticket #2978.
>
> '''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()
> }}}
>
Noted and corrected.
> and the second argument should start immediately below the first (at the
moment it's offset by a few characters). I
>
OMG. Fixed.
>
> '''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.
>
Actually, it should use IOService which is the isc::asiolink wrapper
class. Done.
> Header comments for DControllerBase should expand the description of
BIND 10 callbacks - in what circumstances are they used?
>
Added additional text to DControllerBase brief.
> 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.
Actually I did this so it would be possible to verify the result of launch
for test purposes. Have changed it to be a void method,
which throws a specific exception for the major failure types.
>
> 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.)
Right you are, but since I've replaced them with exceptions...
>
> 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.
>
Good point. Fixed.
> 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.
I wasn't consistent with this, so I have settled on std::string not
reference.
>
> DControllerBase::getXxxx()/isXxxx(): most of these methods can be
declared "const".
Done.
>
> DControllerBase::usage(): "&" should be attached to the "std::string".
Got it.
>
> Member variables: Descriptions should have a blank line before them and
no blank line after.
Yeah, I'm not sure what my fingers were doing on this one. Fixed.
>
> '''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.
>
Have altered setController to accept a shared ptr as above.
> 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.
>
I've removed the conditional compilation. Initially I had thought I could
force the logging levels to be verbose even when in integrated mode.
Turns out it doesn't work that way. Marcin cited in his review that
verbose was only meaningful if stand alone is true.
> launch() - see comments above regarding return status codes.
Corrected per earlier comments.
>
> 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));
> }}}
Got it.
> (Also, note use of "static_cast<char>()" for casting instead of
"(char)".
Done, but somewhere along the line all this safety-mindedness really took
the fun out of this. Lol.
>
> initProcess(): no space between "isc_throw" and "(".
>
Got it.
> commandHandler(): declaration of the second argument should be
immediately below the first.
>
GAAAAHHH! Fixed.
> 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.)
Good idea. Done.
PART II COMMENTS REPLY
Replying to [comment:7 stephen]:
> Second and final part of the review of commit
b222f7faadc7ed7898a936a2b97b2a385c6a2a1b
>
> '''src/bin/d2/tests/d2_controller_unittests.cc'''[[BR]]
> commandLineArgs test: use "const_cast<char*> instead of "(char*)" when
setting up argv.
>
Done.
> commandLineArgs test:: for flexibility, initialise argc to
(sizeof(argv)/sizeof(char*))
>
> (The two comments above also apply to the launchNormalShutdown test.)
>
>
Done.
>
> '''src/bin/d2/tests/d2_process_unittests.cc'''[[BR]]
> construction test: Should be no spaces between "THROW" and "(".
>
>
Sigh. Done.
>
> '''src/bin/d2/tests/d_controller_unittests.cc'''[[BR]]
> See comments for d2_controller_unittests regarding setting up command-
line arguments.
>
Got em.
> commandLineArgs test: should be a space around the "=" sign when setting
xopt. (Incidentally, that variable is probably better set via a memcpy.)
>
>
>
Added spaces. I'm not sure if memcpy is better, but I did do away with
sprintf.
>
> '''src/bin/d2/tests/d_test_stubs.{cc,h}'''[[BR]]
> DStubProcess::stub_proc_command_ (and other static strings): unless
there is a real need for this to be declared a string, it should be a
"const char*" to avoid a potential "static initialisation fiasco".
Alternatively, put the string as a static member inside a global "get"
method - it won't be initialised until the method is called, and that will
occur only after all other global variables have been initialised.
>
Changed to const char*.
> Should probably include "d2_log.h" in the .cc file. d2_logger is
explicitly referenced and, at the moment, the code relies on its
definition being included in files that are themselves included.
>
Done.
> DStrubController::customOption(): use "static_cast<char>" instead of
"(char)"
You'd make me wear a seat belt too wouldn't you? Done.
Changes committed and ready for re-review under commit
#21f3ea90672b5bf84886e58f08f97bbcc63dfd6d
--
Ticket URL: <http://bind10.isc.org/ticket/2956#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list