Opened 7 years ago

Closed 7 years ago

#2355 closed defect (fixed)

DHCP v4 and v6 parsers' unification should be considered

Reported by: tomek Owned by: tmark
Priority: medium Milestone: Sprint-DHCP-20130523
Component: ~dhcpconf(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

There's huge similarity between src/bin/dhcp4/config_parser.{cc|h} and its v6 counterpart.

Most of the code should be merged and moved to libdhcpsrv.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by tomek

Stephen wrote an interesting proposal, it should be considered:

Regarding the parser code, assuming that the same form of parsing is
used for both DHCP4 and DHCP6 (i.e. creating parsers for each
component), then common parsers can be hung off a base class and used by
both.

DhcpConfigParser
  |
Uint32Parser
StringParser
InterfaceListParser

The pool parser could also be common if it were templated:

PoolParser?<typename PoolX, typename PoolXPtr, typename creator>

... where X is 4 or 6. "creator" is a stand-alone function with two
signatures:

PoolXPtr creator(string, len)
PoolXPtr creator(IOAddress, IOAddress)

This function could itself be templated:

template <typename Arg1Type, typename Arg2Type>
Pool6Ptr Pool6Creator(Arg1Type arg1, Arg2Type arg2) {
    return (Pool6Ptr(new Pool6(Pool6::TYPE_IA, arg1, arg2)));
}

The same could also go for SubnetParser?. Of course, the problem comes in
issues like the call to createSubnet6ConfigParser etc. I think that
could be solved using an inheritance hierarchy like:

DhcpConfigParser
    :
SubnetParser<pool6 parameters>
    :
Subnet6Parser

A little restructuring could put the common things (like the parsing the
subnet specification) in the SubnetParser? class, and the specific things
(parsing subnet-specific parameters) in the derived class.

Stephen

comment:2 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130328

comment:3 Changed 7 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:4 Changed 7 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

This initial commit is an interim step in the refactoring effort to unify
DHCP4 and DHCP6 parser code. It moves the bulk of the parser classes formerly
declared in src/bin/dhcp4/config_parser.cc and src/bin/dhcp4/config_parser.cc
into two new files in lib/dhcpsrv:

dhcp_parsers.h and dhcp_parsers.cc

A few classes remain, that provide further opportunity for refactoring
In order to reduce the amount of change to review further refactoring
and unit tests will be handled as additional commits.

The primary challenge in developing common parser classes is the use
of references to global storage structures (e.g. uint32_values, string_values,
option_def_intermediate) inside the parser implementations. These globals
provide the outermost storage context and are specific to the server
instance itself. Removing the need for these references was essential.

For the most part, these references establish a default storage
for parser instances which may then be altered with a subsequent call to
the parser's "setStorage" method. Analysis of the current implementation
reveals that the storage context required for a particular parser is
known at the time the parser is instantiated and that parsers do not
migrate from one storage context to another. Therefore passing the
storage value as a parameter into the constructor seems appropriate.

The current implementation, separates the parser construction
and setting the storage context into separate stages. It first uses a map
of parser factory methods to instantiate the parser, and then in a subsequent
step uses dynamic-cast driven decision-making to set the parser's storage (See
Subnet4ConfigParser build, createSubnet4ConfigParser, and buildParser methods).

This technique was replaced with a more direct approach. createSubnet4ConfigParser
now constructs the parsers directly (not via factories), passing the appropriate
storage context into the constructor. This eliminates the need for buildParse
function template, and reduces the code in the build() method.

The use of factory-map mechanism used in the function createGlobalDhcp4ConfigParser()
as been similarly modified. This was code was also replaced to allow
use of the new parser constructors to set the parser storage.

The refactored parsers all define a private default constructors, and enforce
that the storage parameter passed into their public constructor not be NULL.

These same changes were applied the dhcp6.

This commit is # 53ceb71e36d310a2a4cd5fef7ae9fa5d410f078a

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit 53ceb71e36d310a2a4cd5fef7ae9fa5d410f078a.

src/bin/dhcp4/config_parser.cc
Please start comments with a capital letter.

Subnet4ConfigParser constructor: all parameters (even unused ones) should be listed in the header.

Subnet4ConfigParser::createSubnet4ConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

Subnet4ConfigParser::createSubnet4ConfigParser: please extend the @return comment in the header to note that the caller is responsible for deleting the returned parser once it is no longer needed.

configureDhcp4Server: there are several debug statements outputting to cout here.

Dhcp4OptionDataParser methods: "return" statements should enclose the returned value in parentheses.

createGlobalDhcp4ConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

src/bin/dhcp4/config_parser.h
Dhcp4OptionDataParser constructor: all arguments should be documented.

Dhcp4OptionDataParser constructor and "factory" method: attach the asterisk of a pointer declaration to the data type rather than the variable (i.e. use "type* var" rather than "type *var" - see below).

Dhcp4OptionDataParser: Do you need to define a private default constructor if a non-default one has been defined?

src/bin/dhcp6/config_parser.cc
Please start comments with a capital letter.

PoolParser constructors: arguments (even unused arguments) should be described in the header.

Subnet6ConfigParser constructor: arguments (even unused arguments) should be described in the header.

Subnet6ConfigParser::createSubnet6ConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

Subnet6ConfigParser::createSubnet6ConfigParser: please extend the @return comment in the header to note that the caller is responsible for deleting the returned parser once it is no longer needed.

Dhcp6OptionDataParser methods: "return" statements should enclose the returned value in parentheses.

Subnet6ListConfigParser constructor: arguments (even unused arguments) should be described in the header.

Dhcp6OptionDataParser methods: "return" statements should enclose the returned value in parentheses.

createGlobalDhcpConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

createGlobalDhcpConfigParser: for symmetry, should consider renaming this createGlobalDhcp6ConfigParser.

src/bin/dhcp6/config_parser.h
Try breaking lines at 80 columns - there are only a couple of places where this is violated.

Dhcp6OptionDataParser constructor and "factory" method: attach the asterisk of a pointer declaration to the data type rather than the variable (i.e. use "type* var" rather than "type *var" - see below).

Dhcp6OptionDataParser constructor: all arguments should be documented.

Dhcp6OptionDataParser: Do you need to define a private default constructor if a non-default one has been defined?

src/lib/dhcpsrv/dhcp_parsers.h
BooleanParser, !Uint32Parser and StringParser appear to be identical apart from the data type of the storage_ member (and XxxStorage argument to the constructor). They could be replaced by a template class plus a set of typedefs.

Multiple declarations: the de-facto standard when declaring pointers is to attach the asterisk to the data type, e.g. use

Uint32Storage* storage;

instead of the C-standard:

Uint32Storage *storage;

(This fails if you declare multiple variables on the same line, but our standard is not to do that.) This happens at various points in the code.

XxxParser constructor: second argument should be described in the header. Since a raw pointer is being passed, the description should also describe who is responsible for destropying the pointed-to object and whether the storage object has to remain in scope while an object derived from the parser class is active.

OptionDataParser constructor: all parameters must be described in the header.

OptionDataParser::commit: suggest wrapping the header comments at 80 characters.
src/lib/dhcpsrv/dhcp_parsers.h

Multiple classes: if a non-default constructor has been defined, do you need to define the default constructor? Isn't that only added if no constructor is present?

src/lib/dhcpsrv/dhcp_parsers.cc
XxxParser constructor: as the logic of checking whether the parameter name is empty is the same in each of the specialisations, putting the check into a private method of the template class would avoid the repetition of the error message in the code.

XxxParser::commit(): these are all the same, so can be in the general template class.

XxxParser::build(): these can be specialisations of the template.

comment:7 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

This update includes the new changes as well as corrections to Stephen's review comments. First the new changes:


Replaced individual global storage variables, with single instance of new ParserContext? class. This is passed down into parsers rather than have them access global variables.

Created Abstract classes for PoolParser?, SubnetConfigParser? in dhcpsrv, Added support of Dhcp4/subnet/interface configuration parameter. (Note, this allows it to be specified, does not implement listening on it, same as with Dhcp6, that needs to be done under a new ticket)

Replaced use of raw pointers in most of the code. Added Element type checks to Boolean and String parsers, corrected some commentary.

Added basic parsers units tests in new file, dhcp_parsers_unittest.cc. There is preliminary support for higher order parsers (OptionDataParser? ,OptionDefinitionParser?), These could be expanded with more permutations.

Heavier testing of the higher order parsers requires more thorough integration as they are reliant on each other as well as server-specific derivations which must be replaced with suitable stubs. The tests that do exercise this level of testing exist as unit tests under both DHCP servers, however they are tied tightly to the server-specific derivations.


Now the review response:


Replying to stephen:

Reviewed commit 53ceb71e36d310a2a4cd5fef7ae9fa5d410f078a.

src/bin/dhcp4/config_parser.cc
Please start comments with a capital letter.

Fixed.

Subnet4ConfigParser constructor: all parameters (even unused ones) should be listed in the header.

Fixed.

Subnet4ConfigParser::createSubnet4ConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

Fixed.

Subnet4ConfigParser::createSubnet4ConfigParser: please extend the @return comment in the header to note that the caller is responsible for deleting the returned parser once it is no longer needed.

Fixed.

configureDhcp4Server: there are several debug statements outputting to cout here.

Fixed.

Dhcp4OptionDataParser methods: "return" statements should enclose the returned value in parentheses.

Fixed.

createGlobalDhcp4ConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

Fixed.

src/bin/dhcp4/config_parser.h

Note, I moved Dhcp4OptionDataParser definition into config_parser.cc

Dhcp4OptionDataParser constructor: all arguments should be documented.

Fixed.

Dhcp4OptionDataParser constructor and "factory" method: attach the asterisk of a pointer declaration to the data type rather than the variable (i.e. use "type* var" rather than "type *var" - see below).

Replaced use of raw pointers with boost::shared_ptrs.

Dhcp4OptionDataParser: Do you need to define a private default constructor if a non-default one has been defined?

Have removed private default constructors throughout.

src/bin/dhcp6/config_parser.cc
Please start comments with a capital letter.

Done.

PoolParser constructors: arguments (even unused arguments) should be described in the header.

Done.

Subnet6ConfigParser constructor: arguments (even unused arguments) should be described in the header.

Done.

Subnet6ConfigParser::createSubnet6ConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

Done.

Subnet6ConfigParser::createSubnet6ConfigParser: please extend the @return comment in the header to note that the caller is responsible for deleting the returned parser once it is no longer needed.

Done.

Dhcp6OptionDataParser methods: "return" statements should enclose the returned value in parentheses.

Done.

Subnet6ListConfigParser constructor: arguments (even unused arguments) should be described in the header.

Done.

Dhcp6OptionDataParser methods: "return" statements should enclose the returned value in parentheses.

Done.

createGlobalDhcpConfigParser: in the "else if" block, the "else" should be on the same line as the preceding closing brace, i.e. "} else if (...) {"

Done.

createGlobalDhcpConfigParser: for symmetry, should consider renaming this createGlobalDhcp6ConfigParser.

Done. (FYI, I didn't pick this name ;))

src/bin/dhcp6/config_parser.h

As with dhcp4, I moved Dhcp6OptionsDataParser definition into config_parser.cc

Try breaking lines at 80 columns - there are only a couple of places where this is violated.

Done. (It wasn't me!)

Dhcp6OptionDataParser constructor and "factory" method: attach the asterisk of a pointer declaration to the data type rather than the variable (i.e. use "type* var" rather than "type *var" - see below).

Replaced use of raw pointers with boost::shared_ptrs.

Dhcp6OptionDataParser constructor: all arguments should be documented.

Dhcp6OptionDataParser: Do you need to define a private default constructor if a non-default one has been defined?

Private default constructors removed throughout.

src/lib/dhcpsrv/dhcp_parsers.h
BooleanParser, !Uint32Parser and StringParser appear to be identical apart from the data type of the storage_ member (and XxxStorage argument to the constructor). They could be replaced by a template class plus a set of typedefs.

Will address this under ticket a separate ticket to reduce the number of deltas for this ticket.

Multiple declarations: the de-facto standard when declaring pointers is to attach the asterisk to the data type, e.g. use

Uint32Storage* storage;

instead of the C-standard:

Uint32Storage *storage;

(This fails if you declare multiple variables on the same line, but our standard is not to do that.) This happens at various points in the code.

This has been corrected throughout. Most instances were replaced with switch to using boost::share_ptrs instead of raw pointers anyway.

XxxParser constructor: second argument should be described in the header. Since a raw pointer is being passed, the description should also describe who is responsible for destroying the pointed-to object and whether the storage object has to remain in scope while an object derived from the parser class is active.

This has been corrected throughout. Most instances were replaced with switch to using boost::shared_ptrs instead of raw pointers. There are only a few uses of raw pointers remaining that are part of the original implementation. These are creating parser and definition instances that are immediately wrapped in shared_ptrs, so they appear to be safe.

OptionDataParser constructor: all parameters must be described in the header.

Fixed.

OptionDataParser::commit: suggest wrapping the header comments at 80 characters.
src/lib/dhcpsrv/dhcp_parsers.h

Done.

Multiple classes: if a non-default constructor has been defined, do you need to define the default constructor? Isn't that only added if no constructor is present?

Private default constructors have been removed throughout.

src/lib/dhcpsrv/dhcp_parsers.cc
XxxParser constructor: as the logic of checking whether the parameter name is empty is the same in each of the specialisations, putting the check into a private method of the template class would avoid the repetition of the error message in the code.

XxxParser::commit(): these are all the same, so can be in the general template class.

XxxParser::build(): these can be specialisations of the template.

This applies primarily to the simple value type parsers (Uint32,Boolean,String). This effort will be done under a separate ticket to reduce the scope of deltas for this ticket.

comment:8 Changed 7 years ago by tmark

Additional check-in was needed. I had added a test in StringParser::build to throw a BadValue? exception if the Element "type" of the value Element was not "string". This broke the "versionTest" unit test in DHCP4 and DHCP6. It appears we rely on being able to set a string
configuration value from any Element type. This check was removed.

comment:9 Changed 7 years ago by tmark

Eariler comments cited replacing simple parsers with templated class as planned for a separate ticket. That effort turned out to be rather small and has been included as part of this ticket with an additional commit.

comment:10 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

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.

A review of the code led to the following comments:

src/bin/dhcp4/config_parser.cc
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.)

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?

Dhcp4OptionDataParser::factory - typo in the header: s/fo/of/

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?

Pool4Parser::poolMaker - appears to be a missing line in the header description of the dummy parameter.

Pool4Parser::poolMaker - opening brace should be on same line as method definition.

Subnet4ConfigParser constructor: again, if the context is the global context, why pass it through as an argument?

Subnet4ConfigParser commit: the dynamic pointer cast could return null if subnet_ is an incorrect type. Perhaps throw an exception in this case?

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.)

Subnet4ListConfigParser constructor: typo in header s/ingored/ignored/

src/bin/dhcp6/config_parser.cc
Similar comments to dhcp4/config_parser.cc

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?

src/lib/dhcpsrv/dhcp_parsers.cc
The BIND 10 standard for function definitions is to put the return type on the line preceding the definition (see examples in the coding guidelines. (This refers to the declaration of the ValueParser specialisations.)

src/lib/dhcpsrv/dhcp_parsers.h
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?

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.

It would be more logical (and easier to read) if the class ValueStorage were defined earlier in the file, before ValueParser.

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?

PoolParser header. s/Otherwise exception/Ownerwise an exception/

src/lib/dhcpsrv/subnet.{cc,h}
Just as an aside, I would consider the the getIface() and setIface() methods sufficiently trivial to be put in the header.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
booleanParserTest: after asserting that the parser.build() method works correctly, it would be logical to test that the value has been set correctly.

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, "!".

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.

uint32ParserTest: after building the parser with various values, it would be worth checking what the parser thinks the value is at that point.

comment:11 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

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
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
The BIND 10 standard for function definitions is to put the return type on
the line preceding the definition (see examples in the
coding guidelines. (This refers to the declaration
of the ValueParser specialisations.)

Fixed.

src/lib/dhcpsrv/dhcp_parsers.h
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}
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
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)

comment:12 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20130509 to Sprint-DHCP-20130523

comment:13 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit 36f7b7fa7bddd07da90c9346d5d62dbed77c2a49.

On Ubuntu 11.10 (building with g++ 4.6.1), I got a failure in the unit

tests:

:

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?

For documentation purposes, the problem was down to the fact that the test failed in a 32-bit environment. The way the test was structured - trying to coerce the maximum uint32_t number plus one into a long - meant that it was invalid on a system where the maximum "long" value is less than the maximum "uint32_t" value.

A modification was added to skip the test on 32-bit systems. The rest of this review is on the updated code.

src/lib/dhcp4/config_parser.{cc,h}
Subnet4ConfigParser constructor - need to remove the documentation for the now non-existent second argument.

I think that globalContext() should be declared "ParserContextPtr& globalContext()" and pass back a reference to the internal pointer. (Yes, I know I got it wrong in my example.) The reason can be seen at lines 503-506: the requirement is to reset the global pointer to the original_context value. However, the pointer to the global context on which reset() is called is a copy of the static boost::shared_ptr declared in the globalContext() function, not the actual static instance itself. The reset() only affects the former, not the latter.

src/lib/dhcpsrv/tests/config_parser_unittest.cc
Can take out the "#if 0" block of code now that the method of getting global parser context has changed.

src/bin/dhcp6/config_parser.{cc,h}
Subnet6ConfigParser constructor - need to remove the documentation for the now non-existent second argument.

Subnet6ListConfigParser::build() - trailing space after "subnet" when creating the new parser.

globalContext() should be declared to return a reference.

src/lib/dhcpsrv/dhcp_parsers.cc
ParserContext::operator=(): I presume that the intermediate "tmp" object is created to overcome the problems of self-assignment (i.e. "a = a"). It would be faster to use a construct of the form:

if (this != &rhs) {
   // Do the assignment
   :
}

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
The CfgMgrTest constructor has a commented-out line of code.

comment:14 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

Replying to stephen:

Reviewed commit 36f7b7fa7bddd07da90c9346d5d62dbed77c2a49.

On Ubuntu 11.10 (building with g++ 4.6.1), I got a failure in the unit

tests:

:

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?

For documentation purposes, the problem was down to the fact that the test failed in a 32-bit environment. The way the test was structured - trying to coerce the maximum uint32_t number plus one into a long - meant that it was invalid on a system where the maximum "long" value is less than the maximum "uint32_t" value.

A modification was added to skip the test on 32-bit systems. The rest of this review is on the updated code.

src/lib/dhcp4/config_parser.{cc,h}
Subnet4ConfigParser constructor - need to remove the documentation for the now non-existent second argument.

Got it.

I think that globalContext() should be declared "ParserContextPtr& globalContext()" and pass back a reference to the internal pointer. (Yes, I know I got it wrong in my example.) The reason can be seen at lines 503-506: the requirement is to reset the global pointer to the original_context value. However, the pointer to the global context on which reset() is called is a copy of the static boost::shared_ptr declared in the globalContext() function, not the actual static instance itself. The reset() only affects the former, not the latter.

Good catch. Got it.

src/lib/dhcpsrv/tests/config_parser_unittest.cc
Can take out the "#if 0" block of code now that the method of getting global parser context has changed.

Got it. That does it, I'm writing a "checker" script to look for TKM and #if 0,
two things I use when working code, to make sure I don't miss them.

src/bin/dhcp6/config_parser.{cc,h}
Subnet6ConfigParser constructor - need to remove the documentation for the now non-existent second argument.

Got it.

Subnet6ListConfigParser::build() - trailing space after "subnet" when creating the new parser.

Done.

globalContext() should be declared to return a reference.

Done.

src/lib/dhcpsrv/dhcp_parsers.cc
ParserContext::operator=(): I presume that the intermediate "tmp" object is created to overcome the problems of self-assignment (i.e. "a = a"). It would be faster to use a construct of the form:

if (this != &rhs) {
   // Do the assignment
   :
}

Done.

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
The CfgMgrTest constructor has a commented-out line of code.

Done.

comment:15 Changed 7 years ago by stephen

Reviewed commit commit 2df5585f42f1af03b89204c7cc22569720a26ae4

Ready to merge.

comment:16 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

comment:17 Changed 7 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed

Changes merged in under commit 066c983e091831645218810fb921986a659ccb1b.
ChangeLog? entry 614 added.

Note: See TracTickets for help on using tickets.