Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4081 closed enhancement (complete)

implement Token classes for string, option value, == operator (client class)

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea1.0-beta
Component: Unclassified 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: 0 Internal?: no

Description

This is the first task required for client classification. Its goal is to implement several classes that would represent basic expressions: constant string, value of an option, equal operator.

See ClientClassificationDesign for details.

Subtickets

Change History (8)

comment:1 Changed 4 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0
  • Owner set to tomek
  • Status changed from new to assigned

comment:2 Changed 4 years ago by tomek

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

The code has been implemented. Please review.

Couple notes:

  1. The new directory is called eval. While it is currently planned for classification only, the code is generic and can be used to evaluate any expression. Perhaps one day we'll do something similar to ISC, where option value can be an expression?
  1. The name eval is sufficiently different than existing names. I initially called it expr (short for expressions), but it was too similar to existing exceptions, thus confusing automatic name completion.
  1. I added eval_messages, with a single entry that is not used yet. In principle, this shouldn't be in this ticket. However, due to time constraints, we need to implement multiple tickets in parallel, so I thought #4081 (as the first ticket in client classification) could be a common base that other tickets could build on.

comment:3 Changed 4 years ago by sar

  • Owner changed from UnAssigned to sar

comment:4 follow-up: Changed 4 years ago by sar

  • Owner changed from sar to tomek

I updated Doxyfile to include the eval directory and fixed a small number of typos,
please do a pull before continuing.

src/lib/eval/Makefile.am b/src/lib/eval/Makefile.am

Do we need the additional AM_CXXFLAGS for boost on GCC?

src/lib/eval/eval.dox b/src/lib/eval/eval.dox

src/lib/eval/eval_messages.mes

Should the copyrights be simply 2015?

src/lib/eval/tests/Makefile.am b/src/lib/eval/tests/Makefile.am

Do we need to include $(LOG4CPLUS_LIBS)?

src/lib/eval/token.h

In the TokenString? constructor shouldn't the string be tagged as const?

In the comment for TokenOption::evaluate() it states that the packet isn't used which isn't true.

src/lib/eval/token.cc

I think we may end up wanting or needing more information in the throw. I'm wondering if we may
want to include an indication of the operation that throw the exception. Or do you plan to have
whatever catches the exception figure that out and handle it appropriately (probably log something
and keep on trying to run)?

src/lib/eval/tests/token_unittest.cc

I believe there should be an optionString4 and optionString6 to verify an option can be extracted from both v4 and v6 packets.

comment:5 in reply to: ↑ 4 Changed 4 years ago by tomek

  • Owner changed from tomek to sar

Replying to sar:

I updated Doxyfile to include the eval directory and fixed a small number of typos,
please do a pull before continuing.

src/lib/eval/Makefile.am b/src/lib/eval/Makefile.am

Do we need the additional AM_CXXFLAGS for boost on GCC?

Do we? I presume you're suggesting adding

AM_CXXFLAGS += $(WARNING_NO_MISSING_FIELD_INITIALIZERS_CFLAG)

I looked at the related boost bug (https://svn.boost.org/trac/boost/ticket/3477) and it seems to be closed/worksforme a long time ago. Anyway, to be on the safe side, I added it as suggested.

src/lib/eval/eval.dox b/src/lib/eval/eval.dox

src/lib/eval/eval_messages.mes

Should the copyrights be simply 2015?

Yes. Corrected.

src/lib/eval/tests/Makefile.am b/src/lib/eval/tests/Makefile.am

Do we need to include $(LOG4CPLUS_LIBS)?

Yes. Added.

src/lib/eval/token.h

In the TokenString? constructor shouldn't the string be tagged as const?

It is const now.

In the comment for TokenOption::evaluate() it states that the packet isn't used which isn't true.

Corrected.

src/lib/eval/token.cc

I think we may end up wanting or needing more information in the throw. I'm wondering if we may
want to include an indication of the operation that throw the exception. Or do you plan to have
whatever catches the exception figure that out and handle it appropriately (probably log something
and keep on trying to run)?

As discussed on Jabber, there's only so much information we can share if we catch the problem that late. The stack has been built incorrectly. We can only say at this stage that that stack is malformed. This issue should be caught while building the stack. I'll keep that in mind when coding #4088.

src/lib/eval/tests/token_unittest.cc

I believe there should be an optionString4 and optionString6 to verify an option can be extracted from both v4 and v6 packets.

Good suggestion. Added.


Oh, I just realized that I haven't proposed a ChangeLog? entry. Normally we would skip it as the change is not something that users could take advantange of, but it could be useful for people to be notified that there's a new library coming. I presume only package maintainers would care, though.

10XX.	[func]		tomek
	A new library, libkea-eval has been edded. It is not functional
	yet, but its purpose is to provide a generic expression
	evaluations that will be used in the upcoming client classification.
	(Trac #4081, git tbd)

comment:6 Changed 4 years ago by sar

  • Owner changed from sar to tomek

reviewed and ready to merge

comment:7 Changed 4 years ago by tomek

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

Thanks for the review. Merged. Closing ticket.

comment:8 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.