Opened 3 years ago

Closed 3 years ago

#5039 closed enhancement (fixed)

Dhcp6: Migrate option values and definitions configuration

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea 1.2 - Mozilla Milestone 1
Component: remote-management Version: git
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: 2
Total Hours: 54 Internal?: no

Description (last modified by tomek)

This ticket covers step 4 of the plan laid out in #5036:

  1. Migrate option values and definitions configuration (OptionDefListParser, OptionDataParser, OptionDataListParser);

For overview see #5036. More details will become available when #5015 is done.

Note: This code uses C++11 structures. This code was branched from a master that didn't have C++11 enabled, thus there's a change in src/lib/dhcpsrv Makefile that forcibly enables C++11. This change will not be merged to master.

Subtickets

Change History (19)

comment:1 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

comment:2 Changed 3 years ago by tomek

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

comment:3 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 32
  • Description modified (diff)
  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 32

This ticket is now ready for review. Since this is the first ticket that introduces SimpleParser, there's quite a few of new functionalities. When reviewing the code, please also take a look at the SimpleParser page on wiki.

Proposed ChangeLog?:

11XX.	[func]		tomek
	SimpleParser is now implemented and option defintions and data
	have been migrated. This change simplifies the parser code.
	(Trac #5036, 5021, git tbd)

comment:4 Changed 3 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:5 follow-up: Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 32 to 6
  • Owner changed from marcin to tomek
  • Total Hours changed from 32 to 38

Reviewed commit 0bbc1df7d05e0434fa3918ac514793966f322a23.

In general, I think you have taken a very nice and well organized approach to deal with the parsing. I didn't find anything glaring in the SimpleParser implementation. But, I note that it is very DHCP centric. I mean that methods like setAllDefaults have boolean parameter v6 which distinguishes between the DHCPv4 and DHCPv6 case. But, isn't it meant to be more generic class that could/should be as well used for D2 and CA? In that sense, wouldn't it be better to have a base class (perhaps abstract) and have derivations (SimpleParserV4, SimpleParserV6, SimpleParserD2, SimpleParserCA) of that class that override virtual method like setAllDefaults and do what is appropriate within this context? Or maybe I should ask, how are we going to extend this class and its interface to handle defaults for other applications?

I also wonder if maybe the SimpleParser class shouldn't rather belong to the libkea-config or libkea-cc? This is honest question. *If* this is going to be a generic class that handles configuration of various modules/applications it should maybe be in some common location. Kea LFC app doesn't understand JSON configuration yet, but if it does I don't think it should link with the whole bloated libkea-dhcpsrv to just have access to SimpleParser.

src/bin/dhcp4/json_config_parser.cc
In configureDhcp4Server you now make a copy of the map element:

// This is a way to convert ConstElementPtr to ElementPtr.
// We need a config that can be edited, because we will insert
// default values and will insert derived values as well.
std::map<std::string, ConstElementPtr> values;
config_set->getValue(values);
ElementPtr mutable_cfg(new MapElement());
mutable_cfg->setValue(values);

Why don't you just make a change in the data.h to retrieve std::map<std::string, ElementPtr>, similar to what you did for listValue()? That would make Element interface more consistent.

If nothing else you can just move this logic to data.h/data.cc.

The comments for specific files follow.

src/bin/dhcp4/tests/host_options_unittest.cc
See the question below for host_unittest.cc

src/bin/dhcp6/json_config_parser.cc

The commented "else if" in line 884 should be removed.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
I am puzzled with this change:

diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
index b1a08cb..ac73374 100644
--- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
@@ -2024,10 +2024,12 @@ TEST_F(Dhcpv6SrvTest, rsooOverride) {
         "    \"option-def\": [ {"
         "      \"name\": \"foo\","
         "      \"code\": 120,"
+        "      \"csv-format\": false,"
         "      \"type\": \"binary\""
         "    } ],"
         "    \"option-data\": [ {"
         "      \"code\": 120,"
+        "      \"csv-format\": false,"
         "      \"data\": \"05\""
         "    } ],"
         "    \"preferred-lifetime\": 3000,"

Firstly, do we now support csv-format for option definitions? Or the parser doesn't throw exception having unrecognized parameter accidentally?

I realize that you specified csv-format because the new parser requires all parameters be set (at least this is what the docs say). But, the csv-format is actually a special case, which if not specified, leaves the system with a choice to interpret the data as CSV or binary, depending if the definition for this option exists or not. See section 7.2.12. Unspecified Parameters for DHCPv4 Option Configuration.

Now that you enforce setting all parameters I don't understand how the existing unit tests may pass, as some of them test this behavior... i.e. "no csv-format specified and let the system determine what to do". I've had the look into ParseConfigTest::optionDataCSVFormatNoOptionDef and I see that the tests were probably modified and don't really test this behavior. For instance line 922 "When csv-format is not specified...." followed by the config which includes csv-format.

Apparently you have changed that not so long ago:

f9abab4b (Tomek Mrugalski  2016-11-29 20:09:14 +0100 930)         "    \"csv-format\": false,"

If we get away from this behavior we should update the docs. But, we should also keep in mind that it may (though unlikely) break some existing configs.

src/bin/dhcp6/tests/host_unittest.cc
Similarly, I don't know why this change:

diff --git a/src/bin/dhcp6/tests/host_unittest.cc b/src/bin/dhcp6/tests/host_unittest.cc
index 4d0bba3..12440cd 100644
--- a/src/bin/dhcp6/tests/host_unittest.cc
+++ b/src/bin/dhcp6/tests/host_unittest.cc
@@ -229,7 +229,7 @@ const char* CONFIGS[] = {
         "\"renew-timer\": 1000, "
         "\"option-data\": [ {"
         "    \"name\": \"vendor-opts\","
-        "    \"data\": 4491"
+        "    \"data\": \"4491\""
         "},"
         "{"
...

Is the new parsing code making quotes optional for the "data" parameter? Up until now, the data parameter was always a string. It still to be a string in most cases.

src/lib/cc/data.cc
Copyright date to be updated.

src/lib/cc/data.h
getNonConst is not documented. Also, it could probably be a const function.

Copyright date should be updated.

src/lib/dhcpsrv/Makefile.am
I recommend that you remove the -std=c++11 hack now, because it will be easy to forget it when you merge. And, the C++11 is already supported on master.

Stephen would say that files should follow alphabetical order.

src/lib/dhcpsrv/libdhcpsrv.dox
The new text seems to be misplaced. We do have sections dedicated to configuration parsing under "DHCPv4 Server Component" and "DHCPv6 Server Component". Either this new text is moved there or the existing text is moved to libdhcpsrv. In any case, it should be somehow combined.

src/lib/dhcpsrv/parsers/dhcp_parsers.h
After your changes the OptionDefParser::address_family_ seems to be unused. Same goes for OptionDefListParser::address_family_.

src/lib/dhcpsrv/parsers/simple_parser.cc
You need copyright header for this file.

SetAllDefaults?: There is a commented call to:

setOptionListDefaults(options);

It should probably be uncommented and should replace the foreach above?

src/lib/dhcpsrv/parsers/simple_parser.h
You need copyright header for this file.

I note that the SimpleParser is a class which currently mostly has static methods and no virtual methods, so it doesn't make sense to have virtual destructor.

I also note that SimpleParser includes one non-static method getPosition which is odd given that the whole class is static.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
What is the reason for ParseConfigTest::globalDefaults4 test to be disabled?

comment:6 Changed 3 years ago by fdupont

To answer to one of the questions the #4036 grammar defines the value of "data" in "option-data" as unconditionally be a string even this string can encode booleans, numbers, etc.

comment:7 Changed 3 years ago by stephen

  • Milestone changed from Kea1.2 to Kea 1.2 - Mozilla Milestone 1

Moved to milestone "Kea 1.2 - Mozilla Milestone 1" as a result of Kea meeting on 15 December 2016

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

  • Add Hours to Ticket changed from 6 to 14
  • Owner changed from tomek to marcin
  • Total Hours changed from 38 to 52

Replying to marcin:

Reviewed commit 0bbc1df7d05e0434fa3918ac514793966f322a23.

isn't it meant to be more generic class that could/should be as well used for D2 and CA? In that sense, wouldn't it be better to have a base class (perhaps abstract) and have derivations (SimpleParserV4, SimpleParserV6, SimpleParserD2, SimpleParserCA) of that class that override virtual method like setAllDefaults and do what is appropriate within this context? Or maybe I should ask, how are we going to extend this class and its interface to handle defaults for other applications?

That's a very good point that I haven't thought about.

I also wonder if maybe the SimpleParser class shouldn't rather belong to the libkea-config or libkea-cc? This is honest question. *If* this is going to be a generic class that handles configuration of various modules/applications it should maybe be in some common location. Kea LFC app doesn't understand JSON configuration yet, but if it does I don't think it should link with the whole bloated libkea-dhcpsrv to just have access to SimpleParser.

Agree on all points. I moved SimpleParser to libkea-cc and removed all DHCP-specific elements. There is are now two classes derived from it - SimpleParser4 (in src/bin/dhcp4) and SimpleParser6 (in src/bin/dhcp6), but they're mostly wrappers around the default values.

src/bin/dhcp4/json_config_parser.cc
In configureDhcp4Server you now make a copy of the map element:

// This is a way to convert ConstElementPtr to ElementPtr.
// We need a config that can be edited, because we will insert
// default values and will insert derived values as well.
std::map<std::string, ConstElementPtr> values;
config_set->getValue(values);
ElementPtr mutable_cfg(new MapElement());
mutable_cfg->setValue(values);

Why don't you just make a change in the data.h to retrieve std::map<std::string, ElementPtr>, similar to what you did for listValue()? That would make Element interface more consistent.

I tried, but the amount of code that requires changes (all the code related to parsing commands and unit-tests was the major pain) caused me to rethink this. If you're convince there's a value in that (note that ConstElementPtr? would be essentially obsolete after that change), we can pursue it, but probably as a separate ticket.

If nothing else you can just move this logic to data.h/data.cc.

That's much more preferable. There's ElementPtr? getMutableMap(ConstElementPtr?& const_map) call.

src/bin/dhcp4/tests/host_options_unittest.cc
See the question below for host_unittest.cc

That was a bug in the test. Note that data in option-data is expected to be a string and those tests specified that value as integer. Also, see Francis' comment 6 above.

src/bin/dhcp6/json_config_parser.cc

The commented "else if" in line 884 should be removed.

Removed.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
I am puzzled with this change:

diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
index b1a08cb..ac73374 100644
--- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
@@ -2024,10 +2024,12 @@ TEST_F(Dhcpv6SrvTest, rsooOverride) {
         "    \"option-def\": [ {"
         "      \"name\": \"foo\","
         "      \"code\": 120,"
+        "      \"csv-format\": false,"
         "      \"type\": \"binary\""
         "    } ],"
         "    \"option-data\": [ {"
         "      \"code\": 120,"
+        "      \"csv-format\": false,"
         "      \"data\": \"05\""
         "    } ],"
         "    \"preferred-lifetime\": 3000,"

Firstly, do we now support csv-format for option definitions? Or the parser doesn't throw exception having unrecognized parameter accidentally?

Oops, the first change is a mistake. I have reverted it now.

I realize that you specified csv-format because the new parser requires all parameters be set (at least this is what the docs say). But, the csv-format is actually a special case, which if not specified, leaves the system with a choice to interpret the data as CSV or binary, depending if the definition for this option exists or not. See section 7.2.12. Unspecified Parameters for DHCPv4 Option Configuration.

This (and corresponding section for v6) is now updated. It is now clear that if not specified, the default value of true is used.

Now that you enforce setting all parameters I don't understand how the existing unit tests may pass, as some of them test this behavior... i.e. "no csv-format specified and let the system determine what to do". I've had the look into ParseConfigTest::optionDataCSVFormatNoOptionDef and I see that the tests were probably modified and don't really test this behavior. For instance line 922 "When csv-format is not specified...." followed by the config which includes csv-format.

I have updated those tests. I admit that parts of them probably are less useful than they used to be.

Apparently you have changed that not so long ago:

f9abab4b (Tomek Mrugalski  2016-11-29 20:09:14 +0100 930)         "    \"csv-format\": false,"

If we get away from this behavior we should update the docs. But, we should also keep in mind that it may (though unlikely) break some existing configs.

I have updated the docs. Hopefully they're now clear about that behavior.

src/bin/dhcp6/tests/host_unittest.cc
Similarly, I don't know why this change:

diff --git a/src/bin/dhcp6/tests/host_unittest.cc b/src/bin/dhcp6/tests/host_unittest.cc
index 4d0bba3..12440cd 100644
--- a/src/bin/dhcp6/tests/host_unittest.cc
+++ b/src/bin/dhcp6/tests/host_unittest.cc
@@ -229,7 +229,7 @@ const char* CONFIGS[] = {
         "\"renew-timer\": 1000, "
         "\"option-data\": [ {"
         "    \"name\": \"vendor-opts\","
-        "    \"data\": 4491"
+        "    \"data\": \"4491\""
         "},"
         "{"
...

Is the new parsing code making quotes optional for the "data" parameter? Up until now, the data parameter was always a string. It still to be a string in most cases.

Take a closer look. This used to be "data": 4491 and is now "data": "4491". It was a bug in the test code that incorrectly used integer type. It's now correctly using string.

src/lib/cc/data.cc
Copyright date to be updated.

Updated.

src/lib/cc/data.h
getNonConst is not documented. Also, it could probably be a const function.

Documented and constified.

Copyright date should be updated.

Updated.

src/lib/dhcpsrv/Makefile.am
I recommend that you remove the -std=c++11 hack now, because it will be easy to forget it when you merge. And, the C++11 is already supported on master.

That would cause the code to not build until this change is either merged or rebased. I'd like to avoid rebasing as I'm afraid of conflicts.

Stephen would say that files should follow alphabetical order.

The simple_parser.cc|h is no longer in this directory, so that comment does no longer apply in this particular case. But I tried to add the new files in aphabetical order.

src/lib/dhcpsrv/libdhcpsrv.dox
The new text seems to be misplaced. We do have sections dedicated to configuration parsing under "DHCPv4 Server Component" and "DHCPv6 Server Component". Either this new text is moved there or the existing text is moved to libdhcpsrv. In any case, it should be somehow combined.

Since the parser was moved to libkea-cc, this text was moved there. As there is still the old code, I decided to keep it in those two sections you mentioned. Added references to the new text. This text will probably be updated as part of the work in specific tickets.

src/lib/dhcpsrv/parsers/dhcp_parsers.h
After your changes the OptionDefParser::address_family_ seems to be unused. Same goes for OptionDefListParser::address_family_.

Ok, that started a bit of domino effect. With the address_family_ removed, the constructor became a default one, so I did remove it too. This caused the whole class to be completely static.

src/lib/dhcpsrv/parsers/simple_parser.cc
You need copyright header for this file.

SetAllDefaults?: There is a commented call to:

setOptionListDefaults(options);

It should probably be uncommented and should replace the foreach above?

src/lib/dhcpsrv/parsers/simple_parser.h
You need copyright header for this file.

I note that the SimpleParser is a class which currently mostly has static methods and no virtual methods, so it doesn't make sense to have virtual destructor.

I also note that SimpleParser includes one non-static method getPosition which is odd given that the whole class is static.

It is static now.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
What is the reason for ParseConfigTest::globalDefaults4 test to be disabled?

The original reason was that it caused some tests to fail. Those tests are specific to global values, an area that has its own dedicated tickets. But since I started tweaking the code already, I went ahead, enabled the test and the underlying code and fixed the other tests. This is part of the #5019/#5037 work.

comment:9 Changed 3 years ago by fdupont

BTW don't forget to remove the -std=c++11 in Makefile.am files as mentioned in the ticket description before merging...

Last edited 3 years ago by fdupont (previous) (diff)

comment:10 Changed 3 years ago by fdupont

And some Element::fromJSON in DHCP simple parser unit tests should be updated using flex/bison appropriate parsers.

I fixed a few spelling errors and removed a ~ file so please pull.

comment:11 Changed 3 years ago by fdupont

  • Owner changed from marcin to fdupont

comment:12 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

I believe Marcin's concerns were addressed. For things to do near merging, the c++11 must be removed before, fromJSON can be updated after. Note I asked in #5019 some guide lines for next steps...

comment:13 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 14 to 2
  • Total Hours changed from 52 to 54

Thanks a lot, Francis. The code is now on master. Closing this ticket, but will keep #5021 open.

Also, updated #5021 with your suggestion to migrate SimpleParser unit-tests. Closing this ticket. #5021 should be couple lines long change.

comment:14 Changed 3 years ago by tomek

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

comment:15 follow-up: Changed 3 years ago by marcin

  • Resolution complete deleted
  • Status changed from closed to reopened

I didn't realize that the ticket had been stolen from me and I had another review of this ticket. Thus, I reopen it to give you guys a chance to address my comments. I realize that show must go on while people are on holidays, but one of the reasons why I couldn't complete the review last week was the fact that I got the ticket back very late.

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

This behavior has changed in Kea 1.2. Older versions used additional logic to determined whether the csv-format should be true or false. - typo "determined" vs "determine".

src/bin/dhcp4/json_config_parser.cc
Thanks for incorporating the conversion to mutable config in the data.h. On reflection I think it was also possible to do this:

ElementPtr mutable_cfg = const_pointer_cast<Element>(config_set);

which would get rid of constness and avoid copying. But, because this is non performance critical part of the code I am fine with copying.

src/bin/dhcp4/simple_parser4.cc
I suggest that the defaults are stored in the anonymous namespace.

Typo: SimpleParser4::INHERIT_GLOBAL_TO_SUBNET4 comment says subnet6 scope.

src/bin/dhcp4/simple_parser4.h
This class needs some description, even if it is very brief.

src/bin/dhcp6/json_config_parser.cc
See the comment for dhcp4/json_config_parser.cc

src/bin/dhcp6/simple_parser6.cc
Defaults are static within the SimpleParser6 class. Will they actually be used outside of the parser class?

A description of SimpleParser6::GLOBAL6_DEFAULTS says "DHCPv4 and DHCPv6. It should rather be "DHCPv6".

src/bin/dhcp6/simple_parser6.h
This class needs some description, even if it is very brief.

src/bin/dhcp6/tests/simple_parser6_unittest.cc~
Have you accidentally checked in this file?

src/lib/cc/cc.dox
Take a look at the generated Doxygen docs. This new page is on the same level as "Kea Developer's Guide" section, whereas it should be within this section.

A general comment about naming Kea libraries... I think we should rather refer to the libraries as libkea-cc, libkea-dhcpsrv etc. in the documentation, because these are their actual names in the built code.

The name of the section "Kea configuration commands library" sounds like it merely includes logic for sending commands to Kea. The SimpleParser is in fact about something else, and it is currently the only documentation within cc.dox. Perhaps it would be better to say: "libkea-cc - Kea configuration utilities library"?

"As such there's fewer incentives..." -> "... there are fewer..." ?

src/lib/cc/data.h
Not sure if we have any guideline for this but the doxygen comments in the data.h used back slashes for doxgen tags. The new description of getMutableMap uses @. If you think it doesn't matter, I am fine to leave as is. It is just a bit inconsistent.

src/lib/cc/simple_parser.cc
A nit in line 126:

cnt++;

could be more efficient when replaced by:

++cnt;

src/lib/cc/simple_parser.h
This file should include <vector>

This part of the class description is probably not true anymore: "The only state kept in most cases is whether the parsing is done in v4 or v6 context."

I wonder if the class description should include some little text like "This class has been initially created to facilitate DHCPv4 and DHCPv6 servers' configuration parsing. Thus examples provided herein are related to DHCP configuration. Nevertheless, this is a generic class to be used in other modules too."

I suggest to update the description of the getPosition() method to say when it returns ZERO_POSITION().

src/lib/dhcpsrv/libdhcpsrv.dox
src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
Not sure if this comment is still relevant:

/// @todo: Uncomment as part of the ticket 5019 work.

comment:16 Changed 3 years ago by fdupont

I strongly suggest to move http://kea.isc.org/ticket/5039#comment:15 to #5021...

comment:17 in reply to: ↑ 15 Changed 3 years ago by fdupont

Replying to marcin:

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

This behavior has changed in Kea 1.2. Older versions used additional logic to determined whether the csv-format should be true or false. - typo "determined" vs "determine".

=> fixed (directly in master as it is a typo).

src/bin/dhcp4/json_config_parser.cc
Thanks for incorporating the conversion to mutable config in the data.h. On reflection I think it was also possible to do this:

ElementPtr mutable_cfg = const_pointer_cast<Element>(config_set);

which would get rid of constness and avoid copying. But, because this is non performance critical part of the code I am fine with copying.

=> I am adding a discussion about this in #5021

src/bin/dhcp4/simple_parser4.cc
I suggest that the defaults are stored in the anonymous namespace.

=> I disagree: it could be very useful to access to the defaults, e.g. from a hook.

Typo: SimpleParser4::INHERIT_GLOBAL_TO_SUBNET4 comment says subnet6 scope.

=> already found so fixed.

src/bin/dhcp4/simple_parser4.h
This class needs some description, even if it is very brief.

=> there is a description in the merged version.

src/bin/dhcp6/json_config_parser.cc
See the comment for dhcp4/json_config_parser.cc

=> and see my answer.

src/bin/dhcp6/simple_parser6.cc
Defaults are static within the SimpleParser6 class. Will they actually be used outside of the parser class?

=> this is the same than anonymous space item so IMHO they will be used outside.

A description of SimpleParser6::GLOBAL6_DEFAULTS says "DHCPv4 and DHCPv6. It should rather be "DHCPv6".

=> already fixed.

src/bin/dhcp6/simple_parser6.h
This class needs some description, even if it is very brief.

=> already fixed.

src/bin/dhcp6/tests/simple_parser6_unittest.cc~
Have you accidentally checked in this file?

=> I supposed it was the case so I removed it: already found and fixed.

src/lib/cc/cc.dox
Take a look at the generated Doxygen docs. This new page is on the same level as "Kea Developer's Guide" section, whereas it should be within this section.

A general comment about naming Kea libraries... I think we should rather refer to the libraries as libkea-cc, libkea-dhcpsrv etc. in the documentation, because these are their actual names in the built code.

The name of the section "Kea configuration commands library" sounds like it merely includes logic for sending commands to Kea. The SimpleParser is in fact about something else, and it is currently the only documentation within cc.dox. Perhaps it would be better to say: "libkea-cc - Kea configuration utilities library"?

"As such there's fewer incentives..." -> "... there are fewer..." ?

=> I leave this to #5021

src/lib/cc/data.h
Not sure if we have any guideline for this but the doxygen comments in the data.h used back slashes for doxgen tags. The new description of getMutableMap uses @. If you think it doesn't matter, I am fine to leave as is. It is just a bit inconsistent.

=> unfortunately data.h was already inconsistent so I don't believe we should fix it.

src/lib/cc/simple_parser.cc
A nit in line 126:

cnt++;

could be more efficient when replaced by:

++cnt;

=> first I disagree that "++cnt" is more efficient than "cnt++", second I believe no compiler will produce a different code in this particular case (increment alone at end of block).

src/lib/cc/simple_parser.h
This file should include <vector>

=> I agree: I fixed it in master

This part of the class description is probably not true anymore: "The only state kept in most cases is whether the parsing is done in v4 or v6 context."

=> it applies to derived classes so it is not really out of context.

I wonder if the class description should include some little text like "This class has been initially created to facilitate DHCPv4 and DHCPv6 servers' configuration parsing. Thus examples provided herein are related to DHCP configuration. Nevertheless, this is a generic class to be used in other modules too."

=> moved to #5021

I suggest to update the description of the getPosition() method to say when it returns ZERO_POSITION().

=> can you say more? Of course this includes the no parent case but not only...

src/lib/dhcpsrv/libdhcpsrv.dox
src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
Not sure if this comment is still relevant:

/// @todo: Uncomment as part of the ticket 5019 work.

=> there is a dangling @todo ticket so IMHO the policy is to forget to remove them even when the referred ticket was closed.

comment:18 Changed 3 years ago by tomek

  • Status changed from reopened to reviewing

I fixed all those issues pointed out by Marcin on trac5021. See my comments here: ticket:5021#comment:12

comment:19 Changed 3 years ago by tomek

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

#5021 is now merged, closing this one as well.

Note: See TracTickets for help on using tickets.