Opened 4 years ago

Closed 4 years ago

#4297 closed enhancement (complete)

Hook library should be able to access it its configuration parameters

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea1.1
Component: hooks 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

The current schema allows hook library to to have parameters. However, there's no way to exposed those paraemters to the hook library. This ticket covers implementing such ability.

Subtickets

Change History (11)

comment:1 Changed 4 years ago by tomek

  • Priority changed from medium to high

comment:2 Changed 4 years ago by tomek

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

comment:3 Changed 4 years ago by tomek

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

The code is now ready for review.

Proposed ChangeLog? entry:

1XXX.	[func]		tomek
	It is now possible to specify parameters for hook libraries.
	(Trac 4297, git tbd)

comment:4 Changed 4 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:5 follow-up: Changed 4 years ago by fdupont

  • Owner changed from fdupont to tomek

IMHO the idea to push data.* into src/lib/util was better than to move src/lib/hook in the build order (the async crypto support used a hook for validation) but it can be done later so it is not critical (and obviously the current solution was easier).
Note anyway a followup ticket must be pushed to fix dependency order in all Makefiles using libhook.

In src/lib/dhcpsrv/parsers/dhcp_parsers.cc HooksLibrariesParser::build IMHO the parameters variable should be pushed into the FOREACH. I've checked: it is no used outside the FOREACH block.

In the same routine spelling: a files -> either 'a file' or 'files'

in src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc the new HooksLibrariesParameters? should be with other hook tests.

In the same test changed should be initialized to false (at least for code readability).

In the same test in a comment no <newline> no -> no, parameter -> parameter

in src/lib/hooks/libinfo.h formally we should include <utility> for std::pair?

in src/lib/hooks/Makefile.am libcc.a is not at the right place. BTW I have the same concern for the tests/Makefile.am

in src/lib/hooks/hooks_manager.h I am not happy with "Returns the names of the loaded libraries and its parameters." (its -> their?)

in src/lib/hooks/hooks_user.dox: e.g. -> e.g., repesented -> represented and elemented -> elements

in the same dox file NULL doesn't exist in C++, I suggest null pointer instead.

for Stephen, is "to check if the structure has expected type" correct or should be a "the" before "expected".

in src/lib/hooks/library_handle.cc the - in "Some indexes have special meaning:" have double (so ambiguous) usage. I recommend to change the first by anything else, e.g., "*"

in the same file: NULL -> null pointer

in src/lib/hooks/library_handle.h missing @return for getParameter()?

in src/lib/hooks/tests/callout_params_library.cc I recommend to return different values on errors.

I have a Jenkins working on it but I don't expect errors.

comment:6 follow-up: Changed 4 years ago by stephen

Also, as hooks/library_handle.h now includes cc/data.h the latter file should be installed on the target system along with all the other .h files needed to build hook libraries.

comment:7 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by tomek

Replying to fdupont:

IMHO the idea to push data.* into src/lib/util was better than to move src/lib/hook in the build order (the async crypto support used a hook for validation) but it can be done later so it is not critical (and obviously the current solution was easier).
Note anyway a followup ticket must be pushed to fix dependency order in all Makefiles using libhook.

Created #4491. If the ticket is accepted in 1.1 (hopefully it will), that's something I will focus on next.

In src/lib/dhcpsrv/parsers/dhcp_parsers.cc HooksLibrariesParser::build IMHO the parameters variable should be pushed into the FOREACH. I've checked: it is no used outside the FOREACH block.

Moved.

In the same routine spelling: a files -> either 'a file' or 'files'

Corrected.

in src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc the new HooksLibrariesParameters? should be with other hook tests.

Moved as suggested.

In the same test changed should be initialized to false (at least for code readability).

Initialized.

In the same test in a comment no <newline> no -> no, parameter -> parameter

Your way of describing the problem is not 100% clear to me, but I believe the problem
was duplicate "no" (before and after new line). That's the issue I fixed. If there's
anything else to fix, please let me know.

in src/lib/hooks/libinfo.h formally we should include <utility> for std::pair?

Good catch. It includes <utility> now.

in src/lib/hooks/Makefile.am libcc.a is not at the right place. BTW I have the same concern for the tests/Makefile.am

I do not understand. Can you explain?

in src/lib/hooks/hooks_manager.h I am not happy with "Returns the names of the loaded libraries and its parameters." (its -> their?)

Corrected.

in src/lib/hooks/hooks_user.dox: e.g. -> e.g., repesented -> represented and elemented -> elements

Corrected.

in the same dox file NULL doesn't exist in C++, I suggest null pointer instead.

Updated as suggested.

for Stephen, is "to check if the structure has expected type" correct or should be a "the" before "expected".

I think "the" fits there nicely. Added without bothering Stephen.

in src/lib/hooks/library_handle.cc the - in "Some indexes have special meaning:" have double (so ambiguous) usage. I recommend to change the first by anything else, e.g., "*"

Ok, updated list to stars.

in the same file: NULL -> null pointer

Updated.

in src/lib/hooks/library_handle.h missing @return for getParameter()?

It is now described.

in src/lib/hooks/tests/callout_params_library.cc I recommend to return different values on errors.

Ok, I see little point in that (the test will fail anyway), but it's a small change, so I did it anyway.

I have a Jenkins working on it but I don't expect errors.

Cool.

Ok, this addresses most of the comments, except the Makefile ones. I can address them as I get a better explanation what the problem is. If it's faster to fix it yourself than explain, feel free to push the correction. But I would like to understand what the problem is, so expect questions afterwards :)

Thanks for your review.

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

Replying to stephen:

Also, as hooks/library_handle.h now includes cc/data.h the latter file should be installed on the target system along with all the other .h files needed to build hook libraries.

Good catch. cc/data.h is now installed.

comment:9 Changed 4 years ago by tomek

  • Owner changed from tomek to fdupont

comment:10 in reply to: ↑ 7 Changed 4 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

In the same test in a comment no <newline> no -> no, parameter -> parameter

Your way of describing the problem is not 100% clear to me, but I believe the problem
was duplicate "no" (before and after new line). That's the issue I fixed. If there's
anything else to fix, please let me know.

=> the first one was the duplicate "no", the second was a partameter spelling the editor auto spell messed. I fixed it in the branch so please push.

in src/lib/hooks/Makefile.am libcc.a is not at the right place. BTW I have the same concern for the tests/Makefile.am

I do not understand. Can you explain?

=> library dependencies must be now (the style guide was updated) in the built reverse order so as the hooks cryptolink dns cc was changed into cryptolink dns cc hooks all the Makefile.am files which refer to one of these libraries should be updated. Note it is fine to note in #4491 to cleanup this in particular if it is adopted for the current milestone at a good priority (laziness is a good principle, my sister even offered a book about this for one of my previous birthdays...).

comment:11 Changed 4 years ago by tomek

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

Thanks for your review. Pulled your change, merged to master. Added comment in #4491 what should be done. Code pushed. Closing ticket.

Note: See TracTickets for help on using tickets.