Opened 3 years ago

Closed 3 years ago

#5123 closed enhancement (fixed)

fix SimpleParser exception

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea 1.2 - Mozilla Milestone 1
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5037
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 1 Internal?: no

Description

Cf #5037:

  • move DhcpConfigError declaration in a new file in lib/cc which will be included by simpleparser.h and lib/dhcpsrv/parsers/dhcp_config_parser.h
  • raise DhcpConfigError with location in simple parser tools: the convention should be when DhcpConfigError is raise there is a location with parenthesis. (i.e. make far easier to track if the location has to be added or not)

Subtickets

Change History (11)

comment:1 Changed 3 years ago by fdupont

  • Component changed from Unclassified to remote-management

comment:2 Changed 3 years ago by fdupont

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

comment:3 Changed 3 years ago by fdupont

Merged #5125 (templates) into this.
I'll use src/lib/cc/dhcp_config_error.h (i.e., the name of the exception) with indirect includes (so it will be easy to move the declaration to simple parser.h when DhcpConfigParser will be phased out).
For templates I'll create 2 new ones so migration will be smooth.

comment:4 Changed 3 years ago by fdupont

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

Done. Ready for review. I rewrote in flat simpleparser style the few parsers using templates (D2ClientConfig, PdPool? and the 2 Globals).

comment:5 Changed 3 years ago by fdupont

Added an explanation about the convention with DhcpConfigError and (<position>). Surely not at the best place but now it is somewhere and can be easily found (and moved to a better place).

Still to be reviewed.

comment:6 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:7 follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 1
  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 1

dhcp_config_error.h
Couple comments: First, can you rename this header to config_error.h? The class
will renamed to ConfigError? very soon and then it will have the correct header
name. Or perhaps even add it to simple_parser.h? I'm not aware of any code that
needs to throw DhcpConfigError? and don't derive from SimpleParser. But I don't
insist on this one. Keeping is in a separate header is ok.

Second, I think we should have some sort of macro or method that would accept
ConstElementPtr? and would extract the position automatically. Obviously, adding
such a macro is easy, but actually using it throughout the code is not, so it's
outside of scope for this ticket. If you agree, with this proposal, however,
please add @todo somewhere in the code.

simple_parser.h

I like the way getIntType is organized (so you pass the parent Element, not the
Element itself) very much. It is super convenient. Thanks a lot for doing this.

Comment for getIntType and getAndConvert should indicate that DhcpConfigParser?
may be thrown.

dhcp6/json_config_parser.cc

I think we should move getUint8, getUint32 and similar specific methods to the
base SimpleParser class. But we don't have to do it right now.


Code compiles and unit-tests pass on Ubuntu 16.04.1 x64.


The comments should be very easy to fix. So the last question is how do we want to merge those two tickets? Which one should go first: #5123 or #5122?

comment:8 in reply to: ↑ 7 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

dhcp_config_error.h
Couple comments: First, can you rename this header to config_error.h? The class
will renamed to ConfigError? very soon and then it will have the correct header
name. Or perhaps even add it to simple_parser.h? I'm not aware of any code that
needs to throw DhcpConfigError? and don't derive from SimpleParser. But I don't
insist on this one. Keeping is in a separate header is ok.

=> I was afraid it was used in D2 including indirectly through the old DhcpConfigParser?
class this is why I kept it in its own file. Of course the next term idea is
to move it to simple_parser.h, i.e., the dhcp_config_error.h is just for
the transition (BTW this is why it is only included in 2 files and no more).

So I keep the current file name but with a todo explaining it is temporary.

Second, I think we should have some sort of macro or method that would accept
ConstElementPtr? and would extract the position automatically. Obviously, adding
such a macro is easy, but actually using it throughout the code is not, so it's
outside of scope for this ticket. If you agree, with this proposal, however,
please add @todo somewhere in the code.

=> I believe you mean an extension of isc_throw? If it is the case I know what
to put in the todo (added it, check if it is what you asked for).

simple_parser.h

I like the way getIntType is organized (so you pass the parent Element, not the
Element itself) very much. It is super convenient. Thanks a lot for doing this.

=> now we have the DhcpConfigError? declaration in the right scope there is
no reason to not uniformize all the getXXX tools (BTW on both sides).

Comment for getIntType and getAndConvert should indicate that DhcpConfigError?
may be thrown.

=> yes, this is an useful part of the uniformization... Fixed (and other getXXX()'s
documentation too).

BTW I have an idea about getPosition: perhaps it should return the parent
position (vs ZERO today) when the map member can't be found?
It will make getPosition more useful so simplifies some code.

dhcp6/json_config_parser.cc

I think we should move getUint8, getUint32 and similar specific methods to the
base SimpleParser class. But we don't have to do it right now.

=> BTW it was not possible for previous templates because of the
DhcpConfigError? issue so perhaps I was a bit conservative...


Code compiles and unit-tests pass on Ubuntu 16.04.1 x64.


The comments should be very easy to fix. So the last question is how do we want to merge those two tickets?
Which one should go first: #5123 or #5122?

=> #5123 could have an impact on #5122 so logically it should be
merged before.

Pushed a comments only change in commit c4361e59fd684fbd851c01c7a1fcc4530d4e3c4f

comment:9 Changed 3 years ago by tomek

  • Owner changed from tomek to fdupont

I reviewed your comments. They addressed all my concerns. The code is ready for merge.

Feel free to merge.

comment:10 Changed 3 years ago by tomek

I was curious about your comment that #5123 may have an impact on #5122. You were right there were 2 unit-tests failing. Since I did the experiment of merging both and fixed the unit-tests, I pushed both to master.

I already closed #5122. Feel free to close #5123 as the code is on master.

I will check later today (or maybe tomorrow) if Jenkins is happy. We will sort out if we need a ChangeLog? on Monday and fix any issues that may appear after the merge.

comment:11 Changed 3 years ago by fdupont

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

I fixed a few doxygen warnings (about left comments, i.e. I removed (vs added) some lines).
Note the dhcpv[46]ConfigInherit are now clearly obsolete: they reference(d) ParserContext.
Closing this ticket.

Note: See TracTickets for help on using tickets.