Opened 3 years ago

Closed 3 years ago

#5031 closed enhancement (complete)

Dhcp4: Migrate hook libraries configuration

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

Description

This ticket covers step 6 of the plan laid out in #5017:

  1. Migrate hook libraries configuration (HooksLibrariesParser);

For overview see #5017. More details will become available when #5015 is done.

Subtickets

Change History (12)

comment:1 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

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

comment:3 Changed 3 years ago by tomek

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

comment:4 Changed 3 years ago by fdupont

Should check that #4496 issue is not reintroduced.

comment:5 Changed 3 years ago by tomek

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

The code is now ready for review. It also covers #5041.

Proposed ChangeLog:

12XX.	[func]		tomek
	Hooks-libraries parser migrated to SimpleParser. The hook
	libraries are now always unloaded during reconfiguration,
	regardless if the new configuation has hooks-libraries specified
	or not.
	(Trac #5031, #5041, git tbd)

Also, please note that this work has uncovered a bug in HooksManager? (see #5095) that affected 5 unit-tests. Those tests are now disabled. Appropriate comment was added in #5095 to re-enable them.

Last edited 3 years ago by tomek (previous) (diff)

comment:6 Changed 3 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:7 Changed 3 years ago by fdupont

I took the ticket for review but you can preempt me (it is 19h30 local time, tomorrow is Saturday, the day after both a Sunday and a public holiday in most countries, etc).
BTW where can we discuss about a way to hook the [re-]config operation (e.g. #5041) ?

comment:8 Changed 3 years ago by fdupont

I fixed some spelling errors in dhcp_parser.cc but I recommend to add some comments for check against things the flex/bison parser cannot produce... Anyway please pull and check my changes.

I have 2 concerns about the loadLibraries:

  • its documentation does not reflect the fact the current library set is first and unconditionally unloaded.
  • the return code from the hook library (with same function name) is ignored. BTW it is a known bug.

I am looking at #5095 as IMHO it should be fixed (or at least investigated) before going further.

comment:9 Changed 3 years ago by tomek

  • Parent Tickets set to 5019

comment:10 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Oops, I kept the ticket even I finished. BTW #5095 is ready for review.

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

#5095 is being reviewed so there is no reason to not merge this one.

comment:12 in reply to: ↑ 11 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 8 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 8 to 9

Replying to fdupont:

#5095 is being reviewed so there is no reason to not merge this one.

Fixed the two things you mentioned in loadLibraries (doc update and it now throws if the hooksmanager call returns false).

I asked Stephen to take a look as Hooks manager is his expertise area. Happy to see that #5095 is being reviewed.

Thanks for the review. Code merged. Closing ticket.

Note: See TracTickets for help on using tickets.