Opened 7 years ago

Closed 7 years ago

#2269 closed task (fixed)

Configuration parser for DHCPv6 server

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20121018
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

Ticket #2238 covers implementation of a CfgMgr? that stores the configuration. This ticket covers actual parsing of the user configuration and translation to structures used in CfgMgr?.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:2 Changed 7 years ago by tomek

Ready for review.

As the parser code is somewhat non-obvious, I've created some additional documentation for it. See Developer's Guide, DHCPv6 section (or dhcpv6.dox).

Although the storage in CfgMgr? fully supports triplets (min/default/max values), that is not yet supported in the configuration (only a single value is always used). As this is not requested (a bonus type of feature), it is not implemented now. That would require non-trivial changes in bindctl to support triplets (if we wanted to add new field that allowed either integer or integer/integer/integer syntax). Alternatively, it would require supporting 3 optional parameters with complicated logic, which parameter is mandatory and which is optional in a given case. That would also have to take inheritance into account.

There used to be a single configuration parameter defined in spec file that allowed network interface name to be specified. Is now accepts list of interface names. Those values are still ignored as they were before.

Some of the parsers can be reused for DHCPv4. Right now they are in src/bin/dhcp6, but they will likely to be moved to libdhcpsrv in src/lib/dhcp, so they could be shared.

Current design (many classes defined in .cc) assumed that the parser code should not pollute headers. With the intention to move them to src/lib, that approach is likely to change. That will be done as part of #2270.

comment:3 Changed 7 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 5b9ab4bc5c038d3fe6e12a0cf90c33e5f54d0f45

Owing to the history (#2238 was merged into this ticket a couple of times during development), the review was conducted on the following deltas:

  • b8f538b9fcf70394e5737269d4bed660721c245d to fb86b2f215ca1a08440c5c200f22af7747c9de2a
  • e3cc1803c6fb2aa3373528b6095eb9cf6e8b24d1 to ff837bb1fc512920556a796c40cacc20407b165c
  • 680279e6e9ac7a15d23b8a4d6d03a391166a7c8b to 5b9ab4bc5c038d3fe6e12a0cf90c33e5f54d0f45

Some minor changes have been made and pushed to the repository.

doc/guide/bind10-guide.xml
I've made some minor corrections (typos etc.) and pushed, but this is really very good documentation - thank-you.

We need to explicitly state what addresses <address>/<prefix> corresponds to. (In the v4 case, we have talked about the lowest address and the highest address not being included in the address range. And in this example, although the document talks about 2001:db8:1::/64, it mentions that the network administrator uses addresses 2001:db8:1::1 through 2001:db8:1::ffff - why not use 2001:db8:1::0?)

src/bin/dhcp6/dhcp6.spec
The indentation of lines in the definition of "single-subnet6" was inconsistent (some lines indented a couple of spaces more than the preceding one). I've modified this and pushed.

src/bin/dhcp6/config_parser.cc
If DummyParser is for debugging, I would perhaps call it !Dhcp6DebugParser.

StringParser::build(): not clear why back-slashes are removed from the string value passed in.

StringParser (general): Use "StringStorage* storage" rather than "StringStorage * storage"

PoolParser::build(): require spaces around operators such as "+" and "-" in an expression.

PoolParser::build(): When parsing the string after the "/", the variable "num" is probably better called "prefixlen". Also, the comment talks about downcasting to a

PoolParser::build(): Downcasting to a uint8_t won't catch any errors (such as prefixlen > 127 or negative). The comment should indicate that such errors will be caught by the Pool6 constructor.

Subnet6ConfigParser::build(): In the BOOST_FOREACH block, nested if's would prevent the repeated pointer casting, i.e.

if (uintParser) {
    uintParser->setStorage(...);
} else {
    boost::shared_ptr<StringParser> stringParser = ...
    if (stringParser) {
        :
    } else {
        boost::shared_ptr<PoolParser> poolParser = ...
            :
    }
}

src/bin/dhcp6/config_parser.h
Need "\brief" Doxygen tag at the start of the first line of each method header.

build(): s/an data/a data/

build(): Header refers to "above example" which is not present.

build(): method not 4expected to be called more than once: is this more than once in the life of the object, or more than once in the life of the program?

createDhcp6ConfigParser(): is this more logically a static method of the Dhcp6ConfigParser class?

createDhcp6ConfigParser(): Not clear what the config_id actually is.

src/bin/dhcp6/ctrl_dhcp6_srv.cc
dhcp6ConfigHandler: comment seems truncated ("that should never happen...")

src/bin/dhcp6/dhcp6_messages.mes
Minor corrections to wording have been pushed. Incidentally, the python script "tools/reorder_message_file.py" will reorder the message file so that the message definitions are
in alphabetical order.

src/bin/dhcp6/tests/config_parser_unittest.cc
Dhcp6ParserTest: "Dhcp6Srv * srv_" should be "Dhcp6Srv* srv_"

comment:6 Changed 7 years ago by tomek

It was pointed out in #2237 that src/lib/dhcp/cfgmgr.h there is no definition for removeSubnets6().

comment:7 in reply to: ↑ 5 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Some minor changes have been made and pushed to the repository.

Thank you.

We need to explicitly state what addresses <address>/<prefix> corresponds to. (In the v4 case, we have talked about the lowest address and the highest address not being included in the address range. And in this example, although the document talks about 2001:db8:1::/64, it mentions that the network administrator uses addresses 2001:db8:1::1 through 2001:db8:1::ffff - why not use 2001:db8:1::0?)

Added a separate paragraph that explains it. It is worth noting that with v6 the problem is lesser than in v4 as the last address is not reserved for broadcast (no broadcasts in v6).

src/bin/dhcp6/dhcp6.spec
The indentation of lines in the definition of "single-subnet6" was inconsistent (some lines indented a couple of spaces more than the preceding one). I've modified this and pushed.

Thanks.

src/bin/dhcp6/config_parser.cc
If DummyParser is for debugging, I would perhaps call it !Dhcp6DebugParser.

Renamed to DebugParser?. That will make the work of merging v4 and v6 parsers easier.

StringParser::build(): not clear why back-slashes are removed from the string value passed in.

These are not black-slashes. \t stands for tabulation. We want to allow using spaces and tabs in the subnet and pool definitions. Some users may want to add spaces or tabs for aesthetic purposes.

StringParser (general): Use "StringStorage* storage" rather than "StringStorage * storage"

Fixed 2 such problems.

PoolParser::build(): require spaces around operators such as "+" and "-" in an expression.

Added.

PoolParser::build(): When parsing the string after the "/", the variable "num" is probably better called "prefixlen". Also, the comment talks about downcasting to a

Renamed to prefix_len. I think your comment was truncated. We can't use direct cast to uint8_t as
that will interpret first character as value. For example for 64 it will return ASCII code of '6'
and will actually throw exception because of following characters ('4' in this example).

PoolParser::build(): Downcasting to a uint8_t won't catch any errors (such as prefixlen > 127 or negative). The comment should indicate that such errors will be caught by the Pool6 constructor.

Added.

Subnet6ConfigParser::build(): In the BOOST_FOREACH block, nested if's would prevent the repeated pointer casting, i.e.

As configuration parsing is not performance critial, personally I prefer code simplicity over efficienc y in such cases. Nevertheless, changed the code as you suggested.

src/bin/dhcp6/config_parser.h
Need "\brief" Doxygen tag at the start of the first line of each method header.

build(): s/an data/a data/

A mistake copied over from auth :) Fixed it in dhcp6 and auth.

build(): Header refers to "above example" which is not present.

Removed references to example.

build(): method not 4expected to be called more than once: is this more than once in the life of the object, or more than once in the life of the program?

It is called once for for each object. Each object is created to parse one specific instance of configuration parameter. Clarified

createDhcp6ConfigParser(): is this more logically a static method of the Dhcp6ConfigParser class?
createDhcp6ConfigParser(): Not clear what the config_id actually is.

It's a bit awkward, but I just noticed that this function has no body and is not used anywhere. Removed :)

src/bin/dhcp6/ctrl_dhcp6_srv.cc
dhcp6ConfigHandler: comment seems truncated ("that should never happen...")

Finished that comment.

src/bin/dhcp6/dhcp6_messages.mes
Minor corrections to wording have been pushed. Incidentally, the python script "tools/reorder_message_file.py" will reorder the message file so that the message definitions are
in alphabetical order.

src/bin/dhcp6/tests/config_parser_unittest.cc
Dhcp6ParserTest: "Dhcp6Srv * srv_" should be "Dhcp6Srv* srv_"

Fixed.

Also renamed Dhcp6ConfigParser to DhcpConfigParser?. That will make unification of Dhcp4 and Dhcp6 parsers slightly easier.

Another thing changed is something Shawn pointed out in his #2140 review: tests should have comments about their purpose. For now, a simple comments will do, but we will want to make them more structured in the future. Created #2350 for that.

comment:8 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 1517f32c9e68baacfc68c2aab412b29045d846b9

Apart from the typo on line 376 of src/bin/dhcp6/config_parser.cc ("charater"), all is OK and it can be merged.

comment:9 Changed 7 years ago by tomek

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

Typo fixed, code merged.

Note: See TracTickets for help on using tickets.