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:
                Type:  defect        |  marcin
            Priority:  very high     |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131016
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:
                                     |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 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?

 !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 ...)

 '''option6_iaaddr.cc'''

 '''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.

 '''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.

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

 '''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.

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

 '''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.

 '''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?


 '''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.

 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.

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

 ------

 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.

 Changelog proposal looks good.

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