Opened 8 years ago

Closed 7 years ago

#1955 closed enhancement (fixed)

Implement perfdhcp IPv4 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: 32 Add Hours to Ticket: 4
Total Hours: 30 Internal?: no

Description

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

Subtickets

Change History (8)

comment:1 Changed 8 years ago by stephen

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

comment:2 Changed 7 years ago by marcin

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

comment:3 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 0 to 20
  • Estimated Difficulty changed from 0 to 32
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 20

I added new class derived from dhcp::Pkt4 and corresponding unit tests. Since, both PerfPkt6 and PerfPkt4 have common logic, as a part of this ticket I moved the common logic to new class PktTransform?. This class exposes static functions that can be used to operate on raw DHCP message buffers - needed for implementation of template files.

The 1956 is still in review and has not been checked in to master yet. Since 1956 and 1955 share a lot of code I branched-off trac1955 from trac1956. Finally, after PktTransform? had been implemented I modified v6 packet handler to use it and removed redundant code. This change was checked in to trac1955. For this reason the trac1955 is the branch that has the latest changes for 1956 and 1955 so it makes sense to do code review on trac1955 for both tickets.

Please review.

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 53dfacddb52280654910206f96325c531a480ce0

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

All .cc/.h files in tests/tools/perfdhcp and tests/tools/perfdhcp/tests
These should be copyright 2012. Suggest these be fixed as part of this ticket.

tests/tools/perfdhcp/perf_pkt{4,6}.{h,cc}
Constructor: Looking at the code, the first constructor is redundant - the same can be achieved using the third constructor if the transid_offset argument is given a default value of 1.

Constructor: neither the first nor third constructor set the transid_ value, which implies that the value is not important and should probably be zero. (This is in fact what it is set to when the "Pkt4(buf, len)" constructor is called, although it is set to a random value in the equivalent Pkt6 constructor.) In this case, all constructors could be collapsed into a single constructor with a signature of:

PerfPkt{4|6}(const uint8_t* buf, size_t len,
             size_t transid_offset = 1, uint32_t transid = 0)

Protected Variables
I'm usually against use of "protected", but here - where the PerfPkt4 and PerfPkt6 classes need to manipulate the internals of the Pkt4 and Pkt6 classes to customise packets for the performance tests - I don't really see an easy way round it. The PerfPktN classes manipulate internal variables which have no public interface to allow them to be modified.

In part mitigation, I suggest that the public accessor methods of the PktN classes be used where possible (e.g. use getTransid() instead of accessing transid_ directly). These methods could be extended in some cases, e.g. a PktN::setTransid() method seems logical.

For the remainder, protected access is OK, but the comments in the protected section PktN should be extended to note that these variables are being accessed from classes in the perfdhcp program (and the reason for it). This is imperfect, but should serve as a warning to anyone modifying the PktN classes that changes may have an impact on the perfdhcp code.

tests/tools/perfdhcp/pkt_transform.cc
pack(): Need some comment about the check on transaction ID to state that the transaction ID must fit in the packet with some space to spare. Also, the check that

(transid_offset + transid_len + 1 > in_buffer.size())

... could be simplified to:

(transid_offset + transid_len >= in_buffer.size())

pack(): Identical comments to those in ticket #1956 about the rawPack() function in perf_pkt6.cc.

unpack(): When reading a four-byte transaction ID, it would be easier to use InputBuffer::setPosition followed by InputBuffer::readUint32. (In fact, we could also use it for reading the three-byte transaction ID in the V4 universe: as we know that the offset of the transaction ID is not zero (this was checked in the "validate transaction ID" check), it would be possible to reads a four-byte value from offset "transaction ID - 1" and mask out the high-byte. If this is done, it would require a comment as the logic is not obvious.)

packOptions(): if for some reason the option in the OptionCollection is not a LocalizedOption, the program will segfault when it receives a null pointer through the boost::dynamic_pointer_cast call. Perhaps an additional test that the pointer returned is not null?

PerfPkt4Test::capture(): The code is fine, although I woudn't have bothered with trying to optimise the initialization of "buf". This is test code after all, and the additional overhead of reinitializing "buf" on every call is negligible.

Constructor test: Even though it doesn't make much difference here, in general "++i" is preferred to "i++": it avoids use of temporaries when "i" is a complex object.

RawPack test: suggest adding a comment to say what the data in the buf_hostname and buf_file_size arrays are (i.e. option code, length, data). People modifying this code in the future may not have a deep knowledge of DHCP. (Another possibility is to use the DHO_ code instead of the option number in the array initialization.)

comment:6 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 20 to 6
  • Owner changed from marcin to stephen
  • Total Hours changed from 20 to 26

I implemented changes suggested in the review.

Protected Variables
We already had functions like getTransidOffset and getTransid, so I now use them in the code. I added setTransid public function as it indeed make sense to have something like this anyway. The other protected members I left as they were (they are still protected and no extra getters/setters are added). Instead I added comments in PktN classes that they are referenced directly by perfdhcp classes.

tests/tools/perfdhcp/pkt_transform.cc
The input buffer is a std::vector<uint8_t>, not the InputBuffer?. In this case there is no way to use setPosition() or equivalent.

Please review.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 0235da81624be30ea2aeadaf1c93fde1ba1bd057

I made one small change to sec/lib/dhcp/option.h, adding a "const" to the arguments of setData() to match the declaration. (This was commented on in #1956 and got overlooked in this change.)

All OK, please merge. I don't think this needs a ChangeLog entry - it's still perfdhcp preparatory work, and the changes to libdhcp++ are very minor.

comment:8 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 6 to 4
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 26 to 30

Merged to master 6f914bb2c388eb4dd3e5c55297f8988ab9529b3f.

Note: See TracTickets for help on using tickets.