Opened 3 years ago

Closed 3 years ago

#5017 closed enhancement (complete)

Define Dhcp4 grammar (bison)

Reported by: tomek Owned by: tomek
Priority: high 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: 3
Total Hours: 33 Internal?: no

Description (last modified by tomek)

Once #5015 is done, we need to conduct the following:

  1. General grammar foundation (for bison) written. This will create JSON structures, so we keep using classes derived from DhcpConfigParser for now. Once this is done, we will have basic bison grammar in place. This will give us a number of benefits. First and foremost, we will have the flexibility of bison, like adding multi-line comments, // style comments or ability to implement file inclusions). Second, it will give us a foundation to migrate all configuration parameters. Having the configuration defined in one place is essential for readability of the code. (#5017, this ticket)
  1. Migrate Dhcp4 and Subnet4 structure handling. This will move the useful code from Subnets4ListConfigParser, Subnet4ConfigParser, Pool4ListParser and Pool4Parser to grammar. Afterwards those classes could be deleted. (#5019)
  1. Migrate interfaces configuration (InterfaceListConfigParser, IfacesConfigParser4); (#5020)
  1. Migrate option values and definitions configuration (OptionDefListParser, OptionDataParser, OptionDataListParser); (#5021)
  1. Migrate host reservations configuration (HostReservationParser, HostReservationIdsParser, !HostReservationIdsParser4) (#5030)
  1. Migrate hook libraries configuration (HooksLibrariesParser); (#5031)
  1. Migrate other parameters: MAC sources (!MACSourcesListConfigParser), control socket (ControlSocketParser), relay information (RelayInfoParser); (#5032)
  1. Migrate D2 configuration (!D2ClientConfigParser); (#5033)
  1. Migrate DUID configuration (!DUIDConfigParser); (#5034)
  1. Migrate lease expiration configuration (ExpirationConfigParser). (#5035).

This ticket covers item 1 of that list.

Subtickets

Change History (18)

comment:1 Changed 3 years ago by tomek

  • Description modified (diff)

comment:2 Changed 3 years ago by tomek

  • Summary changed from Define Dhcp4 grammar to Define Dhcp4 grammar (bison)

comment:3 Changed 3 years ago by tomek

  • Description modified (diff)

comment:4 Changed 3 years ago by tomek

  • Description modified (diff)

comment:5 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

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 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 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 16
  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 16

The code is now ready for review.

One particular thing that probably requires fixing is the support for "version" parameter. It was copied over from DHCPv6, but that parameter is not used anywhere or mentioned in the docs.

I don't see it mentioned anywhere in the docs, can't find any use of it in the code either.
see src/bin/dhcp6/json_config_parser.cc:710.
This particular line comes from the year 2013 and was commited as part of ticket 2355.
The comment in that ticket mentions "versionTest", but I can't find anything like that in the code.

Proposed ChangeLog?

11XX.	[func]		tomek, fdupont
	Dhcp4 parser migrated to bison. This yields a number of user
	visible changes:
	- better comment handling (bash, C, C++ style comments)
	- file includes now supported
	- better syntax checking
	(Trac #5017, git tbd)

comment:9 Changed 3 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:10 Changed 3 years ago by fdupont

I am trying to use the parser in unit tests (not yet finished) and I found a syntax error in the classify unit test (spurious comma) which was not yet detected?
I'll continue (and perhaps finish) during the weekend.
BTW some improvements from #5085 must be included so I recommend you look at them...

comment:11 Changed 3 years ago by fdupont

Found another JSON syntax error in Dhcp4ParserTest.reservationWithOptionDefinition code...

comment:12 Changed 3 years ago by fdupont

  • Add Hours to Ticket changed from 16 to 22
  • Total Hours changed from 16 to 22

I have the intention to merge #5085 but I'll wait a bit to leave the time to review changes up to the current git hash 32f910c80d085249226de3f352d0aeaaed254b62.

comment:13 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Added stuff from #5085 (\u00xy escapes and better handling of JSON keywords with bad cases).
Made new example files (used in unit tests) distributable (for make distcheck).
I can't see other things so I finished.

comment:14 Changed 3 years ago by fdupont

  • Add Hours to Ticket changed from 22 to 24
  • Total Hours changed from 22 to 24

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

  • Add Hours to Ticket changed from 24 to 6
  • Owner changed from tomek to fdupont
  • Total Hours changed from 24 to 30

Thanks a lot for the work you did. I thought converting existing unit-tests to use this new parser is way too much work to be covered in this ticket, but since you decided to it - that's great. I reviewed your changes and they mostly look ok, except cb935c8202e977564a8aa6ba21311e45b504ba38, which deleted dhcp6_lexer.cc. I re-added it back in follow up commits.

I do not have enough experience to say whether the changes you did for unicode are sufficient or not, but the unit-tests seem to be happy with them.

The changes I did after your review:

  • implemented a very basic test code that removes c/c++/bash comments from the input file, so it can be loaded by the legacy JSON parser and simplified the tests a bit.
  • I removed obsolete "version" parameter from the parsers.
  • added support for dhcp-socket-type parameter
  • changed the definition for interfaces-config
  • added advanced.json example file that defined dhcp-socket-type

Submitted the latest code from #5017 to Jenkins:

I think the ChangeLog should mention that "Obsolete version parameter has been removed from DHCPv4 and DHCPv6 parsers".

Assuming Jenkins doesn't complain, the code should be ready for merge in my opinion. Do you agree or is there anything outstanding left to do?

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

Replying to tomek:

Thanks a lot for the work you did. I thought converting existing unit-tests to use this new parser is way too much work to be covered in this ticket, but since you decided to it - that's great. I reviewed your changes and they mostly look ok, except cb935c8202e977564a8aa6ba21311e45b504ba38, which deleted dhcp6_lexer.cc. I re-added it back in follow up commits.

=> oops, I regenerated it on docs (a recent flex minimizes spurious warnings from analyzers).

I do not have enough experience to say whether the changes you did for unicode are sufficient or not, but the unit-tests seem to be happy with them.

=> they are not enough but it seems the underlying model of JSON is 16 bit wide chars (i.e., looks like WIN32) so what we have is the best for our 8 bit chars even we are not really in unicode latin-1 as we are not in unicode... So it is not sufficient but it is the best we can do without major changes, so it fits 1.2 goals. BTW there is an unicode ticket as log4cplus is unicode aware.

I'll see other points later but IMHO there should be no reason to wait too long.

comment:17 Changed 3 years ago by tomek

  • Owner changed from fdupont to tomek

Thanks a lot for your review and for the code you wrote.

This is now merged. Closing ticket.

There more work to be done, but we will continue in other tickets.

comment:18 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 6 to 3
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 30 to 33

The Jenkins test runs look good. Merged, closing ticket.

Note: See TracTickets for help on using tickets.