BIND 10 #2355: DHCP v4 and v6 parsers' unification should be considered
BIND 10 Development
do-not-reply at isc.org
Mon May 6 17:10:15 UTC 2013
#2355: DHCP v4 and v6 parsers' unification should be considered
-------------------------------------+-------------------------------------
Reporter: tomek | Owner:
Type: defect | stephen
Priority: medium | Status:
Component: dhcpconf | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-DHCP-20130509
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 => stephen
Comment:
Responses to latest review comments:
> Comment:
>
> Reviewed commit f18722f2e329c939584ff1b45936eed172fb16e3.
>
> On Ubuntu 11.10 (building with g++ 4.6.1), I got a failure in the unit
> tests:
> {{{
> [ RUN ] DhcpParserTest.uint32ParserTest
> dhcp_parsers_unittest.cc:164: Failure
> Expected: parser.build(int_element) throws an exception of type
> isc::BadValue.
> Actual: it throws nothing.
> }}}
>
I do not get this failure on OS-X. A clean pull and build on Ubuntu VM,
also passes unit tests. Something is amiss with your environment?
> A review of the code led to the following comments:
>
> '''src/bin/dhcp4/config_parser.cc'''[[BR]]
> Rather than create the global variable global_context_ptr pointing to a
> parser context, it would be better to create a function that returns
the
> variable, i.e.
> {{{
> ParserContextPtr globalContext() {
> static ParserContextPtr global_context_ptr(new
> ParserContext(Option::V4));
> return (global_context_ptr);
> }
> }}}
> This avoids any possibility of the "static initialization fiasco" by
> deferring initialization of global_context_ptr until everything else
has
> initialized. (As an aside, this also eliminates the need for the
> getGlobalParserContext() function for unit tests.)
>
Done.
> !ParserContext: a non-default copy constructor has been provided. As
> these usually go hand-in-hand with the assignment operator, should one
be
> provided here? Or should at least an assignment operator be declared
> private to prohibit its use?
>
Good point. Created copy operator.
> Dhcp4OptionDataParser::factory - typo in the header: s/fo/of/
>
Fixed.
> Dhcp4OptionDataParser::constructor & factory - if the parameter is the
> global context for the DHCPv4 option data - and if that context is
given
> by the global symbol global_context_ptr - why pass it as an argument?
OptionDataParsers are created by OptionDataListParsers via the factory
method
and these invocations can occur at can occur at both the global and subnet
"contexts" levels, so they require the context as an argument.
>
> Pool4Parser::poolMaker - appears to be a missing line in the header
> description of the dummy parameter.
>
Fixed.
> Pool4Parser::poolMaker - opening brace should be on same line as method
> definition.
Oops.
>
> Subnet4ConfigParser constructor: again, if the context is the global
> context, why pass it through as an argument?
In this instance, yes it need not be passed in. Subnets can only be
created
at the global level.
>
> Subnet4ConfigParser commit: the dynamic pointer cast could return null
if
> subnet_ is an incorrect type. Perhaps throw an exception in this case?
Will now throw isc::exception::Unexpected if the cast fails.
>
> Subnet4ConfigParser::createSubnetConfigParser: minor thing - when
breaking
> a function argument list across lines, subsequent lines are aligned
under
> the character after the opening parenthesis, not under the parenthesis
> itself. (Also, the "config_id.compare" lines should line up.)
>
Broke out the white gloves for this one eh?
> Subnet4ListConfigParser constructor: typo in header s/ingored/ignored/
>
Fixed.
>
> '''src/bin/dhcp6/config_parser.cc'''
> Similar comments to dhcp4/config_parser.cc
>
Done.
> As an aside, a side-by-side comparison between dhcp4/config_parser.cc
and
> dhcp6/config_parser.cc shows a lot of similarities. Further merging of
> the code could be made - another ticket?
>
Yes, there are still similarities and one could go a bit further.
>
>
> '''src/lib/dhcpsrv/dhcp_parsers.cc'''[[BR]]
> The BIND 10 standard for function definitions is to put the return type
on
> the line preceding the definition (see examples in the
> [wiki:CodingGuidelines coding guidelines]. (This refers to the
declaration
> of the !ValueParser specialisations.)
Fixed.
>
>
> '''src/lib/dhcpsrv/dhcp_parsers.h'''[[BR]]
> Typos:
> s/paramaters/parameters/
> s/to option/to option/ (twice)
>
> !ParserContext: a copy constructor usually goes hand-in-hand with an
> assignment operator. If an assignment operator is explicitly not used,
> perhaps declarating it private?
Fixed, was noted earlier.
>
> !ValueParser - What does TKM mean in the "@brief" line.
>
> !ValueParser - header is wrong - this is a template class, not the
parser
> for the boolean type.
>
My initials are TKM. I use it as marker for things I need to come back
and
address. Such as added briefs. Looks like I started updating the brief
for
this class and got interrupted or distracted and didn't come back around.
I
usually grep everything for "TKM" before committing things. Missed this
one.
> It would be more logical (and easier to read) if the class
!ValueStorage
> were defined earlier in the file, before !ValueParser.
Moved ValueStore from dhcp_config_parser.h to dhcp_parsers.h.
>
> !OptionDataListParser constructor - variables should be aligned below
one
> another, not below the parenthesis.
>
> A change in several classes has been to alter the declaration of the
> storage for !XxxxStorage to !XxxxStoragePtr. Why the additional level
of
> indirection?
Ultimately, the storage for a parser's data upon commit, is a container
external to the parser itself. Originally, the parser data members for
storage were raw pointers. These were changed to smart pointers.
>
> !PoolParser header. s/Otherwise exception/Ownerwise an exception/
>
The comment cited above, is actually no longer true as it refers to
setStorage method which no longer exsits. I have deleted the comment.
>
> '''src/lib/dhcpsrv/subnet.{cc,h}'''[[BR]]
> Just as an aside, I would consider the the getIface() and setIface()
> methods sufficiently trivial to be put in the header.
>
Noted. The original auther put them in the cc file. I simply moved them
up to
the base class. I did however, fix the handful of lines that were over 80
and capitalized the first word of the comments.
>
> '''src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc'''[[BR]]
> booleanParserTest: after asserting that the parser.build() method works
> correctly, it would be logical to test that the value has been set
> correctly.
The internal value set by build is private and not accessible until commit
is
invoked. It tests it after the commit.
>
> booleanParserTest: "bool actual_value = ~test_value". The "~" operator
is
> a bit-wise complement operator. As both actual_value and test_value
are
> declared "bool", the correct operator is the "not" operator, "!".
Quite right.
>
> stringParserTest: after verifying that the parser builds with a non-
string
> element, it would be worth checking what the value in the storage
actually
> is after that step.
Added test.
>
> uint32ParserTest: after building the parser with various values, it
would
> be worth checking what the parser thinks the value is at that point.
>
Added test.
If a ChangeLog entry is wanted here, I would recommend:
6XX. [func] tmark
Refactored the configuration parsing code in src/bin/dhcp4
and src/bin/dhcp6 into shared parser class hierarchy in
src/lib/dhcpsrv. This changed no outward functionality.
(Trac #2355, git TBD)
--
Ticket URL: <http://bind10.isc.org/ticket/2355#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list