Opened 8 years ago

Closed 8 years ago

#1226 closed task (fixed)

V4 packet library - packet parsing

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20111109
Component: dhcp 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

Parsing an incoming packet and extracting both the fixed header and variable option information from it.

Subtickets

Change History (8)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2011 to Sprint-DHCP-20111026

comment:2 Changed 8 years ago by tomek

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

This will be worked on together with #1227.

comment:3 Changed 8 years ago by tomek

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

Code pushed in to brach trac1226. Ready for review.

comment:4 follow-up: Changed 8 years ago by stephen

  • Owner changed from UnAssigned to tomek

Reviewed commit 0fed56c3692e358184958cc1263cff67db0f62cb

src/lib/asiolink/io_address.cc
Any reason why a static IOAddress method is used to create an IOAddress rather than have a constructor take a uint32_t? (I see that from_bytes() also does that and I'm puzzled by that as well.)

In the Jabber room although I suggested the uint32_t conversion operator, I then had second thoughts: if the IOAddress were only holding a V4 address, a conversion operator would be fine. But because it can hold a V6 address and because conversions are implicit it would be possible to have an apparently correct statement generate an exception for no discernible reason. For this reason, perhaps a toUint32() method to return the address as a uint32_t would be better?

src/lib/asiolink/pkt4.h
Header of getBuffer() needs to make it clear that the returned output buffer is valid only while the Pkt4 object is valid.

The buffer returned by getBuffer() should be "const" (else it is possible to modify the internals of the Pkt4 object without going through one of the defined interfaces. Similarly, the method getBuffer() should also be const.

Not related to this change, but the various Pkt4::getXxxx() methods should be "const" as they don't change the contents of the Pkt4 object.

src/lib/dhcp/tests/pkt4_unittest.cc
Declaration of "const static" variables: the "static" is not needed as by default, "const"s (and "typedef"s) have internal linkage.

fixedFieldsPack test: Still declaring variables as "type * name" instead of "type* name" :-)

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

Replying to stephen:

Reviewed commit 0fed56c3692e358184958cc1263cff67db0f62cb

src/lib/asiolink/io_address.cc
Any reason why a static IOAddress method is used to create an IOAddress rather than have a constructor take a uint32_t? (I see that from_bytes() also does that and I'm puzzled by that as well.)

Fixed.

In the Jabber room although I suggested the uint32_t conversion operator, I then had second thoughts: if the IOAddress were only holding a V4 address, a conversion operator would be fine. But because it can hold a V6 address and because conversions are implicit it would be possible to have an apparently correct statement generate an exception for no discernible reason. For this reason, perhaps a toUint32() method to return the address as a uint32_t would be better?

Does it matter if the exception is thrown during conversion attempt or during function call? In both cases exception clearly states what went wrong.

There is also warning in the text of the operator that it will throw BadValue? exception during attempt of IPv6 conversion.

src/lib/asiolink/pkt4.h
Header of getBuffer() needs to make it clear that the returned output buffer is valid only while the Pkt4 object is valid.

Done.

The buffer returned by getBuffer() should be "const" (else it is possible to modify the internals of the Pkt4 object without going through one of the defined interfaces. Similarly, the method getBuffer() should also be const.

Returned type is now const. getBuffer() method is now consts. All other getters are const, too.

Not related to this change, but the various Pkt4::getXxxx() methods should be "const" as they don't change the contents of the Pkt4 object.

Ahh. I came to the same conclusion, before reading this. Done already. :)

With added consts, some of the lines are over 80 characters, but I believe that there was agreement on Jabber that we set the limit to 100.

src/lib/dhcp/tests/pkt4_unittest.cc
Declaration of "const static" variables: the "static" is not needed as by default, "const"s (and "typedef"s) have internal linkage.

Ok. Fixed.

fixedFieldsPack test: Still declaring variables as "type * name" instead of "type* name" :-)

And there were no spaces before and after +, / and = signs. Fixed all of them. Number of this kind of changes decreases, which is a good sign.

Fixed code ready for review.

comment:6 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

comment:7 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit b5040b229739c8c69463fe462aa8f7b4a8e47f7f

All OK, please merge.

comment:8 Changed 8 years ago by tomek

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

Code merged to master, pushed.

Note: no changelog added. There will be one summary changelog entry once Pkt4 work is complete (after #1228).

Note: See TracTickets for help on using tickets.