BIND 10 #2957: DHCP-DDNS: Develop the DHCP DDNS configuration manager class
BIND 10 Development
do-not-reply at isc.org
Thu Jun 27 11:28:57 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
-------------------------------------+-------------------------------------
Changes (by stephen):
* owner: stephen => tmark
Comment:
== 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?)
'''src/bin/d2/d2_messages.mes'''[[BR]]
DCTL_ORDER_ERROR: message text should start with a lower-case character.
'''src/bin/d2/d_cfg_mgr.cc'''[[BR]]
DCfgMgrBase::parseConfig: s/Better to make hold/Better to hold/
DCfgMgrBase::parseConfig: in the "if" branch where parse_order_.size() >
0, the iterator "it" could be declared inside the BOOS_FOREACH loop.
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.
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());
}}}
'''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".
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.
!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).
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.
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.
'''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.
== 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.
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.
'''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?
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:
* 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?").
* Can the passed FQDN be empty?
* 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.
* 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.
DnsServerInfoParser::commit: s/paresed/parsed/
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");
}
}}}
(Note that "Dns Server" in the current message could be interpreted as
referring to the BIND 10 DNS server.)
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.)
DdnsDomainParser::commit(): spaces around the "=" sign.
'''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.)
!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.
!DnsServerInfo constructor: in the argument list, "port" could default to
standard_dns_port.
DdnsDomainListMgr::matchDomain header: s/the it will/it will/
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.)
DScalarContext copy constructor: should be a space before the "{".
'''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.
'''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.
'''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.
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.
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.
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".
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.
D2CfgMgr "construction" test: spaces around the "=" sign.
'''src/bin/d2/tests/d_test_stubs.h'''[[BR]]
s/configuraiton/configuration/
in fromJSON, need to remove the "cerr" output.
--
Ticket URL: <http://bind10.isc.org/ticket/2957#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list