Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2956 closed enhancement (complete)

DHCP-DDNS: Create server controller class

Reported by: stephen Owned by: tmark
Priority: medium Milestone: Sprint-DHCP-20130606
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by stephen)

Create the class ControlledDhcpDdnsSrv as described in the DHCP DDNS Design.

Subtickets

Attachments (1)

d2classes_diagram.svg (25.3 KB) - added by tmark 7 years ago.
Class diagram for D2 Controller and Process classes

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by stephen

  • Description modified (diff)

comment:2 Changed 7 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

Changed 7 years ago by tmark

Class diagram for D2 Controller and Process classes

comment:3 Changed 7 years ago by tmark

I've created the initial, implementation DHCP-DDNS service controller class, D2Controller. In the original design this was referred to as CtrlDhcpDdnsSrv.
It plays the essentially the same role as designed: It is the "controlling"
object which creates the runtime environment for and manages the lifecycle of the application processr,D2Process (fromerly known as DhcpDdnsSrv).

Unlike the DHCP server top level classes where the controller derives from the server, D2Controller derives from a new abstract class, DControllerBase. This abstraction provides all of the services necessary to manage an application process class derived from DProcess. These services include:

Command line argument handling
Process instantiation and initialization
Support for stand-alone execution
Support for integrated operation as a BIND10 module (session management and
event handling)
Process event loop invocation and shutdown

The implementation is sufficient to run as a BIND10 module and supports
shutdown command.

New files added:

d_controller.h
d_controller.cc
d2_controller.h
d2_controller.cc

tests/d_test_stubs.h
tests/d_test_stubs.cc

tests/d_controller_unittests.cc
tests/d2_controller_unittests.cc

The following class diagram illustrates the lay of the land (this will be incorporated into
design/implementation documentation)

Class diagram for D2 Controller and Process classes

Recommended ChangeLog entry:

6xx. [func] tmark

Created the initial implementation of DHCP-DDNS service controller class,
D2Controller, and the abstract class from which it derives,DControllerBase. D2Controller manages the lifecycle and BIND10 integration of the DHCP-DDNS
application process, D2Process.

(Trac #2956, git TBD)

comment:4 Changed 7 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 7 years ago by stephen

Reviewed commit b222f7faadc7ed7898a936a2b97b2a385c6a2a1b

This is the first part of the review.

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

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Second and final part of the review of commit b222f7faadc7ed7898a936a2b97b2a385c6a2a1b

src/bin/d2/tests/d2_controller_unittests.cc
commandLineArgs test: use "const_cast<char*> instead of "(char*)" when setting up argv.

commandLineArgs test:: for flexibility, initialise argc to (sizeof(argv)/sizeof(char*))

(The two comments above also apply to the launchNormalShutdown test.)

src/bin/d2/tests/d2_process_unittests.cc
construction test: Should be no spaces between "THROW" and "(".

src/bin/d2/tests/d_controller_unittests.cc
See comments for d2_controller_unittests regarding setting up command-line arguments.

commandLineArgs test: should be a space around the "=" sign when setting xopt. (Incidentally, that variable is probably better set via a memcpy.)

src/bin/d2/tests/d_test_stubs.{cc,h}
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.

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.

DStrubController::customOption(): use "static_cast<char>" instead of "(char)"

comment:8 in reply to: ↑ 6 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

PART I COMMENTS REPLY

Replying to stephen:

Reviewed commit b222f7faadc7ed7898a936a2b97b2a385c6a2a1b

This is the first part of the review.

General
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
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
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}
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
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
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
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
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 stephen:

Second and final part of the review of commit b222f7faadc7ed7898a936a2b97b2a385c6a2a1b

src/bin/d2/tests/d2_controller_unittests.cc
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
construction test: Should be no spaces between "THROW" and "(".

Sigh. Done.

src/bin/d2/tests/d_controller_unittests.cc
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}
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

comment:9 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit 21f3ea90672b5bf84886e58f08f97bbcc63dfd6d

src/bin/d2/Makefile.am
Typo: s/b10-dchdp-ddns.xml/b10-dhcp-ddns.xml/

src/bin/d2/b10-dhcp-ddns.xml
Should replace all occurrences of "D2" with "DHCP-DDNS" for documentation.

You should probably add something to say that the process is started by BIND 10 depending on the configuration options, and that the command-line arguments are for testing purposes only.

src/bin/d2/d2.spec

Gut and paste error. Fixed.

The rest of us have to make do with the "Cut" function. You really are using an industrial-strength text editor, aren't you?

src/bin/d2/d2_log.cc
Typo: s/b10-dhpc-ddns/b10-dhcp-ddns/

src/bin/d2/d_controller.cc
In the isc_throw error messages, suggest a space after the terminating colon in the text string to make the error messages more readable, e.g. change:

"Application process initialization failed:" << ex.what()

to

"Application process initialization failed: " << ex.what()

(This occurs in several places.)

src/bin/d2/d_controller.h

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

Done.

A bit over-enthusiastic in adding "const" to the return value if that value is a primitive type:

../../../src/bin/d2/d_process.h:130:33: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
In file included from ../../../src/bin/d2/d2_controller.h:18:0,
                 from main.cc:17:
../../../src/bin/d2/d_controller.h:347:31: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
../../../src/bin/d2/d_controller.h:361:28: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
cc1plus: all warnings being treated as errors

ChangeLog
I didn't comment on the ChangeLog last time. It looks OK.

comment:10 in reply to: ↑ 9 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

Replying to stephen:

Reviewed commit 21f3ea90672b5bf84886e58f08f97bbcc63dfd6d

src/bin/d2/Makefile.am
Typo: s/b10-dchdp-ddns.xml/b10-dhcp-ddns.xml/

Got it.

src/bin/d2/b10-dhcp-ddns.xml
Should replace all occurrences of "D2" with "DHCP-DDNS" for documentation.

I can't believe I missed that. I replaced all the lower case -d2s.

You should probably add something to say that the process is started by BIND 10 depending on the configuration options, and that the command-line arguments are for testing purposes only.

Done.

src/bin/d2/d2.spec

Gut and paste error. Fixed.

The rest of us have to make do with the "Cut" function. You really are using an industrial-strength text editor, aren't you?

Of course, I am an American afterall. We always use sledge hammers. You should see the size of my mouse.

src/bin/d2/d2_log.cc
Typo: s/b10-dhpc-ddns/b10-dhcp-ddns/

src/bin/d2/d_controller.cc
In the isc_throw error messages, suggest a space after the terminating colon in the text string to make the error messages more readable, e.g. change:

"Application process initialization failed:" << ex.what()

to

"Application process initialization failed: " << ex.what()

(This occurs in several places.)

OMG... Fixed.

src/bin/d2/d_controller.h

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

Done.

A bit over-enthusiastic in adding "const" to the return value if that value is a primitive type:

../../../src/bin/d2/d_process.h:130:33: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
In file included from ../../../src/bin/d2/d2_controller.h:18:0,
                 from main.cc:17:
../../../src/bin/d2/d_controller.h:347:31: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
../../../src/bin/d2/d_controller.h:361:28: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
cc1plus: all warnings being treated as errors

Apparently g++ 4.2 doesn't catch this. Have g++ 6.0 on Debian and it does. Fixed em.

ChangeLog
I didn't comment on the ChangeLog last time. It looks OK.

Ok.

comment:11 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Apparently g++ 4.2 doesn't catch this. Have g++ 6.0 on Debian and it does. Fixed em.

... except for being a bit too enthusiastic about removing "const"s. All were removed, not just the first occurrence. I was suggesting changing:

const bool isStandAlone() const

to

bool isStandAlone() const

... as the compiler was objecting to the "const" on the return type, not the one designating that the method does not alter the contents of the class.

I don't need to see it again, please merge after restoring the trailing "const"s.

comment:12 Changed 7 years ago by tmark

  • Resolution set to complete
  • Status changed from reviewing to closed

Changes merged under git commit a41cac582e46213c120b19928e4162535ba5fe76. Note, I found
that I missed module renaming in a couple of spots, these changes are under commit 270d14dcd9c376ec207f61bb736eb6ef8e9b1a2d.

Added ChangeLog? entry 626.

comment:13 Changed 7 years ago by tmark

  • Component changed from dhcp to dhcp-ddns
Note: See TracTickets for help on using tickets.