Opened 12 months ago

Closed 9 months ago

#5076 closed task (complete)

Implement configuration parsing within Control Agent (use Bison)

Reported by: marcin Owned by: tomek
Priority: medium Milestone: Kea1.2
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: 0
Total Hours: 20 Internal?: no

Description

See http://kea.isc.org/wiki/ControlAPIDesign

The Kea Control Agent comes with a few configuration parameters. These parameters need to be parsed and the agent configured. We should use Bison in the same way as we do for other Kea processes.

Subtickets

Change History (14)

comment:1 Changed 12 months ago by marcin

  • Milestone changed from Kea-proposed to Kea1.2

comment:2 Changed 12 months ago by fdupont

I suggest:

  • you write a grammar explaining what you want
  • either you write the flex and bison files following the trac5014_phase2 style, or you write a first version of these files, or you give this task to someone else (e.g., me but not I'll be on holiday from the 3th to the 7th)
  • you decide a few critical details as for instance should be the parser reentrant (the no answer is simpler but introduces a constraint)

comment:3 Changed 12 months ago by marcin

  • Component changed from management API to remote-management

comment:4 Changed 10 months ago by tomek

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

comment:5 Changed 9 months ago by tomek

  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing

The flex/bison parser is now ready for review. The diff is 7,7KLOC already, so I decided move the Element tree to CtrlAgentConfigContext? parsing to a separate ticket (see #5134).

Proposed ChangeLog?:

12XX.	[func]		tomek
	Bison parser implemented for Control-agent. The code is able
	to syntactically parse input configuration, but the output
	is not used yet.
	(Trac #5076, git tbd)

comment:6 Changed 9 months ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:7 follow-up: Changed 9 months ago by fdupont

Last edited 9 months ago by fdupont (previous) (diff)

comment:8 Changed 9 months ago by fdupont

The lexer should use more syntactic contexts. I'll address this when the review will be finished.

I have a concern about http-host: currently it means all addresses, e.g. for localhost it is 127.0.0.1 and ::1. I'll see further in the code if it is what we want...

lexer keywords don't seem to be in a logical order (if such a thing exists).

the lexer prefix is still parser6_ (obviously it was forgotten: I'll fix this).

the parser is wrong in its lack of syntactic context handling...

some missing ; in the parser (even they are not required they mark the end of a rule so are needed for readability).

missing blank line in the parser (#5110 has the same so I'll fix it at the source too).

Same for determing and syntactix

parser_unittests.cc includes cc/dhcp_config_error.h which is a temporary file (not a real issue)

The file name should be parser_unittest.cc (without the last 's'), I'll rename all the files in a specific commit.

The review is finished: I'll update the lexer and parser to the correct use of syntactic context.

comment:9 Changed 9 months ago by fdupont

trac5076 d8ccdaa..e339343 for unit test C++ file renames.

comment:10 Changed 9 months ago by fdupont

  • Owner changed from fdupont to tomek

Did the syntactic context update, regenerated flex and bison files.
Please pull, review and merge.

comment:11 in reply to: ↑ 7 ; follow-up: Changed 9 months ago by fdupont

Replying to fdupont:

I have a concern more general than the agent: for flex and bison files the Makefile.am put in source *lexer.ll but *parser.cc. IMHO it should be only the generated files and the .ll and .yy files must be EXTRA_DIST. Note until we pass the generate parser flag to distcheck it should not be a critical issue.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 months ago by tomek

Replying to fdupont:

Replying to fdupont:

I have a concern more general than the agent: for flex and bison files the Makefile.am put in source *lexer.ll but *parser.cc. IMHO it should be only the generated files and the .ll and .yy files must be EXTRA_DIST. Note until we pass the generate parser flag to distcheck it should not be a critical issue.

Yes, that's a mistake in the makefiles. Let's wait till we have D2 and agent parsers merged, then we can have one ticket to fix all 4 (or 5 if lib/eval has the same problem).

comment:13 in reply to: ↑ 12 Changed 9 months ago by fdupont

Replying to tomek:

Replying to fdupont:

Replying to fdupont:

I have a concern more general than the agent: for flex and bison files the Makefile.am put in source *lexer.ll but *parser.cc. IMHO it should be only the generated files and the .ll and .yy files must be EXTRA_DIST. Note until we pass the generate parser flag to distcheck it should not be a critical issue.

Yes, that's a mistake in the makefiles. Let's wait till we have D2 and agent parsers merged, then we can have one ticket to fix all 4 (or 5 if lib/eval has the same problem).

=> 4 (lib/eval is correct). I've already added this point in the cleanup ticket so we likely shan't forget it.

comment:14 Changed 9 months ago by tomek

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 20

I have reviewed Francis' changes and they're ok.

It looks like to be ready to go. Code merged. Closing ticket.

Thanks for the review.

Note: See TracTickets for help on using tickets.