Opened 7 years ago

Closed 7 years ago

#2994 closed enhancement (fixed)

Add selected few (3-4) hooks into DHCPv4 server

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130731
Component: dhcp4 Version:
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 (last modified by tomek)

This is a sort of pilot ticket. Its goal is to implement 3-4 selected hook points to get practical understanding of the hook mechanism. Depending on how it goes, we will revisit (tweak) our hook design or will go ahead with implementation of the other hooks (see ticket #2983).

Subtickets

Change History (8)

comment:1 Changed 7 years ago by tomek

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

comment:2 Changed 7 years ago by jwright

  • Summary changed from Add selected few (3-4) hoots into DHCPv4 server to Add selected few (3-4) hooks into DHCPv4 server

comment:3 Changed 7 years ago by tomek

  • Description modified (diff)
  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

The code is ready for review. The following hooks have been implemented: pkt4_receive, subnet4_select, lease4_select, pkt4_send.

Developer's Guide has been updated. Also couple minor mistakes in DHCPv6 docs were fixed as well.

Hopefully, this code mimics all changes done in #2995 (currently merged).

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 049d11fa1b6b08e8ad866686e57726e5906ced1c

I've updated the .mes files - please pull and review.

ChangeLog
Don't forget to update with the commit ID after the merge.

doc/devel/mainpage.dox
I suggest taking the "Hooks API" entries out of the maintenance guide. The topics "Other DHCPv{4,6} Topics" mention hooks and point to the appropriate section. Also, the Hooks API is aimed at the user, not the DHCP maintainer.

It looks as if you've omitted to check in the DHCPv4 hooks document.

src/bin/dhcp4/dhcp4_srv.cc
Dhcp4Srv constructor - remove the call to loadLibraries(). It's not needed here, as before the configuration is set the hooks framework will work as if no libraries are loaded.

Dhcp4Srv::{receive,send}Packet - return type should be on a separate line according to the coding standards.

Dhcp4Srv::{run,selectSubnet} - remove the getHooksManager() calls - most HooksManager methods have static equivalents.

Dhcp4Srv::selectSubnet - instead of converting the address to text format and check against the string "0.0.0.0", it is likely to be faster to use the conversion operator that returns a uint32_t (i.e. "if (relay != 0) {"). Alternatively, it might be clearer to declare 'static const IOAddress notset("0.0.0.0")' and compare against that.

Dhcp4Srv::sanityCheck: when checking the hardware address, even single-line "if" blocks need braces.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
NakedDhcpv4Srv: minor issue but the headers for the newly-created methods (sendPacket, receivePacket etc) seem to use a much short line-length than the header for the constructor (or the header for the member variables).

HooksDhcpv4SrvTest::generateSimpleDiscover: after we have delivered this Comcast work and we have a breathing space, we may want to consider writing a function that allows the creation of a binary DHCP packet from a texttual description - the DNS tests have this (see files in /src/lib/dns/tests/testdata). It should make the creation (and readibility) of unit tests easier.

src/lib/dhcpsrv/alloc_engine.cc
createLease4: no need to clear the "skip" flag in the CalloutHandle - callCallouts does that.

reuseExpiredLease (V4 version): the V6 version of the method gets passed a Subnet6Ptr, but this version gets passed a general SubnetPtr - why the difference?

reuseExpiredLease (V4 version): I would clarify the comment about the pointer cast. Something like:

Convert the general subnet pointer to a pointer to a Subnet4.  Note that
because we are using boost smart pointers here, we need to do the cast
using the boost version of dynamic_pointer_cast.

src/lib/dhcpsrv/alloc_engine.h
createLease{4,6} header: strictly speaking, lease callouts will only be executed if the parameter is passed and there are callouts present on the hook.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
General: use HooksManager::loadLibraries() instead of HooksManager::getHooksManager().loadLibraries().

lease4_select: Instead of "EXPECT_EQ(callback_fake_allocation_, false)", EXPECT_FALSE is briefer and clearer.

lease4_select: The comment starting "Check if all expected parameters are reported" highlights a potential future problem. Why not solve it now? Before the EXPECT statement add the line:

sort(callback_argument_names_.begin(), callback_argument_names_.end());

(...assuming that you've coded the expected_names in alphabetic order.)

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

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit 049d11fa1b6b08e8ad866686e57726e5906ced1c

I've updated the .mes files - please pull and review.

Thank you. Your changes look fine.

ChangeLog
Don't forget to update with the commit ID after the merge.

doc/devel/mainpage.dox
I suggest taking the "Hooks API" entries out of the maintenance guide. The topics "Other DHCPv{4,6} Topics" mention hooks and point to the appropriate section. Also, the Hooks API is aimed at the user, not the DHCP maintainer.

Ok, removed. The DHCPv4 and DHCPv6 hooks are listed in

It looks as if you've omitted to check in the DHCPv4 hooks document.

src/bin/dhcp4/dhcp4_srv.cc
Dhcp4Srv constructor - remove the call to loadLibraries(). It's not needed here, as before the configuration is set the hooks framework will work as if no libraries are loaded.

Removed.

Dhcp4Srv::{receive,send}Packet - return type should be on a separate line according to the coding standards.

Fixed.

Dhcp4Srv::{run,selectSubnet} - remove the getHooksManager() calls - most HooksManager methods have static equivalents.

Fixed.

Dhcp4Srv::selectSubnet - instead of converting the address to text format and check against the string "0.0.0.0", it is likely to be faster to use the conversion operator that returns a uint32_t (i.e. "if (relay != 0) {"). Alternatively, it might be clearer to declare 'static const IOAddress notset("0.0.0.0")' and compare against that.

Although it was me who added the uint32_t operator, I don't like it at all. It is too error prone, e.g. logging using arg(addr); will compile ok, but the actual code will try to log uint32_t. That will look ugly for v4 and will throw for v6. I plan to remove the uint32_t conversion operator from IOAddress one day. For that reason I chose to implement your second proposal - static const IOAddress.

Dhcp4Srv::sanityCheck: when checking the hardware address, even single-line "if" blocks need braces.

Added.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
NakedDhcpv4Srv: minor issue but the headers for the newly-created methods (sendPacket, receivePacket etc) seem to use a much short line-length than the header for the constructor (or the header for the member variables).

Fixed new comments and couple existing ones. On a related note, BIND10 coding guidelines should be updated. We had an agreement that the code width should be limited to 80 if possible, but in some cases it may be wider, up to 100 characters. That was only agreed in bind10 chatroom, but the guidelines were never updated.

HooksDhcpv4SrvTest::generateSimpleDiscover: after we have delivered this Comcast work and we have a breathing space, we may want to consider writing a function that allows the creation of a binary DHCP packet from a texttual description - the DNS tests have this (see files in /src/lib/dns/tests/testdata). It should make the creation (and readibility) of unit tests easier.

Agree. That one was painful. I also have a code in Dibbler that take hexstring from Wireshark, so I can copy paste a real packet into C++ test code. It is easy to do. I'm willing to donate that code. We may also consider developing a support for libpcap in our tests. That would be a good robustness test. I have a collection of various (sometimes odd) DHCPv6 packets. It would be useful to throw them at our implementation and see what happens.

src/lib/dhcpsrv/alloc_engine.cc
createLease4: no need to clear the "skip" flag in the CalloutHandle - callCallouts does that.

Removed.

reuseExpiredLease (V4 version): the V6 version of the method gets passed a Subnet6Ptr, but this version gets passed a general SubnetPtr - why the difference?

I tried to unify (SubnetPtr?) the alloc engine code as far as I could, but hit a problem that would require doing dynamic_pointer_cast. After that I reverted back to Subnet4Ptr and Subnet6Ptr, but apparently I missed some spots. I think now that we have the code up and running, we may try again to use SubnetPtr? more. Anyway, ultimately the library user needs to get a specific (Subnet4Ptr or Subnet6Ptr) pointer, so he won't have to do any dynamic_casts.

reuseExpiredLease (V4 version): I would clarify the comment about the pointer cast. Something like:

Convert the general subnet pointer to a pointer to a Subnet4.  Note that
because we are using boost smart pointers here, we need to do the cast
using the boost version of dynamic_pointer_cast.

Done as suggested.

src/lib/dhcpsrv/alloc_engine.h
createLease{4,6} header: strictly speaking, lease callouts will only be executed if the parameter is passed and there are callouts present on the hook.

Comment updated.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
General: use HooksManager::loadLibraries() instead of HooksManager::getHooksManager().loadLibraries().

lease4_select: Instead of "EXPECT_EQ(callback_fake_allocation_, false)", EXPECT_FALSE is briefer and clearer.

Done.

lease4_select: The comment starting "Check if all expected parameters are reported" highlights a potential future problem. Why not solve it now? Before the EXPECT statement add the line:

sort(callback_argument_names_.begin(), callback_argument_names_.end());

(...assuming that you've coded the expected_names in alphabetic order.)

Added sorting routines.

I also checked in the missing dhcp4_hooks.dox file.

comment:7 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 89d6be56eefa1ed79d9c0cfb8112243e1435beb6

src/bin/dhcp4/dhcp4_hooks.dox
subnet4_select: do you mean to say that "prefixes" will be assigned here?

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
I would add

#include <algorithm>

to the file to ensure that the definition of sort() is present even if changes are made to other header files.

These issues are minor. Please merge after you have fixed them - I don't need to see the code again.

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

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

Replying to stephen:

Reviewed commit 89d6be56eefa1ed79d9c0cfb8112243e1435beb6

src/bin/dhcp4/dhcp4_hooks.dox
subnet4_select: do you mean to say that "prefixes" will be assigned here?

There is no prefix delegation for DHCPv4. It was a leftover from DHCPv6. Removed.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
I would add

#include <algorithm>

to the file to ensure that the definition of sort() is present even if changes are made to other header files.

Added.

These issues are minor. Please merge after you have fixed them - I don't need to see the code again.

Thank you for your review.

Code merged, closing ticket.

Note: See TracTickets for help on using tickets.