Opened 3 years ago

Closed 3 years ago

#5019 closed task (complete)

Migrate Dhcp4 and Subnet4 to the bison parser

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: 1
Total Hours: 41 Internal?: no

Description (last modified by fdupont)

Migrate Dhcp4 and Subnet4 structure handling. This will move the useful code from Subnets4ListConfigParser and Subnet4ConfigParser to grammar. Afterwards those classes could be deleted.

Overview in ticket #5017. More details will become available when #5015 is done.

Subtickets

#5020: Dhcp4: migrate interfaces configuration to new parserclosedtomek
#5021: Dhcp4: Migrate option values and definitions configuration to new parserclosedtomek
#5031: Dhcp4: Migrate hook libraries configurationclosedtomek
#5032: Dhcp4: Migrate MAC sources, control socket, relay informationclosedtomek
#5034: Dhcp4: Migrate DUID configurationclosedtomek
#5096: DhcpX: migrate database configurationclosedfdupont
#5097: DhcpX: Migrate Pools configurationclosedfdupont
#5098: DhcpX: Migrate client class configurationclosedfdupont

Change History (33)

comment:1 Changed 3 years ago by tomek

  • Component changed from classification to configuration

comment:2 Changed 3 years ago by tomek

  • Summary changed from Migrate Dhcp4 and Subnet4 to the new parser to Migrate Dhcp4 and Subnet4 to the bison parser

comment:3 Changed 3 years ago by tomek

  • Priority changed from medium to high

comment:4 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

comment:5 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:6 Changed 3 years ago by tomek

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

comment:7 Changed 3 years ago by fdupont

I pick up this ticket in an arbitrary way to discuss about migration to simple parser:

  • the canonical example is trac5039 (migration of the top parser)
  • what names to use for files, classes, default value tables, etc.

In some cases (and in individual tickets, most already exist) we have to discuss about optional vs mandatory stuff as default values won't work everywhere.
Note as soon as these details are decided IMHO we could quasi mechanically migrate current parsers...

comment:8 Changed 3 years ago by tomek

  • Owner tomek deleted

comment:9 Changed 3 years ago by tomek

Add a subticket #5034.

comment:10 Changed 3 years ago by tomek

Add a subticket #5020.

comment:11 Changed 3 years ago by tomek

Add a subticket #5031.

comment:12 Changed 3 years ago by tomek

Add a subticket #5032.

comment:13 Changed 3 years ago by tomek

Add a subticket #5021.

comment:14 Changed 3 years ago by tomek

Add a subticket #5096.

comment:15 Changed 3 years ago by tomek

Add a subticket #5097.

comment:16 Changed 3 years ago by tomek

Add a subticket #5098.

comment:17 Changed 3 years ago by tomek

  • Priority changed from high to medium

comment:18 Changed 3 years ago by tomek

That top level parser will be migrated last. It makes more sense, because otherwise we would be updating the code twice - once when the top level parser is migrated, then every time its child parsers are migrated. Hence decreasing priority to regular value (medium).

comment:19 Changed 3 years ago by tomek

  • Owner set to tomek

comment:20 Changed 3 years ago by fdupont

  • Description modified (diff)

comment:21 Changed 3 years ago by fdupont

  • Description modified (diff)

comment:22 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 32
  • Owner changed from tomek to fdupont
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 32

The code is now ready for review. Due to dependencies on #5097 (the macro defined there is very useful) it's better to review/merge #5019 and #5097 and then finish the remaining work in SubnetXConfigParser in #5037.

There is also #5016 that will get rid of the remaining minor things.

comment:23 Changed 3 years ago by tomek

  • Owner fdupont deleted

comment:24 Changed 3 years ago by fdupont

  • Owner set to fdupont

comment:25 Changed 3 years ago by tomek

The changes on trac5019 cover migration of global v4 parser, global v6 parser and much of the work needed for subnet4 and subnet6 parsers. Subnet parsers are now derived from SimpleParser and use it to some degree, but there are still some parts of the code that use old parser.

comment:26 Changed 3 years ago by fdupont

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

Cancelled.

comment:27 Changed 3 years ago by fdupont

  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:28 Changed 3 years ago by fdupont

Oops, I wanted to cancelled 5119... Still under review.

comment:29 Changed 3 years ago by fdupont

  • Status changed from reopened to reviewing

comment:30 follow-up: Changed 3 years ago by fdupont

  • Add Hours to Ticket changed from 32 to 36
  • Owner changed from fdupont to tomek

Many changes:

  • I copied the templates to simpleparser.h
  • I restored the protected in this file
  • I renames sub[46]ptr into sn[46]ptr
  • I renamed the build function of Subnet*ConfigParser to parse
  • I created a global parser derived to SimpleParser, moved setGlobalParameter[46] to it (renamed into parse) to use templates & co

But IMHO there are a lot of things which should be done:

  • get rid of ParserContext outside places where old DhcpParser storage is still needed
  • use or remove duplicate_option_warning (with a preference to remove)
  • make HRMode values keywords
  • rename the pools_ storage member
  • add a position to first throw in createSubnet and all other places I have not yet found
  • avoid to add twice the position (I suggest to add for exceptions other than DhcpConfigError
  • remove createSubnetConfigParser (but this point is already in the plan)
  • migrate initSubnet without giving up on unknown parameters (cf previous point)

I know some points must be postponed because they depend on the merge of the last migrated parsers and a rebase will be at least as painful as a merge followed by cleanups...

comment:31 in reply to: ↑ 30 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 36 to 4
  • Owner changed from tomek to fdupont
  • Total Hours changed from 32 to 40

Replying to fdupont:

Many changes:

  • I copied the templates to simpleparser.h
  • I restored the protected in this file
  • I renames sub[46]ptr into sn[46]ptr
  • I renamed the build function of Subnet*ConfigParser to parse
  • I created a global parser derived to SimpleParser, moved setGlobalParameter[46] to it (renamed into parse) to use templates & co

Thanks. I have reviewed your changes and they look good. I changed one minor thing: the srv_config is now passed as a parameter, so it is easier to test the code.

I have rebased the code to latest master and pushed as trac5019_rebase.

But IMHO there are a lot of things which should be done:

  • get rid of ParserContext outside places where old DhcpParser storage is still needed

This is something I'd like to do as part of 5037.

  • use or remove duplicate_option_warning (with a preference to remove)

Removed.

  • make HRMode values keywords

We can do this once this code is merged to master as it does not have any dependencies.

  • rename the pools_ storage member

Which one? The only pools_ I found is in SubnetConfigParser? in src/lib/dhcpsrv/parsers/dhcp_parsers.h and that name seems appropriate. Can you explain why you want it to be renamed and renamed to what name?

  • add a position to first throw in createSubnet and all other places I have not yet found

Added.

  • avoid to add twice the position (I suggest to add for exceptions other than DhcpConfigError

In which specific place in the code?

  • remove createSubnetConfigParser (but this point is already in the plan)

Removed in both v4 and v6.

  • migrate initSubnet without giving up on unknown parameters (cf previous point)

I know some points must be postponed because they depend on the merge of the last migrated parsers and a rebase will be at least as painful as a merge followed by cleanups...

I would prefer to do this as part of the 5037 work.

I also migrated one last parser that we missed RSOOListConfigParser.

The code is now on trac5019_rebase. Once it is merged, I plan to create 5037 branch off master and address remaining points there (or we can split the work into more tickets if you prefer that).

comment:32 follow-up: Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

About pools_ it is because of parser.parse(pools_, pools); but it can be done later (and it is not clear what variable should change name).

For the twice position it is a generic issue which will be addressed later, e.g., making the convention than DhcpConfigError is always thrown with a position (so a catch (const DhcpConfigError&) { throw; } catch (something else) { adding position and throw DhcpConfigError will do the job).

Thanks for RSOOListConfigParser I missed...

OK to merge but we have to agree about the next steps as we need to split the work. Perhaps it will be easier with children than to put everything in #5037?

comment:33 in reply to: ↑ 32 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 4 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 40 to 41

Replying to fdupont:

About pools_ it is because of parser.parse(pools_, pools); but it can be done later (and it is not clear what variable should change name).

Got it. Ok, that's very easy to do.

For the twice position it is a generic issue which will be addressed later, e.g., making the convention than DhcpConfigError is always thrown with a position (so a catch (const DhcpConfigError&) { throw; } catch (something else) { adding position and throw DhcpConfigError will do the job).

Sounds good. This is more like a guideline that we should follow rather than a material for a separate ticket.

OK to merge but we have to agree about the next steps as we need to split the work. Perhaps it will be easier with children than to put everything in #5037?

5019 merged.

As for the 5037, I saw that you created tickets 5121-5127. Adding 7 new tickets 2 days before the code drop will be a problem. I think we should trim them down to 2 tickets: you will work on one and I'll work on another, then we will cross-review. My preferred split would be:

  1. any changes in SubnetParserConfig?
  1. everything else

Let's discuss that on the call in 13 minutes.

Thanks for the review. Code is now merged to master. Closing ticket.

Note: See TracTickets for help on using tickets.