Opened 7 years ago

Closed 7 years ago

#2983 closed enhancement (complete)

Add remaining (15+) Hooks into the DHCPv4 Server

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130904
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)

There is a pre-work for this ticket: couple selected hooks (#2994).

Add the appropriate remaining hook points to the BIND 10 DHCP4 server.

This ticket also includes the task of updating the relevant section of the DHCP Hooks design document with the actual API to be used (together with examples). Examples were never part of the estimates.

Subtickets

Change History (15)

comment:1 Changed 7 years ago by stephen

  • Component changed from documentation to dhcp4

comment:2 Changed 7 years ago by tomek

  • Description modified (diff)
  • Summary changed from Add Hooks into the DHCP4 Server to Add remaining (15+) Hooks into the DHCPv4 Server

comment:3 Changed 7 years ago by tomek

  • Description modified (diff)

comment:4 Changed 7 years ago by tomek

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

comment:5 Changed 7 years ago by tomek

  • Status changed from accepted to assigned

comment:6 Changed 7 years ago by tomek

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

The code is ready for review. It is an equivalent of its counterpart (#2984).

comment:7 Changed 7 years ago by tomek

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

comment:8 Changed 7 years ago by tomek

  • Status changed from accepted to assigned

comment:9 Changed 7 years ago by tomek

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

This ticket also includes a minor tweak in CalloutManager?. If an exception is thrown in user library, the text of that library is now printed in error message.

comment:10 Changed 7 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:11 Changed 7 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130821 to Sprint-DHCP-20130904

comment:12 follow-up: Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit 8abbdb589f6205c51f461541778f40dd355db60b

ChangeLog
This is ok.

src/bin/dhcp4/dhcp4_hooks.dox
pkt4_receive description: Suggest that the first sentence is modified:

this callout is executed when an incoming DHCPv4 packet is received and its content has been parsed
When using present tense it is confusing if the hook is called before or after parsing the message.

pkt4_receive:
By the time this hook is reached, that information has been already parsed...

Suggest that this:
data_ contains the incoming packet as raw buffer. By the time this hook is reached, that information has already parsed and is available though other fields in the Pkt4 object. For this reason, it doesn't make sense to modify it.

is rephrased:
By the time this hook is reached, the contents of the data_ field has been already parsed and stored in other fields. Therefore, the modification in the data_ field has no effect.
Or something like this....

Also, this may sound better:
The callout should sanity check all modifications as the server will use that data as is with no further checking.

lease4_select:
a value of true indicates that the lease won't be inserted into the database (DISCOVER), a value of false indicates that it will (REQUEST).

It doesn't make sense to modify it at this time., I wonder if there are an (negative) implications of modifying it?

this callout is executed when server's response is about to be sent back

object that contains the packet, with source and destination addresses set, interface over which it will be sent and a list of all options and relay information

All fields of the Pkt4 object can be modified at this time, except bufferOut_ . The highlighted name does not conform to the naming convention for variables. Is it a typo in the Developer's Guide or in the code?

buffer4_send:
this callout is executed when server's response is about to be sent back to the client

object that contains the packet, with source and destination addresses 'set, interface over which it will be sent and list of all options and relay information

The raw on-wire form is already prepared in bufferOut_ , again something is wrong with this name.

src/bin/dhcp4/dhcp4_messages.mes
DHCP4_HOOK_LEASE4_RELEASE_SKIP: typo in
If client requested release of multiple leases (by sending multiple IA options), the server will retain this particular lease and will proceed with other renewals as usual.

DHCP4_PACKET_DROP_NO_TYPE dropped packet received on interface %1: does not have msg-type option
It has to be rephrased because I read it as: There was a dropped packet on the interface X and then it turned out that this packet didn't have msg-type option, while actually it is that The packet has been dropped and the reason for it was that it hadn't had the msg-type option.

Also, typo in: THis is a debug message

src/bin/dhcp4/dhcp4_srv.cc
General comment: I suggest that you stick to 80 lines. I haven't seen this documented anywhere that we can do more than 80. Even if there was an informal agreement that we can, there are a lot of cases when you actually could do 80 lines by wrapping the arguments of the function invocation. I am using emacs on my laptop and I often have two windows open side-by-side. The 80 line text is displayed just fine. Anything wide gets wrapped and makes the code unreadable until I close the other buffer.

Shouldn't ''Hooks'' be named using lower case letters?

Dhcp4Hooks hooks;

With the recent changes the Dhcpv4Srv::run function is now over 250 lines long. It makes it very hard to read. Therefore I suggest that the parts of this function which process the certain callouts are enclosed in the private functions. These functions could return the boolean flags indicating whether to skip the next step or not. I guess it could even be a sole private function taking parameters.

line 355: should be Execute all callouts registered for packet4_send

line 394: the same issue

line 525: the same issue

process Discover and processRequest: Why is this? Why is this commented? And why it should be uncommented?

    /// @todo Uncomment this
    // sanityCheck(request, MANDATORY);

line 922: packet6_send

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

Tests look good to me. The only thing is to rename the test cases so as, instead of:
''simple_buffer4_receive'' you'll have ''simpleBuffer4Receive''.

src/lib/dhcp/pkt4.h

bufferOut_ should be renamed to buffer_out_.

Also the description should start with capital letter.

data_
The description should start with capital letter.

src/lib/dhcpsrv/alloc_engine.cc

renewLease4: It should be Execute all callouts registered for packet4_send

The rollback which is at the end of the function (for memfile) will not be valid anymore when I finish the ticket number #3083. With this ticket, I am making a change in memfile to return a copy of the lease, so as the change to the returned structure doesn't affect the lease in the lease container. I suggest that you add a todo to this ticket saying that the rollback is removed once 3083 is merged.

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

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit 8abbdb589f6205c51f461541778f40dd355db60b

ChangeLog
This is ok.

src/bin/dhcp4/dhcp4_hooks.dox
pkt4_receive description: Suggest that the first sentence is modified:

this callout is executed when an incoming DHCPv4 packet is received and its content has been parsed
When using present tense it is confusing if the hook is called before or after parsing the message.

Done.

pkt4_receive:
By the time this hook is reached, that information has been already parsed...

Done.

Suggest that this:
data_ contains the incoming packet as raw buffer. By the time this hook is reached, that information has already parsed and is available though other fields in the Pkt4 object. For this reason, it doesn't make sense to modify it.

is rephrased:
By the time this hook is reached, the contents of the data_ field has been already parsed and stored in other fields. Therefore, the modification in the data_ field has no effect.
Or something like this....

Done.

Also, this may sound better:
The callout should sanity check all modifications as the server will use that data as is with no further checking.

Done.

lease4_select:
a value of true indicates that the lease won't be inserted into the database (DISCOVER), a value of false indicates that it will (REQUEST).

Done.

It doesn't make sense to modify it at this time., I wonder if there are an (negative) implications of modifying it?

Its value not used anymore.

this callout is executed when server's response is about to be sent back

Done.

object that contains the packet, with source and destination addresses set, interface over which it will be sent and a list of all options and relay information

Done.

All fields of the Pkt4 object can be modified at this time, except bufferOut_ . The highlighted name does not conform to the naming convention for variables. Is it a typo in the Developer's Guide or in the code?

That's how that field used to be named. Renamed to buffer_out_.

buffer4_send:
this callout is executed when server's response is about to be sent back to the client

Done.

object that contains the packet, with source and destination addresses 'set, interface over which it will be sent and list of all options and relay information

Done.

The raw on-wire form is already prepared in bufferOut_ , again something is wrong with this name.

Fixed in the code and in the docs as well.


src/bin/dhcp4/dhcp4_messages.mes
DHCP4_HOOK_LEASE4_RELEASE_SKIP: typo in
If client requested release of multiple leases (by sending multiple IA options), the server will retain this particular lease and will proceed with other renewals as usual.

Good catch, but it's not what you meant. There are no multiple leases in DHCPv4 :) Removed the whole sentence altogether.

DHCP4_PACKET_DROP_NO_TYPE dropped packet received on interface %1: does not have msg-type option
It has to be rephrased because I read it as: There was a dropped packet on the interface X and then it turned out that this packet didn't have msg-type option, while actually it is that The packet has been dropped and the reason for it was that it hadn't had the msg-type option.

Rephrased. Hopefully is reads better now.

Also, typo in: THis is a debug message

Fixed.

src/bin/dhcp4/dhcp4_srv.cc
General comment: I suggest that you stick to 80 lines. I haven't seen this documented anywhere that we can do more than 80. Even if there was an informal agreement that we can, there are a lot of cases when you actually could do 80 lines by wrapping the arguments of the function invocation. I am using emacs on my laptop and I often have two windows open side-by-side. The 80 line text is displayed just fine. Anything wide gets wrapped and makes the code unreadable until I close the other buffer.

Wrapped some of the longer lines, at least those related to hooks and couple others. There are still some lines over 80 chars, but I don't want to turn this ticket into code beautification exercise.

Shouldn't ''Hooks'' be named using lower case letters?

Dhcp4Hooks hooks;

Yes and no. Hooks is a global variable. Had it been named with lower case letters, it would strongly suggest that it is a local variable, which is not. Using capital letter was my idea to signify that this is a global object. If you think that it is not a good idea and all lower case is better, I'll update it.

With the recent changes the Dhcpv4Srv::run function is now over 250 lines long. It makes it very hard to read. Therefore I suggest that the parts of this function which process the certain callouts are enclosed in the private functions. These functions could return the boolean flags indicating whether to skip the next step or not. I guess it could even be a sole private function taking parameters.

I've created a ticket for this #3117. The same issue is with v6.

line 355: should be Execute all callouts registered for packet4_send
line 394: the same issue
line 525: the same issue

Fixed.

process Discover and processRequest: Why is this? Why is this commented? And why it should be uncommented?

    /// @todo Uncomment this
    // sanityCheck(request, MANDATORY);

While implementing hooks, I made the callouts to modify incoming packets in a way that would later cause the server to drop the packet, because of my modification. I wanted to use the drop as a verification technique that hook's modification was indeed affecting the server. In the process I discovered that many methods do not call sanityCheck() function. However, once I added it around 10 tests started to fail. And this ticket is about implementing hooks and not improving sanity checking. I've added separate ticket for that (#3116).

line 922: packet6_send

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

Tests look good to me. The only thing is to rename the test cases so as, instead of:
''simple_buffer4_receive'' you'll have ''simpleBuffer4Receive''.

It was explained in one comment it was a normal camel notation, underscore, name of the callout. For example: changeClientId_pkt4_send. But I agree that it was different than all other test names, so I've updated them. It is now somewhat not obvious to get the callout names from the test names.

src/lib/dhcp/pkt4.h

bufferOut_ should be renamed to buffer_out_.

Also the description should start with capital letter.

Renamed and capitalized the description.

data_
The description should start with capital letter.

Description updated.

src/lib/dhcpsrv/alloc_engine.cc

renewLease4: It should be Execute all callouts registered for packet4_send

Fixed.

The rollback which is at the end of the function (for memfile) will not be valid anymore when I finish the ticket number #3083. With this ticket, I am making a change in memfile to return a copy of the lease, so as the change to the returned structure doesn't affect the lease in the lease container. I suggest that you add a todo to this ticket saying that the rollback is removed once 3083 is merged.

Added.

I believe I addressed all he issues. Thanks for the review.

comment:14 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit fd47f18f898695b98623a63a0a1c68d2e4b37568.

Changes are ok. Please merge.

comment:15 Changed 7 years ago by tomek

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

Thanks for the review. Code merged. Closing ticket.

Note: See TracTickets for help on using tickets.