Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#4027 closed enhancement (complete)

4o6: Implement Pkt4o6 class to convey DHCPv4-over-DHCPv6 packet

Reported by: tomek Owned by: jinmei
Priority: low Milestone: Kea1.0-beta
Component: dhcp4 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

See Dhcp4o6Design for details. This is the first step required for implementing RFC7341.

It will be used to store DHCPv4-over-DHCPv6 packet as Pkt4-derived class, so it could be passed around in DHCPv4 server code.

Subtickets

Change History (15)

comment:1 Changed 5 years ago by fdupont

I am not sure it should be a Pkt4-derived class. A DHCP406 message includes a DHCPv6 message which itself includes a DHCPv4 message. So in fact the only use of the proposal is really "pass around in DHCPv4 server code". Note perhaps it is enough but IMHO it is a bit too soon to decide.

comment:2 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from medium to low

per Sept 9 team meeting, accept 1.0 as low

comment:3 Changed 5 years ago by fdupont

I have a proposal but it needs more work to know if it makes the job. So please don't rush on this ticket...

comment:4 Changed 5 years ago by fdupont

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

I did some progress on my DHCP4o6 experiment so now I know what to put in this class. Taking the ticket...

comment:5 follow-up: Changed 5 years ago by tomek

  • Owner changed from fdupont to contributor
  • Status changed from accepted to assigned

Francis, please do not work on this. All tickets related to 4o6 are reserved for the Tsinghua University. As Tianxiang doesn't have an account in trac, I'll reassign this to a generic 'contributor' account.

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

Replying to tomek:

Francis, please do not work on this. All tickets related to 4o6 are reserved for the Tsinghua University. As Tianxiang doesn't have an account in trac, I'll reassign this to a generic 'contributor' account.

=> what is the level of help (not including the private fd4o6 branch which is not so private) which is acceptable? Perhaps a short description of what could be in the class is right?

comment:7 Changed 4 years ago by fdupont

Updated the design document about ticket splitting.

comment:8 Changed 4 years ago by tomek

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

This ticket has been implemented by Jinmei and pushed to trac4027 on our github repo. Tomek is reviewing it now.

comment:9 follow-up: Changed 4 years ago by tomek

I reviewed the code on trac4027 (github repo). Here are my comments.
Most of them are small, so I went ahead and fixed them. Please pull
and see if you agree with my suggestions.

The code compiles and unit-tests pass on Mac OS X 10.10.3.

Makefile.am
src/lib/dhcp/Makefile.am mentions dhcp4o6.h, but there's no such file.

pkt4o6.h
The Pkt4o6 class comment should reference RFC7341 and http://kea.isc.org/wiki/Dhcp4o6Design.

Pkt4o6 constructor. pkt4 parameter comment should be updated to
"content of the DHCPv4-message option". Updated.

Comment for pack() uses invalid doxygen references. The class names
should be capitalized (pkt4 -> Pkt4, pkt6-> Pkt6). Fixed this one.

s/beeb/been

pkt4o6.cc
Instead of DEFAULT_ADDRESS, you could use asiolink::IPV4_ZERO_ADDRESS().

Pkt4o6::pack()
It would be useful to add a comment that the code converts
OutputBuffer? to OptionBuffer?.

pkt4o6_unittest.cc
Pkt4o6Test should have a comment, even if it's a very brief one. Something
like "A Fixture class dedicated to testing of the Pkt4o6 class that
represents a DHCPv4-over-DHCPv6 packet. Please check if the comment I added is ok.

Fields in the Pkt4o6Test should have doxygen comments (/ rather than ).
Updated.

Each test should have a brief comment. I added two comments. Please
pull. If you're happy with them, there's nothing to do here.

I did: s/u_int8_t/uint8_t/.

I also added a @todo comment that we should do a test that processes
actual DHCP4o6 packet once we get a traffic capture of it. Just to
keep that on our todo list.


The only things left to do are:

  • replace DEFAULT_ADDRESS with asiolink::IPV4_ZERO_ADDRESS().
  • remove non-existing file dhcp4o6.h from Makefile.am

Once that is done, please update AUTHORS. The whole team is always
excited when we get a new contributor, so don't want to miss anyone.

With those changes in, please go ahead and merge.

Thank you a lot for your contribution.

comment:10 Changed 4 years ago by tomek

  • Owner changed from tomek to jinmei

comment:11 in reply to: ↑ 9 Changed 4 years ago by fdupont

Replying to tomek:

Makefile.am
src/lib/dhcp/Makefile.am mentions dhcp4o6.h, but there's no such file.

=> my fault (it was used by the IPC code).

comment:12 Changed 4 years ago by jinmei

I just pushed an updated branch.

Regarding DEFAULT_ADDRESS, I actually simply removed it since it's not used.

Besides, as commented in the commit I suspect this is a dangerous practice. I wouldn't bother to argue about it to block this task, but I suggest you revisit this practice in general on other occasion.

comment:13 Changed 4 years ago by jinmei

And I found I'm already included in AUTHORS.

comment:14 Changed 4 years ago by jinmei

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

comment:15 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.