BIND 10 #2957: DHCP-DDNS: Develop the DHCP DDNS configuration manager class
BIND 10 Development
do-not-reply at isc.org
Mon Jul 1 15:24:09 UTC 2013
#2957: DHCP-DDNS: Develop the DHCP DDNS configuration manager class
-------------------------------------+-------------------------------------
Reporter: stephen | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp-ddns | Milestone:
Keywords: | Sprint-DHCP-20130703
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
-------------------------------------+-------------------------------------
Comment (by tmark):
General:
Initial check in, did not include TSIG key list in the spec file or
parsing. This came up in discussion with Stephen as part of this review.
I have added in the ability to configure and parse a list of TSIG keys.
IF a domain's "key_name" entry is NOT blank, that key_name must map to a
key int the TSIG key list.
Updated class diagrams are included at the end of this section to reflect
the new classes.
Replying to [comment:8 stephen]:
> == General ==
>
> First off, thanks for the excellent design - it certainly helped to
clarify the relationships between the classes and the rest of the system.
>
> As suggested, reviewed the branch in two stages. Some general points
first:
>
> * Bear in mind that the documents are processed by Doxygen to create the
developers' guide. If comment text has a particular structure that needs
to be preserved, the appropriate Doxygen tags should be used.
>
> * In general the "@brief" should be little more than a description of
the class. Extended text should be in a separate paragraph below it,
separated from it by a blank line. (The blank line serves to terminate the
"@brief" and start a new paragraph.)
>
> * When referrring to TSIG keys, I wasn't clear whether the system stored
the name of a key and separately a list of key names and their values, or
whether "key name" referred to TSIG key values. That should be clarified
somewhere.
>
>
> == Review of commit dac0b87d5b0466e3aaf0f49ec7b2362606c03415 ==
>
> '''src/bin/d2/d2_cfg_mgr.h'''[[BR]]
> D2CfgContext::clone: In the header comments it should be made clear that
the caller is responsible for deleting this object. (Perhaps
consideration should be given to returning a shared pointer to the object
as a way of ensuring this?)
>
I have altered the clone method(s) to return a DCfgContextBasePtr.
>
> '''src/bin/d2/d2_messages.mes'''[[BR]]
> DCTL_ORDER_ERROR: message text should start with a lower-case character.
>
Done.
>
> '''src/bin/d2/d_cfg_mgr.cc'''[[BR]]
> DCfgMgrBase::parseConfig: s/Better to make hold/Better to hold/
Got it.
>
> DCfgMgrBase::parseConfig: in the "if" branch where parse_order_.size() >
0, the iterator "it" could be declared inside the BOOS_FOREACH loop.
>
It could be, but it clutters the loop code a bit, and the life time of the
iterator isn't really much different in this case. In this case I'd pick
clarity over tightness of scope.
> DCfgMgrBase::parseConfig: shouldn't the elements in parse_order all be
in the map? At the moment if one isn't, it is ignored. At the very least
a debug message should be output.
>
In looking at this again I actually found a few issues in the logic in
general.
One of my unit tests includes a configuration with one, invalid element.
This case fell right through undetected. There was also a path through
whereby in error occurred it, roll back did not. So I have reworked. I
added an error to
catch the case you cited above.
> DCfgMgrBase::buildAndCommit(): don't need the "std::string" constructor
in the isc_throw. Just use a text string and the "<<" operator (e.g.
> {{{
> isc_throw(DCfgMgrBaseError, "could not build and commit: " <<
ex.what());
> }}}
>
This is what's known as a "brain fart". Fixed.
> '''src/bin/d2/d_cfg_mgr.h'''[[BR]]
> The comment in the header for DCfgContextBase concerning use of clone()
is a bit obscure. Rather than "Derivations must supply an implementation
of clone that calls the base class copy constructor as well as performing
whatever steps are necessary to copy its additional storage", it is
shorter and more accurate to say that "Derivations must supply an
implementation of clone that calls the derived class's copy constructor".
>
And this is why I am not an author.
> DCfgContextBase::clone() header: The "@brief" text is a bit long:
"Create a clone" is sufficient, the rest can go into a new paragraph after
a blank line. Also, the example code should be between @code and @endcode
tags.
Check.
>
> !ElementIdList: as this is the type of DCfgMgrBase::parse_order_ which
is described as a FIFO, this might be better defined as an std::list than
a std::vector (where removing an element from the front will lead to the
remaining elements in the vector being shifted up one place).
I settled on removing the word FIFO. Using a std::list and removing
entries as they are parsed, means the parse order would have to be
repopulated before each configuration update is parsed, rather than during
the construction of the singleton instance of the confugration manager.
>
> DCfgMgrBase header: check how the header appears in Doxgen. I suggest
that the call chain be presented as a numbered list, and the pseudo-code
for the parsing effort be put within a @code... at endcode pair of tags.
Also, separate paragraphs by blank lines as this signifies a paragraph
break to Doxygen.
>
> DCfgMgrBase header: In the call chain explanation, a brief description
of what "Controller" and "Process" are would be helpful.
>
In re-reading this, I didn't find the call chain to be of particular value
so I removed it. Added code tags and spacing as suggesetd.
> DCfgMgrBase header: I'm not fully clear about the parse order. In
particular, how does a derivation specify in which order the objects are
parsed? There is the addToParseOrder() method, but when is that called?
The explanation should be extended.
>
I added some commentary to addToParseOrder which explains that it should
be used in the derivation's constructor.
>
> '''src/bin/d2/tests/d_cfg_mgr_unittests.cc'''
> Test "construction": suggest that cfg_mgr is a boost::scoped_ptr and
that destruction of the DStubCfgMgr is tested by using
scoped_ptr.reset(). (If the call to getContext() were to thrown an
exception, the memory pointed to by cfg_mgr would be lost.)
>
> Test "construction": need spaces around the "=" sign in the
ASSERT_NO_THROW.
>
Done.
>
> == Review of commit ae6f706d85ae90ca971d2cfef2abfe7e5bc7856d ==
>
> '''src/bin/d2/d2_cfg_mgr.cc'''[[BR]]
> matchForward/reverse: there should be no space between "isc_throw" and
the following left parenthesis.
I don't why I keep doing that. Fixed.
>
> matchForward/reverse: using std::empty() to test for an empty string is
faster than a comparison against the empty string.
>
>
> '''src/bin/d2/d2_cfg_mgr.h'''[[BR]]
> getForward/ReverseMgr: there is probably not a lot of overhead in
returning a pointer rather than a reference to a pointer. However, if a
reference is returned the documentation must make clear that the reference
is valid only for as long as the !D2CfgCOntext object.
>
Removed use of reference, returns pointer now.
>
> '''src/bin/d2/d2_config.cc'''[[BR]]
> The setting of 0.0.0.0 for "empty_ip_str" seems to imply that this
configuration is only for IPv4. The documentation should clarify that
these classes work for both v4 and v6. Perhaps using the empty string ""
would be better?
>
IOAddress doesn't like "" and you cannot create an "uninitialized"
IOAddress. I believe this is used
elsewhere as well.
>
> DdnsDomainListMgr::setDomains/matchDomain: I got somewhat confused by
the terminology here. The method works on the domains_ object of type
> {{{
> typedef std::map<std::string, DdnsDomainPtr> DdnsDomainStorage;
> }}}
> The method checks the fqdn passed to it against various domains (first
member of the pair) to find the longest match. But the comments talk
about the second member of the pair as being a domain. (E.g. the header
says "The wildcard domain is specified as a domain whose name is '*', and
the function talks about the wildcard domain. But "*" is not put into
domains_ and wildcard_domain_ is a pointer to something else.) I'd
suggest explicitly distinguishing between "domain name" and "domain". Or
use "domain" and "domain servers". (For that reason, the class
"!DdnsDomain" may be better named "!DdnsDomainServers".) Other points:
>
Altered some names to perhaps clear things up a bit
DdnsDomainStorage is now DdnsDomainMap - this is a map of DdnsDomains
keyed by their domain name DdnsDomainPtrPair isnow DdnsDomainMapPair
I also see your confusion. Origianally, I stored DdnsDomains in a vector
and had to iterate over it. When I changed to using a map I didn't rework
setDomains properly. It should be much clearer now.
> * A FQDN ends with a ".". Where is this checked? As an aside, are the
list of search domains normalized on input (i.e. adding a trailing "." if
they don't have one?").
Currently this not enforced and there's no normalization as you describe.
This would fall into the todo category. FQDN entry validation should be
enforced during configuration parsing.
> * Can the passed FQDN be empty?
No.
> * Doesn't really make any difference for an int, but in general
"++object" is preferred to "object++" as it avoids the object making a
temporary copy of itself.
Ok.
> * If at the point the domain is found, instead of a "break" there is a
"return (true)", the while loop will only exit if "domain" is empty: in
this case, the subsequent "if" check on "domain" is not needed. However,
this will only work if it is guaranteed that all the pointers in the
domians_ list are not empty.
There should never be empty domains int the list. I restructured it.
>
> DnsServerInfoParser::commit: s/paresed/parsed/
Missed that during my spell check.
>
> DnsServerInfoParser::commit: use 'hostname.empty()' instead of 'hostname
== ""' (and similar expressions for other tests). Also, the two tests
could be combined with a less-specific message, e.g.
> {{{
> if (hostname.empty() == ip_address.empty()) {
> isc_throw(D2CfgError, "DDNS server must specify one or other of
hostname and IP address");
> }
> }}}
Sigh. I know but in other reviews you have wanted MORE specific messages.
I specifically wrote it this way for you. Now you're just being fickle.
I have reworked the test.
> (Note that "Dns Server" in the current message could be interpreted as
referring to the BIND 10 DNS server.)
Suggestions?
>
> DnsServerInfoParser::commit: being defensive, I suggest that serverInfo
should be an object of type !DnsServerInfoPtr and the !DnsServerInfo
created by the new be placed straight into it. (If any code is added
between the creation and push_back, and it generates an exception, the
allocated memory will be lost.)
Good point. Corrected.
>
> DdnsDomainParser::commit(): spaces around the "=" sign.
n/a due to prior comment.
>
>
> '''src/bin/d2/d2_config.h'''[[BR]]
> File header documenation: s/!DdnsDomainLiatParser/!DdnsDomainListParser/
>
> File header documentation: to make things clearer for the reader it
would be worth giving an example a valid configuration. (Enclose it
between @code and @endcode tags.)
Added a sample config, hopefully it helps.
>
> !DnsServerInfo: standard_dns_port does not need to be declared static.
The line
> {{{
> const int standard_dns_port = 53;
> }}}
> in a class should be enough to define the constant.
Corrected.
>
> !DnsServerInfo constructor: in the argument list, "port" could default
to standard_dns_port.
Done.
>
> DdnsDomainListMgr::matchDomain header: s/the it will/it will/
Check.
>
> DScalarContext: the class is sparse so the header should make clear that
this implements a concrete version of the base class by supplying a
"clone" method. (I had to look up the base class definition to find out
what was so special about this class.)
>
Check.
> DScalarContext copy constructor: should be a space before the "{".
>
Got it.
>
> '''src/bin/d2/d2_messages.mes'''[[BR]]
> DHCP_DDNS_NO_MATCH: in the explanation, the first time FQDN is used,
spell it out, i.e. "... which match the cited fully-qualified domain name
(FQDN)." Readers of the manual might be familiar with domain names but
not recognise the FQDN abbreviation.
>
Done.
>
> '''src/bin/d2/d_cfg_mgr.{cc,h}'''[[BR]]
> Why is there a static "optional_" member?. If this is a constant that
can be referenced outside the class, using "const" with a value is
sufficient. (In addition, such constants should be in capitals and not
have a trailing underscore). Also, if OPTIONAL is defined, REQUIRED
should be defined for completeness.
>
Ok, captilized, no underscore, and added REQUIRED.
>
> '''src/bin/d2/dhcp-ddns.spec'''[[BR]]
> Within the "ddns_dcomain" map, there is an element "key_name". If this
is the TSIG key itself, the name "tsig_key" would be better. If it is the
name of a key, then the .spec file should contain a separate map for TSIG
key name/key value.
>
It is the name of the TSIG key to use. This is a pattern I took directly
from BIND 9.
> The forward_ddns and reverse_ddns elements should be optional. It could
be that the user wants to set up one but not the other. If they are non-
optional, then the list of domains in them should be optional. The current
.spec file seems to indicate that at least one forward and reverse server
must be supplied.
>
The forward and reverse ddns elements are now optional, as is the new
tsig_keys section.
> command_description should be "Shuts down the DHCP DDNS server".
>
>
> '''src/bin/d2/tests/d2_cfg_mgr_unittests.cc'''[[BR]]
> Various places in the file: in EXPECT_EQ, the expected value should be
the first argument and the actual value the second. (Google Test's failure
messages expect this order.)
>
> Various places in the file: fromJSON returns false on error, so the
ASSERT_NO_THROW should be ASSERT_TRUE.
Oops. Fixed.
>
> Various places in the file: According to the
[http://bind10.isc.org/wiki/CodingGuidelines#TestingDocumentationaddressesandprefixes
BIND 10 coding guidelines], IP addresses used in tests should be in the
ranges 192.0.2.0/24 and 2001:db8::/32. In addition, names such as
example.com, .test, .example, etc. should be used for test domains.
>
> checkServer: in the first "if", the "{" should be on the same line as
the "if".
>
Got it. Given the number of opening braces involved, I'm doing pretty
good.
> ddnsDomainParsing: rather than declare parser as a "!DdnsDomainParser"
(and BTW, the "*" should be attached to the type, not the variable),
declare it as a boost::scoped_ptr. (As it is, "parser" does not appear to
be deleted.) This comment applies to other cases where "parser" is
declared.
>
This has been replaced by use of DdnsDominTest fixture class.
> D2CfgMgr "construction" test: spaces around the "=" sign.
Done.
>
>
> '''src/bin/d2/tests/d_test_stubs.h'''[[BR]]
> s/configuraiton/configuration/
>
> in fromJSON, need to remove the "cerr" output.
>
Done.
Configuration classes with new TSIG classes:
[[Image(d2_config_TSIG.svg)]]
Parser classes with new TSIG parsing classes:
[[Image(d2_parsers_TSIG.svg)]]
--
Ticket URL: <http://bind10.isc.org/ticket/2957#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list