Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#5126 closed enhancement (complete)

migrate to SimpleParser "flat style"

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea1.3 beta
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: 6
Total Hours: 6 Internal?: no

Description (last modified by fdupont)

cf #5037:

  • get rid of FOREACH loops and simply handle each parameter in sequence (aka flat simpleparser style)
  • use defaults when possible so we know parameters exist

Subtickets

Change History (16)

comment:1 Changed 3 years ago by fdupont

  • Component changed from Unclassified to remote-management
  • Description modified (diff)

comment:2 Changed 3 years ago by tomek

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

comment:3 Changed 3 years ago by fdupont

  • Description modified (diff)

comment:4 Changed 3 years ago by fdupont

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

comment:5 Changed 3 years ago by fdupont

List of parsers:

comment:6 Changed 3 years ago by fdupont

Dependency summary: #3389, #5061, #5121, #5124.

comment:7 Changed 3 years ago by fdupont

About dhcp4o6_port I remember why it is an uint32_t: the old parser returns uint32_t for integer values and a cast will accept out of range values without control. This can be handled correctly with the new simpleparser.

comment:8 Changed 3 years ago by fdupont

I propose to finish easy pending works on this and to fork into:

  • review and merge done stuff
  • create a following ticket to handle remaining work, i.e. mainly what is in the description
  • create or recycle a ticket to cleanup exception and DhcpConfigParser as soon as #5110 is merged.

comment:9 Changed 3 years ago by fdupont

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

Addressed the parsers marked as to do with no dependency in the comment:5 at the exception of the HookLibrariesParser which requires to use configuration (aka Cfg*) so as a dependency too.
So this ticket is now ready for review with a follower ticket to address D2, server functions and to do with dependencies.

comment:10 Changed 3 years ago by tomek

  • Milestone changed from Kea1.2 to Kea1.2-final

Code freeze for 1.2-beta. Moving all remaining open tickets to 1.2-final.

comment:11 Changed 3 years ago by tomek

  • Milestone changed from Kea1.2-final to Kea1.3

As discussed on 2017-04-13 Kea call, moving all low review tickets that have not started review yet to Kea1.3.

comment:12 Changed 2 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:13 follow-up: Changed 2 years ago by tomek

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

Reviewing this ticket took me a lot more time than planned, because
I just got a new LCD and had to tweak many things. Oh well.

simple_parser_unittest.cc
The simpleParserClassTest makes the getUint8 public.
I think the better way is to make it public in the
base class. Did the change. Please pull.

dhcp_parser.cc
The RelayInfoParser::getIOAddress() logically belongs to SimpleParser
(there are lots of other places that want to retrieve a parameter that
is a IP address). But I can understand why you didn't move it there.
If moved to cc/simple_parser.cc, it would introduce new dependency
to libcc on libasiolink. Currently libcc depends only on exceptions,
so this would be a serious change.

So I'm ok with leaving the code as is, but moving it to SimpleParser
is also ok. I leave the decision up to you.

duid_config_parser.cc

DUIDConfigParser::parse(): What's the point of catching DhcpConfigError
if you immediately throw it without doing anything?

expiration_config_parser.cc
parse() - the same question as above.


I made several small corrections. Please pull and review.

Once you review and answer my questions above, feel free to merge.

I just would like to know the answers, but I can look at them after
the code is merged.


The code builds and unit-tests pass on Ubuntu 17.04 x64 which uses
g++ 6.3.

This is not a user visible change, so it's ok to not have a
ChangeLog for it.


Please make sure you update the add hours and total hours. This
should cover the total number of hours you spent on this ticket,
no matter which milestone it was.

comment:14 in reply to: ↑ 13 Changed 2 years ago by fdupont

Replying to tomek:

simple_parser_unittest.cc
The simpleParserClassTest makes the getUint8 public.
I think the better way is to make it public in the
base class. Did the change. Please pull.

=> pull (and BTW reviewed the few changes you made).

dhcp_parser.cc
The RelayInfoParser::getIOAddress() logically belongs to SimpleParser
(there are lots of other places that want to retrieve a parameter that
is a IP address). But I can understand why you didn't move it there.
If moved to cc/simple_parser.cc, it would introduce new dependency
to libcc on libasiolink. Currently libcc depends only on exceptions,
so this would be a serious change.

So I'm ok with leaving the code as is, but moving it to SimpleParser
is also ok. I leave the decision up to you.

=> the idea is to move common things to the base class so
if it is not used at another place there is no immediate need to
move the definition.

duid_config_parser.cc

DUIDConfigParser::parse(): What's the point of catching ! DhcpConfigError?
if you immediately throw it without doing anything?

=> the idea is to handle exceptions which are not DhcpConfigError.
As the try catch provides only positive filters this is the way to code that.

expiration_config_parser.cc
parse() - the same question as above.

=> same. BTW in languages like OCaml you can add a guard to a filter
(but these languages use heavily filters so it is not a surprise the
filter syntax is far more powerful).

Note this ticket was limited to the first of 3 items (which BTW has no
dependency at the opposite of the whole plan).

comment:15 Changed 2 years ago by fdupont

  • Add Hours to Ticket changed from 5 to 6
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 5 to 6

Merged. Created #5343 and #5344 as followup (#5343 removes code so could be done as soon as possible, for instance as 1.3-final, #5344 is a followup more for 1.4).

comment:16 Changed 2 years ago by vicky

  • Milestone changed from Kea1.3 to Kea1.3 beta

Milestone renamed

Note: See TracTickets for help on using tickets.