BIND 10 #2898: DHCPv6 server must handle relayed traffic

[BIND 10 Development]

#2898: DHCPv6 server must handle relayed traffic
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  defect        |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130509
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed commit 131a89175e97714ac87f14f95703cd76df7a965c

 '''doc/guide/bind10-guide.xml'''[[BR]]
 I've pushed an update correcting a few errors in the text I supplied.

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > I did as you requested. The code would is shorter by couple lines, but
 is marginally slower now. It is on a critical path, but the performance
 penalty is minimal (just one extra comparison).
 I'm not sure where the one extra comparison comes from.  The number of
 "if" tests is unchanged for every path through the method.  Besides which,
 the optimising compiler is likely to have optimised both versions into the
 same set of machine instructions.


 '''src/bin/dhcp6/tests/config_parser_unittest.cc'''[[BR]]
 >> All tests contain a comment that says "return value should be..." after
 configureDhcp6Server is called. The comment is wrong, as the returned
 value is a pointer to a new configuration. (Incidentally, "status" seems
 the wrong name here, as a pointer to a new configuration is returned.
 Perhaps "new_config" is a better name?)
 > No. See documentation or implementation for configureDhcp6Server. It
 returns a pointer to very small element that contains rcode (integer
 value) and optionally a string that provides extra info (usually explains
 error condition is rcode!=0). Moved the status declaration closer to its
 usage, but kept its name.
 Fair enough, but the comment says:
 {{{
 Returned value should be 0 (configuration success)
 }}}
 ...but when rcode is checked, it is checked against a value of 1.
 (Besides which, the description of the test is that it is checking that an
 invalid configuration is being rejected.)

 '''src/lib/dhcp/pkt6.cc'''[[BR]]
 > Your proposal wouldn't work, because in case of RELAY_SEARCH_LAST and
 RELAY_SEARCH_FIRST there is only one relay to check, so direction is 0 and
 i is not incremented. Furthermore, the code used to assume that after the
 loop i points to the last relay to be checked, so it would break down.
 Although I didn't state it in my comment, I had assumed that direction
 would be set to be non-zero.

 > I've refactored the code a bit and it is now simpler and there are more
 comments around it. Please let me know if it is sufficient or should I
 tweak it more.
 It's fine and equivalent to my suggestion - rather than modify "end" and
 test against the modified value, you've included the modified value in the
 test itself by comparing against "end + direction".



 '''src/lib/dhcp/pkt6.h'''[[BR]]
 >> It's OK, but why move the definition of Pkt6Ptr before the definition
 of Pkt6? It only necessitates the addition of a forward declaration.
 > Please move it back and see what happens :)
 OK, I missed that one.

 >> Since methods have been added to search the relay information and to
 copy relay information, should relay_info_ still be a public field?
 > Yes. Otherwise you get into the object lifetime mess. If you implement a
 method that hides it, will you return const references?
 You still have the lifetime problem whether the field is public or
 accessible via a method.  In the former case, you may well retain a
 reference or a pointer to the field, in which case it becomes invalid as
 soon as the object is destroyed.

 > I personally think about Pkt{4,6} not as a proper class, but rather a
 structure with some extra methods specified for convenience.
 That's a valid approach, in which case it should be declared as "struct".
 (No difference in C++ terms, but I think it would imply that it is a data
 carrier).  Let's not change it now, but we should revisit this later: it
 is inconsistent to have the relay information public, whilst other data is
 private.

 '''src/lib/dhcpsrv/cfgmgr.cc'''[[BR]]
 > This seemingly simple function would be then 5 indentation levels deep
 then. Simple return statement is not that much. Can we keep it as it is?
 Yes, we can keep it as it is.


 The only changes I'm suggesting in this review is the comments in
 config_parser_unittest.cc.  After than is changed, the code can be merged
 - I don't need to see it again.

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