BIND 10 #3180: Kea6 is not able to handle relayed traffic from DOCSIS3.0

[BIND 10 Development]

#3180: Kea6 is not able to handle relayed traffic from DOCSIS3.0
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  defect        |                       Status:
            Priority:  very high     |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:
         Total Hours:  0             |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Replying to [comment:5 tomek]:
 > Reviewing changes on ff3ddd8332fbaab5ff76f140ae9f185a2d2d6e34.
 >
 > '''option.h'''
 > !OptionCollection - why is it for DHCPv6 options only ? It is perfectly
 > capable to store v4 options. If it's intended for v6 only, there should
 > be a comment that explains that.
 >
 > If !OptionCollection is really v6 only, perhaps it should be
 > renamed for Option6Collection?

 Actually I copied over the typedef along with its description from the
 interiors of the Option class to outside of the class. So, the comment had
 been there for a while. I now fixed it.

 >
 > !UpackOptionsCallback - why the parameters are not named? Those should
 > be named and documented (or if naming the parameters create warnings,
 > at least they should be documented by number, e.g. parameter 1
 > specifes option buffer to be parsed, parameter 2 specifies ...)

 Good point. Fixed.

 >
 > '''option6_iaaddr.cc'''

 Should I be happy that you haven't found an issue here or you forgot to
 put your comments with respect to this file? :)

 >
 > '''option_int.h'''
 > It is fine to keep the option constructors as they are for now,
 > but I think the constructors should be extended to cover
 > other option spaces one day, e.g. vendor option. I think that
 > adding a @todo about it now would be sufficient, but I won't
 > object if you have other plans for handling interger suboptions
 > within vendor-option.

 True. I didn't want to mess up in constructors and just provided the
 modifier function: !''SetEncapsulatedSpace!'' if someone wants to set the
 space name after creation of the object. I added a @todo for this if we
 decide that we want to add this parameter to the constructor.

 >
 > '''pkt4.cc'''
 > The comment in line 214 is wrong and useless. This is not the
 > first use of readVector(). It would be better to explain why
 > the readVector has to be used. For the callback_ call in line
 > 219 it would be useful to explain why the the last 2 arguments
 > passed are zeros. And shouldn't they be NULLs, not zeros? Those
 > arguments are pointers.

 This comment had been there for a while so it is not really related to
 this ticket. I changed the comment.

 >
 > '''pkt6.cc'''
 > The same comment as for pkt4. The zeros passed to first use
 > of callback_() should be NULLs and explained what they mean.

 Right. They can be NULL but that maps to zeros anyway? I changed to NULLs.

 >
 > '''option_unittest.cc'''
 > !OptionTest.unpackCallback(): Comments for opt_data are wrong.
 > length is 2 for the first option, length for the second
 > option is not specified. The content of second option should
 > be different than the first one.

 Fixed.

 >
 > Parameters passed to boost::bind should be commented on. It is
 > unclear what _1 to _5 mean.

 Added a comment.

 >
 > '''pkt4_unittest.cc'''
 > Generic comment: why is !CustomCallback defined in option_unittest.cc
 > and in pkt4_unittest.cc? I would expect it to be either in
 > pkt4_ and pkt6_ or both defined in option_unittest.cc which is
 > common for v4 and v6. There's no need to change anything, I'm just
 > curious.

 It is possible to set custom callback for either a PktX or Option. Note
 that each of these objects has to unpackOptions (they just unpack options
 on different encapsulation level). Since each of these classes come with
 different unit tests, I have to replicate the test class
 !''CustomCallback!''. I could actually share it by storing it in the
 header file which would be included by all files. But, I thought this
 class is small enough to be copied into each unit test file so as whole
 unit tests code is in a single file.

 >
 > '''pkt6_unittest.cc'''
 > packAndClone(): why do you create one packet and then create
 > second one based on the first one? Wouldn't it be easier to just
 > create a packet and return it?

 I have to admit I didn't think much about it when I created the new
 function. I just copied common parts of the existing tests to the new
 function. On reflection I can see that it had been like this because it
 simulates the reception of the packet when the constructor taking a
 pointer to data and data length is used. This constructor copies the
 provided buffer to data_ field (See pkt6.cc, line 48). In such case, when
 I later call unpack the, the data are taken from this buffer and parsed.
 If I use a non-cloned packet it is slightly different. I create a packet
 and add options to it. Options are held as a collection of objects. I call
 pack() and options are stored in output buffer (not data_ field), ready to
 be sent. When I call unpack() the data_ is still blank (because actual
 data is in output buffer. This results in failure to unpack() because of
 the !''truncation!''. I am not sure that the behaviour of the Pkt6 class
 is correct here but it was not my aim to fix this here.

 >
 >
 > '''dhcp4_srv.cc'''
 > Dhcpv4Srv::unpackOptions() why the check is for dhcp6? This is v4
 > code. If that is correct, an explanation comment is required. This
 > looks like a bug.

 Yes it is.

 >
 > On a related note, this is not picked up by unit-tests. We don't want
 > to spend any more time on this, so please just add a todo that more
 > thorough tests for unpackOptions() that also cover dhcp4 option space
 > are needed.

 Right. The test covers option spaces other than standard option space and
 it should have been implemented. I added a todo comment.

 >
 > Dhcv4Srv::unpackOptions() - parameters are not indented correctly
 > (one space too far)
 >

 Fixed.

 > ------
 >
 > The code does not compile:
 > {{{
 > make[5]: Wejście do katalogu
 `/home/thomson/devel/bind10-clean/tests/tools/perfdhcp'
 >   CXX    perfdhcp-perf_pkt6.o
 >   CXX    perfdhcp-perf_pkt4.o
 >   CXX    perfdhcp-pkt_transform.o
 >   CXX    perfdhcp-test_control.o
 > In file included from pkt_transform.cc:22:0:
 > pkt_transform.h:69:28: error: ‘OptionCollection’ in ‘class
 isc::dhcp::Option’ does not name a type
 > pkt_transform.h:69:60: error: ISO C++ forbids declaration of ‘options’
 with no type [-fpermissive]
 > pkt_transform.h:91:30: error: ‘OptionCollection’ in ‘class
 isc::dhcp::Option’ does not name a type
 > pkt_transform.h:91:62: error: ISO C++ forbids declaration of ‘options’
 > with no type [-fpermissive]
 > }}}
 > Many more errors follow.

 What a shame! It seems that I didn't do full compilation, just bits that I
 modified. SOrry about it. Now it should compile just fine.

 >
 > Changelog proposal looks good.
 >

 Ok. Thanks for the review! Please review again.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3180#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development