BIND 10 #2954: DHCP-DDNS: Create BIND 10 process skeleton
BIND 10 Development
do-not-reply at isc.org
Thu May 16 09:58:14 UTC 2013
#2954: DHCP-DDNS: Create BIND 10 process skeleton
-------------------------------------+-------------------------------------
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 marcin):
* owner: marcin => tmark
Comment:
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
}}}
> >
> > 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.
>
>
> > '''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.
> >
> > '''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.
> >
> > '''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
}}}
>
> > 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 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.
>
> >
> > '''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.
>
> 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).
>
> >
> > '''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.
>
>
>
> > '''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?
> >
> > '''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:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list