Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#5036 closed enhancement (complete)

Define Dhcp6 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: 1
Total Hours: 63 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. (#5036, this ticket)
  1. Migrate Dhcp6 and Subnet6 structure handling. This will move the useful code from Subnets6ListConfigParser, Subnet6ConfigParser, Pool6ListParser and Pool6Parser to grammar. Afterwards those classes could be deleted. (#5037)
  1. Migrate interfaces configuration (InterfaceListConfigParser, IfacesConfigParser6); (#5038)
  1. Migrate option values and definitions configuration (OptionDefListParser, OptionDataParser, OptionDataListParser); (#5039)
  1. Migrate host reservations configuration (HostReservationParser, HostReservationIdsParser, !HostReservationIdsParser6) (#5040)
  1. Migrate hook libraries configuration (HooksLibrariesParser); (#5041)
  1. Migrate other parameters: MAC sources (!MACSourcesListConfigParser), control socket (ControlSocketParser), relay information (RelayInfoParser); (#5042).
  1. Migrate D2 configuration (!D2ClientConfigParser); (#5043)
  1. Migrate DUID configuration (!DUIDConfigParser); (#5044)
  1. Migrate lease expiration configuration (ExpirationConfigParser). (#5045).

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

  • Description modified (diff)

comment:3 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

comment:4 Changed 3 years ago by tomek

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

comment:5 Changed 3 years ago by fdupont

Perhaps this is the place where the trac5014_phase2 grammar will be completed? BTW I'd like to get a definition of configuration parameters or in case if is simpler the name of the corresponding grammar rule(s).

comment:6 Changed 3 years ago by tomek

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

This code (trac5036) is now ready for review. This branch contains cherry-picked code from trac5014_phase2 with many other improvements, comments and clean-ups.

Proposed changelog:

11XX.	[func]		tomek, francis
	Dhcp6 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 #5036, git tbd)

This ticket is already large (the diff says 16,5K lines, but over 10K lines are generated output flex/bison files, which do not need to be reviewed). If there are any significant changes needed, please consider pushing them to separate tickets.

Responding to comment:5 from Francis, I went through all parameters specified in dhcp6.spec and added several missing ones. There's a group of about 10 parameters in DhcpDdns? that is not supported and the code simply parses them as generic JSON. That is something we hopefully fix in #5043. I have spotted several other parameters that we previously did not include in the bison grammar. They are now supported and I have updated the example files to actually use those parameters at least once.

I think one day we should develop a way to export the grammar from bison and generate formal grammar description or maybe even include it as part of the documentation. This is something that is clearly outside of this ticket, though.

comment:7 Changed 3 years ago by fdupont

  • Owner changed from Unassigned to fdupont

Getting the ticket for review. BTW you can steal it or provide comments...

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

First 2 points where trac5014_phase2 so trac5036 are incompatible with the previous code:

  • following JSON standard now booleans are case sensitive true and false, i.e. True or False are rejected. I believe the admin guide just says the opposite.
  • empty mac-sources are rejected by bison cf the macSourcesEmpty comment. IMHO empty mac-sources does not make sense but we have to decide if we accept them (and fix the grammar) or reject them (and fix the doc with a specific change warning).

Remember now JSON conforms to the standard about control characters on input but the output function was not fixed (and can't be without additional code e.g. using \u00xy escapes.

Some comments about the dhcp6.dox:

  • perhaps we should say more about syntactic contexts?
  • partial parsing is not really a proof-of-concept as it will be heavily used by unit tests (i.e. there will stay!)

I pushed a few re-indent, spell fix and (not cosmetic) a statement about the leave() matching enter() for syntactic contexts.

comment:9 Changed 3 years ago by fdupont

This will be fixed in #5043 but the list for ddns keywords doesn't match unit tests... (this is why I didn't address them in phase2)

I added a unknown_map_entry to hooks_param

Cosmetics changes to parse (e.g. missing ending ';') and include (e.g. doxygen comments) files.

I regenerated parser produced files on docs.

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

parseDHCP6() in dhcp6_test_utils.h should not write exception details by default. As the #ifdef ENABLE_DEBUG was rejected I am trying another approach.

Added 2 unit tests fro True and Null. Perhaps we should catch them in the lexer more explicitly (e.g. (?i:true) error true must be in lower case)

comment:11 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

In conclusion it looks fine even there are a few pending questions and of course a zillion of unit tests to add. Note nothing is really blocking so IMHO we can proceed with next steps.

comment:12 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by tomek

Replying to fdupont:

First 2 points where trac5014_phase2 so trac5036 are incompatible with the previous code:

  • following JSON standard now booleans are case sensitive true and false, i.e. True or False are rejected. I believe the admin guide just says the opposite.

Really? I couldn't find any place where True or False was specific. Anyway, I added specific text that Kea 1.2 introduces stricter checking and that those are not allowed. Hopefully that's clear enough.

  • empty mac-sources are rejected by bison cf the macSourcesEmpty comment. IMHO empty mac-sources does not make sense but we have to decide if we accept them (and fix the grammar) or reject them (and fix the doc with a specific change warning).

This is correct. Empty mac-sources doesn't make any sense. Clarified the docs regarding that. User can either not define it at all or define it with one value: "any".

Remember now JSON conforms to the standard about control characters on input but the output function was not fixed (and can't be without additional code e.g. using \u00xy escapes.

Ok, I don't know what to do with this one, but I think this is not important enough for us to spend any more time on this. Has anyone used the control characters anyway?

Some comments about the dhcp6.dox:

  • perhaps we should say more about syntactic contexts?

Updated existing text and added one more paragraph. Let me know if this is sufficient. If it isn't, can you be more specific which aspects I should describe in more detail?

  • partial parsing is not really a proof-of-concept as it will be heavily used by unit tests (i.e. there will stay!)

Good catch. It was a left-over from the experimental phase. Rephrased the text.

I pushed a few re-indent, spell fix and (not cosmetic) a statement about the leave() matching enter() for syntactic contexts.

Thanks.

comment:13 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 60 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 60 to 62

Replying to fdupont:

parseDHCP6() in dhcp6_test_utils.h should not write exception details by default. As the #ifdef ENABLE_DEBUG was rejected I am trying another approach.

I removed it, because I really dislike conditional compilation. Sometimes it is unavoidable, but in this case it wasn't. And I think it's ok to print out the exception details. In fact, I dislike how googletest reacts to exceptions. It sometimes doesn't show the what() output at all.

Added 2 unit tests fro True and Null. Perhaps we should catch them in the lexer more explicitly (e.g. (?i:true) error true must be in lower case)

Thanks for writing them. There are many more unit-tests waiting to be written here, but I don't think we need to wait with the merge for them. We can gradually increase the number of tests in upcoming tickets.

Ok, I have addressed your comments (hope I haven't missed any). Let me know if you think the code is ready for merge after the changes.

Submitted full system tests run on jenkins:

DHCPv6: https://jenkins.isc.org/view/Forge-developers/job/Kea6-single-branch/39/
DHCPv4: https://jenkins.isc.org/view/Forge-developers/job/Kea4-single-branch/ (build 37)

comment:14 in reply to: ↑ 12 Changed 3 years ago by fdupont

Replying to tomek:

Replying to fdupont:

First 2 points where trac5014_phase2 so trac5036 are incompatible with the previous code:

  • following JSON standard now booleans are case sensitive true and false, i.e. True or False are rejected. I believe the admin guide just says the opposite.

Really? I couldn't find any place where True or False was specific. Anyway, I added specific text that Kea 1.2 introduces stricter checking and that those are not allowed. Hopefully that's clear enough.

=> in fact there is a few places in the code and documentation where quoted (vs JSON) true/false and even 0/1 are accepted. But I agree there is nothing misleading about JSON (and in #5085 I cleaned up some remaining "cvs-format":True and co).

  • empty mac-sources are rejected by bison cf the macSourcesEmpty comment. IMHO empty mac-sources does not make sense but we have to decide if we accept them (and fix the grammar) or reject them (and fix the doc with a specific change warning).

This is correct. Empty mac-sources doesn't make any sense. Clarified the docs regarding that. User can either not define it at all or define it with one value: "any".

=> the unit test will have to be updated too.

Remember now JSON conforms to the standard about control characters on input but the output function was not fixed (and can't be without additional code e.g. using \u00xy escapes.

Ok, I don't know what to do with this one, but I think this is not important enough for us to spend any more time on this. Has anyone used the control characters anyway?

=> anybody who wants to put a 8 bit binary value. It is done on #3085.

Some comments about the dhcp6.dox:

  • perhaps we should say more about syntactic contexts?

Updated existing text and added one more paragraph. Let me know if this is sufficient. If it isn't, can you be more specific which aspects I should describe in more detail?

=> I propose to leave it at it is now. If more is needed (about this or how to parse a possible empty construct without accepting spurious comas) we'll add something (but not now).

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

Replying to tomek:

Replying to fdupont:

parseDHCP6() in dhcp6_test_utils.h should not write exception details by default. As the #ifdef ENABLE_DEBUG was rejected I am trying another approach.

I removed it, because I really dislike conditional compilation. Sometimes it is unavoidable, but in this case it wasn't.

=> I added an optional verbose flag: if we find a case where an unexpected exception is raised it will be enough to set it.

And I think it's ok to print out the exception details.

=> it makes unit tests verbose even when an exception is expected.

In fact, I dislike how googletest reacts to exceptions. It sometimes doesn't show the what() output at all.

=> I agree but my proposed extension to match the what() was rejected (too heavy).

Added 2 unit tests fro True and Null. Perhaps we should catch them in the lexer more explicitly (e.g. (?i:true) error true must be in lower case)

Thanks for writing them. There are many more unit-tests waiting to be written here, but I don't think we need to wait with the merge for them. We can gradually increase the number of tests in upcoming tickets.

=> I added in #3085 the more specific error messages.

Ok, I have addressed your comments (hope I haven't missed any). Let me know if you think the code is ready for merge after the changes.

=> Seems fine. BTW I believe #5084 was fixed too, wasn't it? If Jenkins is happy I have nothing new to add.

comment:16 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

comment:17 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 2 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 62 to 63

Thanks a lot for your review. The code is now merged. Closing ticket.

comment:18 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

Note: See TracTickets for help on using tickets.