Opened 8 years ago

Closed 8 years ago

#1350 closed enhancement (fixed)

V4 packet library - dedicated DHCPv4 options needed

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

Support for the following DHCPv4 options is needed.

  • Message type
  • Default router
  • DNS server
  • Supplement mask
  • Parameter request list
  • Subnet mask
  • Requested IP address
  • "End" option
  • Padding option
  • Field use option
  • Unknown option

That is a follow-up of a #1228 ticket (generic DHCPv4 options support).

Subtickets

Change History (10)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from New Tasks to Sprint-DHCP-20111109

comment:2 Changed 8 years ago by tomek

We need:

  1. 1 octet (Message type, option overload ("field use option") )
  2. IPv4 address (requested IP address, subnet mask)
  3. IPv4 address list (Router Option, DNS Servers)
  4. 1-byte long, just type, not even length field (pad, end)
  5. list of octets (PRL)

Cases 1 and 5 can be handled with current Option class.
Case 4 will be coded as a property of the framework. There's not much sense in adding class for a single byte.

To cover those cases, Option4Addr and Option4AddrLst classes should be implemented. Although a single address could be represented as degraded case of AddrLst?, it is more convenient to have 2 separate classes with simpler interfaces.

comment:3 Changed 8 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Implemented Option4_AddrLst that can be used both as a list of IPv4 addresses and a convenient container for a single IPv4 address.

While just implemented Option4_AddrLst and existing classes address this requirement, one extra class for could be convenient. See ticket #1368. That is not necessary for now, so let's move it off from critical path.

Please review.

comment:4 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to tomek

Reviewed commit 36dc8dd6f15a42f401ffa32829ed7c436e529eb3.

src/lib/dhcp/option.h
Some comments not related to this change:

len(): Not related to this change, but len() should return a uint16_t instead of "unsigned short". The former is guaranteed 16 bits long, the latter has no guaranteed length (although is almost always 16 bits).

getData(): Header comment not quite correct: it does not return NULL if there is no data, it returns a reference to an empty vector.

getType()/getData(): as these are just methods just comprising "return (xxx_)", they are sufficiently trivial to put the body in the header file.

src/lib/dhcp/option4_addrlst.h
Typos in constructor headers - "contructor", "Constrcutor".

Constructor for received options (header comment): the constructor is not simplar to the previous one: it is explicitly for the case where the addresses are presented as a byte array, as in the case when parsing the address list in a received packet.

src/lib/dhcp/option4_addrlst.cc
Constructor for received options: all OK, but the use of creating an intermediate vector to get the length is not necessary. The STL function distance will do that without the overhead of allocating memory.

pack4: Using "addr++" instead of "++addr". Not really an issue here but in general, unless a post-increment is really needed, use a pre-increment: it can avoid an unnecessary object copy. (A good explanation can be found here.)

to_text(): Single-line for-loops should include braces.

to_text(): The current implementation ends up with a trailing space. Instead of that, remove the space after the ":" in the line before the loop, and append a space before "*addr" instead of after it.

to_text(): Just a style comment - here addrs_ is iterated over using a "for loop". In pack4(), it is a "while" loop.

src/lib/dhcp/pkt4.cc
Doesn't just adding one byte to the output buffer - as opposed to creating an option and using packOptions - run the risk of overflowing the packet? It would be better to put this functionality into packOptions: check the last option output processed and if not an END option, create one and append it to the buffer using pack4().

General
There are sufficient similarities in Option4AddrLst and Option4AddrLst to derive them from a common ancestor class. Assuming that the base class holds a "Universe" member to distinguish between header sizes and address families etc., addAddress, setAddress, setAddresses, len, to_text could all be put in a base class as they are more or less identical. In addition, if the type were passed as a uint16_t and the DHCPv4 code threw an exception if it were greater than 255, some of the constructors have common code.

comment:6 Changed 8 years ago by tomek

  • Component changed from dhcp to dhcp4

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

Replying to stephen:

Reviewed commit 36dc8dd6f15a42f401ffa32829ed7c436e529eb3.

Thank you.

src/lib/dhcp/option.h
Some comments not related to this change:

len(): Not related to this change, but len() should return a uint16_t instead of "unsigned short". The former is guaranteed 16 bits long, the latter has no guaranteed length (although is almost always 16 bits).

Done. In option.h and all derived classes.

getData(): Header comment not quite correct: it does not return NULL if there is no data, it returns a reference to an empty vector.

Done.

getType()/getData(): as these are just methods just comprising "return (xxx_)", they are sufficiently trivial to put the body in the header file.

Done.

src/lib/dhcp/option4_addrlst.h
Typos in constructor headers - "contructor", "Constrcutor".

Fixed.

Constructor for received options (header comment): the constructor is not simplar to the previous one: it is explicitly for the case where the addresses are presented as a byte array, as in the case when parsing the address list in a received packet.

Removed comment about similarity.

src/lib/dhcp/option4_addrlst.cc
Constructor for received options: all OK, but the use of creating an intermediate vector to get the length is not necessary. The STL function distance will do that without the overhead of allocating memory.

Nice trick. I didn't know that.

pack4: Using "addr++" instead of "++addr". Not really an issue here but in general, unless a post-increment is really needed, use a pre-increment: it can avoid an unnecessary object copy. (A good explanation can be found here.)

Yes, I understand the difference. For some cases (e.g. for loop) I use it automatically. Unfortunately, I sometimes forget about this rule in other cases.

to_text(): Single-line for-loops should include braces.

Done.

to_text(): The current implementation ends up with a trailing space. Instead of that, remove the space after the ":" in the line before the loop, and append a space before "*addr" instead of after it.

Done.

to_text(): Just a style comment - here addrs_ is iterated over using a "for loop". In pack4(), it is a "while" loop.

I actually prefer variety. Using different techniques is an opportunity to gain (and maintain) experience. Using the same type of loops does not improve readability as everyone who is looking into the code is expected to be familiar with all loop concepts. It would be possible to say "we should use X loop type only and deprecate all others", but that would be unwise.

src/lib/dhcp/pkt4.cc
Doesn't just adding one byte to the output buffer - as opposed to creating an option and using packOptions - run the risk of overflowing the packet?

No. We are using OutputBuffer? that is made of rubber. It will expand. :)
Seriously speaking, we call writeUint8 that calls ensureAllocated(). This method in turn checks if there is enough space available. In case of running out of space, extra memory will be allocated.

It would be better to put this functionality into packOptions: check the last option output processed and if not an END option, create one and append it to the buffer using pack4().

No, for couple of reasons. In some cases we want END option to be added (assembling v4 packet options field) and in some case we don't (assembling sub-options in v4, assembling v6 options). We could add a parameter to packOptions() to explicitly command that, but this would add extra complexity and sanity checks (e.g. what if someone wants to add END option after v6 options?). In fact, the only case when we want END option to be added is when assembling v4 packet. That's why we are adding the byte there.

General
There are sufficient similarities in Option4AddrLst and Option4AddrLst to derive them from a common ancestor class. Assuming that the base class holds a "Universe" member to distinguish between header sizes and address families etc., addAddress, setAddress, setAddresses, len, to_text could all be put in a base class as they are more or less identical. In addition, if the type were passed as a uint16_t and the DHCPv4 code threw an exception if it were greater than 255, some of the constructors have common code.

I assume you meant Option6_AddrLst and Option4_AddrLst.

Agree. Unfortunately, v4 options use Input/OutputBuffer?, while v6 option use shared_array<uint8_t>. This unification should be done once #1312 is complete. Added a comment to #1312 about merging AddrLst? classes.

Do you think this change should be mentioned in ChangeLog?? Currently the only text is entry 304 (that was merged already as part of 1228 ticket).

Please review.

comment:8 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

comment:9 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 537af1705fc5c1695b4b601571f65ead81dc1289

src/lib/dhcp/tests/option_unittest.cc
Test fails at line 90. (I notice this was corrected in the code for #1238.) Correct that and it can be merged - another review is not needed.

Regarding the ChangeLog, yes that would be a good idea. I suggest you get the ChangeLog entry approved in the BIND 10 Jabber room to avoid delaying things.

comment:10 Changed 8 years ago by tomek

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

Code merged to master. Closing ticket.

Note: See TracTickets for help on using tickets.