Opened 8 years ago

Closed 8 years ago

#1956 closed enhancement (fixed)

Implement perfdhcp IPv6 packet handling

Reported by: stephen Owned by: marcin
Priority: medium Milestone: DHCP-Sprint-20120611
Component: perfdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 40 Add Hours to Ticket: 2
Total Hours: 39.5 Internal?: no


Extract the handling of IPv6 packets in perfdhcp into a separate class.


Change History (10)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20120528

comment:2 Changed 8 years ago by marcin

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

comment:3 Changed 8 years ago by marcin

  • Status changed from assigned to accepted

comment:4 Changed 8 years ago by marcin

  • Add Hours to Ticket changed from 0 to 37.5
  • Estimated Difficulty changed from 0 to 40
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 8 years ago by marcin

  • Total Hours changed from 0 to 37.5

comment:6 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 Changed 8 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit bff3d4786c50018143abe2316c9e837f24c52e81

I've corrected the comments in some of the method headers and pushed to the repository - "git pull" before making any more changes.

In setData():

  • Use "std::distance" to calculate the distance between two iterators. It does not make any assumptions about the type of iterators.
  • The "while" loop can be replaced by a call to std::copy().
  • The function arguments are of type "OptionBufferConstIterator" in the function declaration, but "const OptionBufferConstIterator" in the function definition.


  • The unit tests should be extended to test this method.

If the code were such that the timestamp was initialized when the packet was created and updateTimestamp() did nothing, the tests would still pass. A check needs to be made that the timestamp is invalid after packet construction.

Also, if the ASSERT_FALSE failed, the test would have a memory leak, as pkt would not be deleted when the function exits. Suggest encapsulating pkt in a boost::scoped_ptr and getting rid of the "delete".

The first and third constructors are redundant: just specify a default value of zero for the last (offset) parameter in the argument list of the second and fourth constructors.

Constructor: I would prefer that a method be added to set the transaction ID to the Pkt6 class and used, rather than modifying a member variable of the parent directly. (I think that variables in the parent class are set protected for test use only.)

rawPack(): A comment should be added to the check on transaction id. It is not clear (to me) what the restrictions on the transaction ID offset are.

rawPack(): There is a second call to bufferOut_.clear() ("Seek to the transaction id position in output buffer"). At the moment, it looks as if all the data just written to the buffer is destroyed (although I see looking at the code for OutputBuffer that it just clears the internal pointer.) As it is feasible that a future change to buffer could erase the data if clear() is called (perhaps as a debugging aid), this should be altered. Perhaps write the data in three chunks: upto the transaction ID, the transaction ID, and the part after the ID?

rawPack(): suggest you liaise with Tomek to define the offsets for the message type and transaction ID in the dhcp6.h file. (Admittedly, assuming that the message type is the first octet in the buffer is pretty safe. All the same, hard-wiring "magic numbers" in the code is usually a bad idea.)

Class Offset: the first constructor can be removed - specify a default value of 1 for the offset argument of the second constructor.

Also, I'm not convinced that use of the Offset class does give any real advantage. The comments suggest that explicitly specifying offset reduces the chances of getting arguments mixed up with the offset: but what about getting other arguments mixed up?

Constructor: The size of the data in pkt1/pkt2 is checked against 6 - that test will fail if ever the static initialization of "data" is changed. Perhaps check against "sizeof(data)"?

RawUnpack: buf_elapsed_time is declared without an explicit size, buf_duid with an explicit size. This is an inconsistency.

RawUnpack: creation of OptionBuffers from static data - using "sizeof(<buffer>)" instead of explicitly putting the buffer size in the constructor guards against a future change of the buffer sizes.

RawUnpack: "magic numbers" 4 and 86 would be better declared as "const int" symbols.

PackTransactionId/UnpackTransactionId: The initialization of data[100] will only initialize the first element, as there is only one constant. (True the rest of the array may well be initialized to zeroes, but that is down to implementation of the compiler.) Safer would be to use memset() or std::fill().

comment:8 Changed 8 years ago by marcin

  • Add Hours to Ticket changed from 37.5 to 2
  • Owner changed from marcin to stephen
  • Total Hours changed from 37.5 to 39.5

I implemented changes suggested in this review and committed to trac1955 as this branch is now holding overall changes for #1955 and #1956.

I removed all invocations to bufferOut_.clear() and instead I used writeUintXAt() functions that allowed me to specify location where I wanted to write. I couldn't do the other way around because when I read options from the collection there is no guarantee that their offsets are in incremental order.

Since we are operating on raw buffers and we don't parse whole the packet content we keep light restrictions on transaction id offset. It should be positive value and it must not be higher than packet size - 3 for DHCPv6 and packet size - 4 for DHCPv4.

In a function where there are multiple integer parameters inline forcing explicit object types is a good method to avoid user errors like putting things in wrong order. However in this particular case I tend to agree that this approach would produce inconsistency with other functions and parameters so I got rid of this.

Please review branch trac1955.

comment:9 Changed 8 years ago by stephen

  • Owner changed from stephen to marcin

Ready to merge - see ticket #1955.

comment:10 Changed 8 years ago by marcin

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

Merged to master 6f914bb2c388eb4dd3e5c55297f8988ab9529b3f.

Note: See TracTickets for help on using tickets.