BIND 10 #2827: v6 relays support in Kea

[BIND 10 Development]

#2827: v6 relays support in Kea
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130411
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:3 marcin]:
 > Reviewed commit 859e0b3e82d4cc5270d8fb557f0120dc35edd1a3
 >
 > Please update the copyright dates in modified files.
 Updated.

 > '''!ChangeLog'''
 > Changes are limited to !''libdhcp++!'' so maybe reflect that in the
 changelog by replacing !''b10-dhcp6!'' with !''libdhcp++''.
 Done as suggested.

 > '''pkt6.cc'''
 > Please update the copyright date.
 Done.

 > Pkt6::len(): wouldn't it be better to call calculateRelaySizes() on
 packet reception - when unpackUDP() is called? The ''Pkt6::unpackUDP()''
 is called once for the specific packet while ''Pkt6::len()'' can be called
 several times from different parts of the code. This will cause re-
 calculation.
 No. This would return invalid values after each addOption() or delOption()
 call, so its validity would base on calling calculateRelaySizes() after
 each option manipulation. Our code should call Pkt6::len() only once or
 twice, so it's not that big deal. Also, the "relays" are typically just
 one relay, so there cost of calling calculateRelaySizes() is not that big.
 And for really complex multi-relay networks... well, there's a price to
 pay for complex design.

 > Pkt6::calculateRelaySizes(): Proposing alternative to the loop in this
 function. However, this is really minor matter so you don't need to pay
 attention to this proposal. :-)
 > {{{
 > for (int relay_index = relay_info_.size(); relay_index > 0;
 --relay_index) {
 >     relay_info_[relay_index - 1].relay_msg_len_ = len;
 >     len += getRelayOverhead(relay_info_[relay_index - 1]);
 > }
 > }}}
 Done as requested.

 > Pkt6::packUDP():
 > Having some more comments would be useful in the part of the function
 that packs relay headers and options.
 Added.

 > '''pkt6.h'''
 > getRelayOption(): this function is not documented.
 It is now.

 > directLen(): IMO, it could be better to say !''if received message was
 not relayed!'' because !''directly!'' may be ambiguous.
 > Also, this function can be declared const.
 Comment clarified, method is now const.

 > getRelayOverhead(): the !''relay!'' parameter is not documented.
 > This function could be declared const.
 Parameter documented, function is now const.


 > '''pkt6_unittest.cc'''
 > It is probably not a good idea to...
 > {{{
 > using namespace boost;
 > }}}
 >
 > On Solaris this will shade the uint8_t type which will result in
 compilation error.
 Aahhh, I forgot about that. using namespace boost removed.

 > capture2(): typo in the comment. s/related/relayed.
 Fixed.

 Thanks for the review.

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