BIND 10 #2954: DHCP-DDNS: Create BIND 10 process skeleton
BIND 10 Development
do-not-reply at isc.org
Thu May 16 11:57:34 UTC 2013
#2954: DHCP-DDNS: Create BIND 10 process skeleton
-------------------------------------+-------------------------------------
Reporter: stephen | Owner:
Type: enhancement | marcin
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 => marcin
Comment:
Replying to [comment:8 marcin]:
> Replying to [comment:5 tmark]:
> > Replying to [comment:4 marcin]:
> > > Reviewed commit e448bbba3ecae68b261612954aa9777edc384be4
>
> Reviewed commit b3dda76df66adecf3af2ba419a23b4169bfe9541
>
> The doxygen documentation is not generated for D2 module. You should
update !''bind10/doc/Doxygen!'' file with the path to D2 and verify that
there are no errors generating D2 documentation:
> {{{
> cd doc
> make devel
> less html/doxygen-error.log
> }}}
>
Done. Did not know to do this.
> > >
> > > I have made this comment some time ago via email. I believe that in
D2Srv, the !''Srv!'' is redundant or even wrong. Please note that none of
the modules in BIND10 are called like this. For example, b10-dhcp4 is
indeed a server process but it is called b10-dhcp4. Perhaps, simply using
d2 would be sufficient. If we are not concerned about the length of the
name of this module, we could even try !''dhcpddns!''. Even in this stub
implementation, there is a lot of !''Srv!'' in the code already.
Programmers are lazy by their nature, so writing !''Srv!'' everywhere
might be a pain for some people :-).
> > >
> >
> > After discussing the issue with the the Dhcp team consensus to remove,
"Srv" and "srv" throughout. Attempting to work in dhcp-ddns or
permutations thereof is cumbersome at best. The module will be "b10-d2"
and files renamed as follows:
> >
> > bind10/src/bin/d2
> > Makefile.am
> > b10-d2.xml
> > d2.spec
> > d2_log.cc
> > d2_log.h
> > d2_messages.mes
> > main.cc
> > spec_config.h.pre.in
> >
> > tests/
> > Makefile.am
> > d2_test.py
> > d2_unittests.cc
> >
> >
> > > '''b10-d2srv.xml'''
> > > Copyright date should be corrected in file header.
> > >
> > > Errors in the following tags:
> > > - refentryinfo - invalid date
> > > - refnamediv - refpurpose is invalid - copied from DHCP module
> > > - docinfo - invalid date
> > > - cmdsynopsis - the (-v) option is not supported right now, so there
is no reason to document it.
> > > - refsynopsisdiv - same as above with -v option
> > >
> >
> > Corrected.
>
> Under !''cmdsynopsis!'' the !''-s!'' command line option is not
documented.
>
Added.
> >
> >
> > > '''d2srv_log.cc'''
> > > Invalid copyright date.
> > >
> > > Comment before !''include!'' does not match the module. We define
the logger for !''d2!'', not b10-dhcp4.
>
> This hasn't been corrected yet.
>
Done.
> > >
> > > '''d2srv_log.h'''
> > > Copyright date is invalid.
> > > Comment before !''extern!'' is copy-pasted from DHCP4 and thus
invalid in this context.
>
> This hasn't been corrected.
Done
>
> > >
> > > '''d2srv_messages.mes'''
> > > Invalid copyright date.
> > >
> >
> >
> > Corrected.
>
> There are spurious whitespaces in the file at the end of
> {{{
> It lists some information about the parameters with which the
> }}}
>
> and
>
> {{{
> This is a debug message issued when a D2 process shuts down
> }}}
>
>
Done
> >
> > > D2SRV_STARTING & D2SRV_START_INFO: Shouldn't it be
!''D2Controller!'', instead of !''DController!''?
> > >
> >
> > Corrected.
> >
> > > D2SRV_START_INFO: Do we really plan to have the ability to run the
server in a standalone mode? If we do, how do we get the configuration?
> > > BTW, if we decide that we need it (for example to run unit tests),
it should be documented in b10-d2srv.xml?
> >
> > I based this on dhcp4/6. Since they allegedly support "verbose" and
"stand-alone" flags, I presumed D2 should as well. It was my
understanding that we wanted D2 to be capbale of running without bind10.
I have been working under the assumption that the arguments claimed in
dhcp4 and dhcp6 function as advertised. Do they not?
>
> On reflection, I believe running the process without BIND10
infrastructure may be useful for testing. However, running it without the
infrastructure implies that you need to provide its configuration by other
means and we currently don't have any idea how that should be. The DHCP
modules used to be run in standalone mode when we didn't have any
configuration capabilities through BIND10. In this case, the server was
sending out the same fixed lease to everybody, but that was an early stage
of development.
>
> If we are planning to run the module standalone to test it, then that's
fine to keep the standalone capability.
I believe that it will be infinitely useful to run stand-alone, certainly
during development. I'll consider the configuration issue while
implementing the
controller and server classes.
>
> >
> > I added a comment stating that the parameters are preliminary.
>
> That's cool! The only sugggestion is that any of !''@todo!'', !''TODO!''
or !''todo!'' is added to this comment as a keyword. That way, you will be
easily able to grep the code for all outstanding issues that we decided to
change or fix at some point. We follow that in the DHCP modules.
Added @TODO
>
> >
> > >
> > > '''main.cc'''
> > > Invalid copyright date.
> > >
> >
> > Corrected.
> >
> > > usage(): what is the [-p number] parameter? It is not described
anywhere, including the error message printed by the usage(). In fact, any
attempt to specify this parameter would result in error and usage
printing.
> > >
> > bind10/doc
>
> > Removed reference to -p.
> >
> > > Shouldn't verbose mode be only available when standalone is turned
on? We control the verbosity by other means, i.e. using bindctl if running
a module using bind10.
Altered the call to initLogger to set log level to DEBUG only if both
verbose_mode and stand_alone are true.
> >
> > See my comment above regarding standalone mode.
> >
> > >
> > > main(): Why define the !''ret!'' variable? You could do:
> > > {{{
> > > return (EXIT_SUCCESS);
> > > }}}
> >
> > This was residual from the dchp4 version main.cc, which was more
involved
> > than this main is at the moment. I have cleaned it up a bit.
>
> Ok. Thanks.
> There are many spurious whitespaces in the code (comments).
>
Seriously? Ok, so I added an autocmd to my .vimrc to automagically strip
off
trailing whitespace from .cc and .h files.
> >
> > >
> > > '''Makefile.am'''
> > > Do we really have to link with all these libraries already? I would
be in favor to link against the minimal number of libraries and extend the
dependencies as we go. The libb10-dhcp++.la is already linked with the
number of libs, so I don't know whether explicitly adding these is really
needed.
> > >
> >
> > Again, residual
>
> It looks like the following is required:
> {{{
> b10_d2_LDADD = $(top_builddir)/src/lib/exceptions/libb10-exceptions.la
> }}}
>
> otherwise, linker fails.
>
It linked for me under OS-X, but I added the library anyway.
> >
> >
> >
> > > '''spec_config.h.pre'''
> > > Copyright date is invalid.
> > >
> > > The location of the spec file is also invalid.
> > >
> > > '''tests/Makefile.am'''
> > > The same comment regarding linking with many libraries as above.
>
> Can we remove some of the libraries, we are linking the test with?
>
Done.
> > >
> > > '''tests/d2srv_unittests.cc'''
> > > Copyright date in the header is invalid.
> > >
> > > '''tests/d2srv_test.py'''
> > > Copyright date is invalid.
> > >
> > > Do we actually have a ticket to implement !''test_alive!''?
Currently the comment says it is just a placeholder.
> > >
> >
> > I made the test functional.
>
> Great!
>
> >
> > > '''!ChangeLog'''
> > > If we wanted to follow the convention from the !ChangeLog we would
rather say:
> > > {{{
> > > 6xx. [func] tmark
> > > b10-d2srv: Initial DHCP-DDNS (a.k.a. D2) module implemented.
Currently it does
> > > nothing useful, except for providing the skeleton implementation
> > > to be expanded in the future.
> > > (Trac #2954, git TBD)
> > > }}}
> > >
> >
> > This wording is fine.
--
Ticket URL: <http://bind10.isc.org/ticket/2954#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list