Opened 8 years ago

Closed 8 years ago

#1228 closed task (complete)

V4 packet library - option processing

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 (last modified by tomek)

This ticket is for supporting DHCPv4 options in general - option themselves, support for options in Pkt4 and LibDHCP. Separate ticket for specific options was added (#1350).
For the present, it needs to be able to handle:

  • 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

After this ticket is complete, all of the above options may be handled by generic Option class. However, to reasonably use them, a specialized classes are needed. Thus ticket #1350.

Subtickets

Change History (13)

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 accepted

comment:3 Changed 8 years ago by tomek

Implemented generic support for DHCPv4 options (in libdhcp, options and Pkt4). Due to extensive changes, work on dedicated options was defined as a separate ticket (#1350).

Note: DHCPv6 code uses share_array<uint8_t>. DHCPv4 code uses InputBuffer? and OutputBuffer?. This will be done during work on #1312. Currently some of the methods use strange conversions. That is a compromise and will be fixed in #1312. The alternative is to spend several days on refactoring DHCPv6 code - a luxury we can't afford at this time.

Code is ready for review.

comment:4 Changed 8 years ago by tomek

  • Description modified (diff)

comment:5 Changed 8 years ago by tomek

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

comment:6 follow-ups: Changed 8 years ago by stephen

  • Owner changed from UnAssigned to tomek

Reviewed commit bfab5a33ceabe3f0d31bd465d13308c8b84adf68

src/lib/dhcp/libdhcp.h
unpackOptions4 needs its own header.

Should unpackOptions4() be taking an InputBuffer object?

packOptions - should this be called packOptions4?

src/lib/dhcp/libdhcp.cc
Regarding the "TODO" in unpackOptions4: Looking at this and the definition of Option, perhaps the easiest thing would be to alter the Option constructor to something like:

Option::Option(Universe u, uint16_t type, InputIterator first,
               InputIterator last)

... and pass &buf[offet] and &buf[offset + opt_len] as the "first" and "last" arguments.

src/lib/dhcp/option.h
See above comments regarding Option constructor.

src/lib/dhcp/option.cc
Spaces around binary operators.

pack6(): If checking an STL container for being empty, it's best to use the empty() method. For some STL containers the time taken to perform a size() depends on the number of elements in the container. Having said that, for a vector, the time to perform an empty() should be about the same as that for a size(). But I think "if (! data_.empty())" is clearer than "if (data_.size())".

src/lib/dhcp/tests/libdhcp_unittest.cc
packOptions4 test: there is no need to resize a vector before adding elements to the end of it. The use of push_back() to append an element to the end of a vector is less error-prone.

src/lib/util/buffer.h
readData(): The problem here is that if the vector is too long, you end up with a vector of which the first "len" bytes hold the data and the remainder are junk. Also, when filling the data, an intermediate vector is created then copied by the assignment operator. I would suggest the is better:

data.resize(len);		// Ensure vector is correct length
readData(&data[0], len);	// Copy data into it using overloaded method

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

Replying to stephen:

Reviewed commit bfab5a33ceabe3f0d31bd465d13308c8b84adf68

src/lib/dhcp/libdhcp.h
unpackOptions4 needs its own header.

Why? This is part of the library. Traditionally, you provide a single header that exposes all methods offered by a library. User is not expected to use all methods, just the ones that are suitable for specific use.

Should unpackOptions4() be taking an InputBuffer object?

No. Due to several reasons. Honestly, DHCPv4 packet format is a mess. There is options field that contain options. We want to parse that. However, if certain option is specified (I don't recall exact name of that option), also SNAME and/or FILE fields should be parsed as options as well. There are some options (like option 82) that contains sub-options. In such case, we could use unpackOptions() in implementation of that specific option classes to parse only a range of buffer, not the whole buffer. So this will need to parse only fragments of InputBuffer?, never the whole buffer. There may be cases, when we get content for such option from somewhere else (e.g. custom option specified by user). In that case we don't really have input buffer at all.

packOptions - should this be called packOptions4?

In theory - yes, in practice - no. Once v6 code is refactored to use Input/OutputBuffer?, there will be only packOptions() and unpackOptions(). Unpack will have extra parameter that designates universe (v4 or v6). Consider current name (packOptions()) as a first step of having those names unified.

Review will continue on Monday...

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

Replying to stephen:

src/lib/dhcp/libdhcp.cc
Regarding the "TODO" in unpackOptions4: Looking at this and the definition of Option, perhaps the easiest thing would be to alter the Option constructor to something like:

Option::Option(Universe u, uint16_t type, InputIterator first,
               InputIterator last)

... and pass &buf[offet] and &buf[offset + opt_len] as the "first" and "last" arguments.

src/lib/dhcp/option.h
See above comments regarding Option constructor.

Added constructor:

Option(Universe u, uint16_t type,

std::vector<uint8_t>::const_iterator first,
std::vector<uint8_t>::const_iterator last);

It is ok for now, but it should be migrated to templated version eventually. Added TODO text in comments that suggest that. There are currently 2 constructors that take different parameters. One takes the whole vector (easier to use), while the second takes two iterators (more flexible). I think both are useful and should stay.

src/lib/dhcp/option.cc
Spaces around binary operators.

Done. I hope I picked them all.

pack6(): If checking an STL container for being empty, it's best to use the empty() method. For some STL containers the time taken to perform a size() depends on the number of elements in the container. Having said that, for a vector, the time to perform an empty() should be about the same as that for a size(). But I think "if (! data_.empty())" is clearer than "if (data_.size())".

Done.

src/lib/dhcp/tests/libdhcp_unittest.cc
packOptions4 test: there is no need to resize a vector before adding elements to the end of it. The use of push_back() to append an element to the end of a vector is less error-prone.

Done that in options test. However, in HWAddr test, I really want vector length to be kept fixed at field length.

src/lib/util/buffer.h
readData(): The problem here is that if the vector is too long, you end up with a vector of which the first "len" bytes hold the data and the remainder are junk. Also, when filling the data, an intermediate vector is created then copied by the assignment operator. I would suggest the is better:

data.resize(len);		// Ensure vector is correct length
readData(&data[0], len);	// Copy data into it using overloaded method

Clever. Done. Fix in buffer.h went as a separate checkin in case someone wants cherry-pick this on his branch.

Note: Also added ChangeLog? entry.

Please review.

comment:9 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

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

  • Owner changed from stephen to tomek

src/lib/asiolink/tests/io_address_unittest.cc
Just noticed - the "uint32" test should also check to see that an exception is raised if an attempt is made to convert a V6 address to a uint32_t.

src/lib/dhcp/libdhcp.h

unpackOptions4 needs its own header.

Why? This is part of the library. Traditionally, you provide a single header that exposes all methods offered by a library. User is not expected to use all methods, just the ones that are suitable for specific use.

Mainly for the Doxygen documentation. Without an individual header, the entry for the method in the LibDHCP Doxygen page contains just the bare function signature.

Also, it would be useful to have a summary of what the LibDHCP class does (again for the Doxygen page).

src/lib/dhcp/option.{cc,h}
Changes OK. However...

In the constructor taking the first/last arguments and the one taking an offset and length, data_ can be initialized on the declaration line. Currently it is initialized in the body of the constructor via copy constructor.

Also, the checking of the arguments is the same in all constructors. This could be extracted into a private method and each constructor call that method.

When checking the arguments, the test "u == V4" is carried out twice. Would be clearer to have:

if (universe_ == V4) {
    if (data_.size() > 255) {
        ...
    } else if (type_ > 255) {
        ...
    }
}

src/lib/dhcp/option.cc
Spaces around binary operators.

Done. I hope I picked them all.

There is an extraneous space in the declaration of "uint8_t * ptr" in pack6() :-)

src/lib/dhcp/tests/option_unittest.cc
This now fails to compile. The problem seems to be line 90:

EXPECT_EQ(optData, data);

This fails because in case of inequality, the macro attempts to output the first argument using operator<< and this is not defined for a vector. Suggested is:

EXPECT_TRUE(optData == data);

src/lib/util/buffer.h

Clever. Done. Fix in buffer.h went as a separate checkin in case someone wants cherry-pick this on his branch.

There was no need to leave the test in that method, as it will be carried out in the call to the other readData() method. (Although the test does guarantee that the input vector is not changed if an exception is thrown.)

comment:11 in reply to: ↑ 10 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

src/lib/asiolink/tests/io_address_unittest.cc
Just noticed - the "uint32" test should also check to see that an exception is raised if an attempt is made to convert a V6 address to a uint32_t.

src/lib/dhcp/libdhcp.h

unpackOptions4 needs its own header.

Why? This is part of the library. Traditionally, you provide a single header that exposes all methods offered by a library. User is not expected to use all methods, just the ones that are suitable for specific use.

Mainly for the Doxygen documentation. Without an individual header, the entry for the method in the LibDHCP Doxygen page contains just the bare function signature.

Also, it would be useful to have a summary of what the LibDHCP class does (again for the Doxygen page).

I see your point. Providing documentation for libdhcp is a task on its own. It's not a matter of explaining what each method does (that is complete), but rather to show how to use them together. Created ticket #1367 for that. In its description I included a link to Doxygen documentation for Dibbler.

src/lib/dhcp/option.{cc,h}
Changes OK. However...

In the constructor taking the first/last arguments and the one taking an offset and length, data_ can be initialized on the declaration line. Currently it is initialized in the body of the constructor via copy constructor.

Ok, fixed that. I don't like it much as we copy the data even when invalid type is defined. On the other hand, this means that code is broken anyway, so one extra vector copy is not a big deal.

Also, the checking of the arguments is the same in all constructors. This could be extracted into a private method and each constructor call that method.

Ok, done. Now it makes sense, when class variables are set.

When checking the arguments, the test "u == V4" is carried out twice. Would be clearer to have:

if (universe_ == V4) {
    if (data_.size() > 255) {
        ...
    } else if (type_ > 255) {
        ...
    }
}

Done. see void check() method.

src/lib/dhcp/option.cc
Spaces around binary operators.

Done. I hope I picked them all.

There is an extraneous space in the declaration of "uint8_t * ptr" in pack6() :-)

BTW do we want space after unary operator '!' Should it be

if (! data_.empty())

or

if (!data_.empty())

src/lib/dhcp/tests/option_unittest.cc
This now fails to compile. The problem seems to be line 90:

EXPECT_EQ(optData, data);

This fails because in case of inequality, the macro attempts to output the first argument using operator<< and this is not defined for a vector. Suggested is:

EXPECT_TRUE(optData == data);

Fixed. I'm wondering why this does compile for me. I really need to switch to a different version of gcc/g++. I'm currently using g++ 4.5.2 (the default in Ubuntu 11.04). It seems very liberal in what it accepts. This caused problems more than once...

src/lib/util/buffer.h

Clever. Done. Fix in buffer.h went as a separate checkin in case someone wants cherry-pick this on his branch.

There was no need to leave the test in that method, as it will be carried out in the call to the other readData() method. (Although the test does guarantee that the input vector is not changed if an exception is thrown.)

Hmmm. Maybe better approach would be to modify this method a bit, so len would mean "give me at most X bytes". If there are less bytes available, just send that.

In the current form, I prefer to leave the test as it is. Otherwise we don't have such guarantee you mentioned. This operation is sufficiently trivial to consider is atomic - completely succeeds or completely fails. The only thing we would have gained in removing this if statement is just a couple of bytes of compiled code.

Changes pushed. commit-id 5d6c71aeb2575883488b2cde87501aa84260b1ab

Please review.

Last edited 8 years ago by tomek (previous) (diff)

comment:12 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

BTW do we want space after unary operator '!' Should it be

   if (! data_.empty())

or

  if (!data_.empty())

I think the latter for unary operators. It's not in the BIND 10 Coding Guidelines but the issue of spaces between operators is covered in the BIND 9 Coding Guidelines.

(EXPECT_EQ v EXPECT_TRUE)
Fixed. I'm wondering why this does compile for me. I really need to switch to a different version of gcc/g++. I'm currently using g++ 4.5.2 (the default in Ubuntu 11.04). It seems very liberal in what it accepts. This caused problems more than once...

My guess is that the library that comes with g++ 4.5 has an operator<<() defined for outputting a vector.

All OK, please merge.

comment:13 Changed 8 years ago by tomek

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

Added compilation fix for Mac and NetBSD, checked on ubuntu 11.04 (my box), Mac OS X (macmini in buildbot) and NetBSD (xen-netbsd-5-amd64 in buildbot). After fixing one issue in vector testing (see commit 31d5a4f66b18cca838ca1182b9f13034066427a7), code compiles and tests pass.

Resolving.

Note: See TracTickets for help on using tickets.