Opened 3 years ago

Closed 3 years ago

#5098 closed enhancement (complete)

DhcpX: Migrate client class configuration

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea 1.2 - Mozilla Milestone 1
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5019, #5037
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 3 Internal?: no

Description


Subtickets

Change History (9)

comment:1 Changed 3 years ago by tomek

  • Parent Tickets set to 5019, 5037

comment:2 Changed 3 years ago by fdupont

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

comment:3 Changed 3 years ago by fdupont

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

Done. BTW I moved up the parser in dhcp*/json_config_parser.cc (IMHO this should not have bad side effects).
Other points:

  • previous code has an obvious bug when called a null pointer (deference the config if null to get its location)
  • both previous and new codes accept null name: I'd like to check this as it will make missing name too.
  • should we inherit next-server and *name from global?

So I am waiting for the review...

comment:4 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 Changed 3 years ago by tomek

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

I have reviewed the changes on trac5098 up to fa41a74a7c52a3e0ac30a10001887e2290977b63
and have couple comments:

client_class_def_parser.h
ExpressionParser? should not take the expression pointer, this
should be an argument for parse() method. This way the parser
is more stateless and there's no need to have a constructor
anymore. Unless you want to move family from the parse method
to the constructor.

This change would eliminate the need for local_expression_ and
for expression_ members.

Similar comment applies to ClientClassDefParser?. If you move
the class_dictionary parameter to parse method, you'd eliminate
the constructor, options_ and match_expr_ could become local
variables in parse().

client_class_def_parser.cc
You asked a question in line 112 whether name should be made
mandatory. I'm ok with that. In principle we could talk about
nameless classes that define a test and option values. But
our code internally tags packets with classes using strings.
So we would need to do a major code refactoring to support
that. It's much simpler to require classes to have unique names.

ClientClassDefListParser::parse I would prefer this method to take
SrvConfigPtr? instead of calling setClientClassDictionary on its own. I
have 2 reasons for that. First, it will make it obvious what the parse
method does and where it stores the parsed content. Second, one day we
may have some use cases for storing the output in places other than
the staging config. Maybe some unit-tests could simply use a local
instance of SrvConfig? without touching CfgMgr? at all. Or may one
day we could have command channel commands that add new classes?
Those could apply the change to the running, not staging config.

client_class_def_parser_unittest.cc
There's a lot of AF_INET tests, but there are only 4 for AF_INET6.
Can you add a couple of @todos in there?

comment:6 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Addressed!

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

  • Add Hours to Ticket changed from 2 to 1
  • Owner changed from tomek to fdupont
  • Total Hours changed from 2 to 3

The code compiled and unit-tests passed on Ubuntu 16.04.1 x64.

Thanks for making the name parameter mandatory. The exception
must log the approximate location where the name is missing.
As it covers two cases (not specified at all and specified as
empty string), I think the best way to log is to log the
parent scope. I did that. Please pull and review.

Since the parameter being mandatory is user visible change,
we need a changelog. If you don't have any preferences, here's
my proposal:

12XX.	[func]		fdupont
	Client classes parser migrated to SimpleParser. The 'name'
	parameter in 'client-classes' definitions is now mandatory.
	(Trac #5098, git tbd)

I have reviewed your other changes and they're ok. Please pull,
and if you agree with edit, please merge.

comment:8 in reply to: ↑ 7 Changed 3 years ago by fdupont

Replying to tomek:

The code compiled and unit-tests passed on Ubuntu 16.04.1 x64.

Thanks for making the name parameter mandatory. The exception
must log the approximate location where the name is missing.
As it covers two cases (not specified at all and specified as
empty string), I think the best way to log is to log the
parent scope. I did that. Please pull and review.

=> there is a try/catch to add the parent location so
I reserved your change.

Since the parameter being mandatory is user visible change,
we need a changelog. If you don't have any preferences, here's
my proposal:

12XX.	[func]		fdupont
	Client classes parser migrated to SimpleParser. The 'name'
	parameter in 'client-classes' definitions is now mandatory.
	(Trac #5098, git tbd)

I have reviewed your other changes and they're ok. Please pull,
and if you agree with edit, please merge.

=> I am merging, regen, add your proposal ChangeLog.

comment:9 Changed 3 years ago by fdupont

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

Merged. Closing. BTW perhaps we should consider to add the corresponding check (name parameter mandatory) in the grammar (action at closing bracket)...

Note: See TracTickets for help on using tickets.