BIND 10 #2898: DHCPv6 server must handle relayed traffic
BIND 10 Development
do-not-reply at isc.org
Wed May 8 09:31:34 UTC 2013
#2898: DHCPv6 server must handle relayed traffic
-------------------------------------+-------------------------------------
Reporter: tomek | Owner:
Type: defect | stephen
Priority: medium | Status:
Component: dhcp6 | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-DHCP-20130509
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 => stephen
Comment:
Replying to [comment:4 stephen]:
> Reviewed commit df3ee4b9e04459e2ba6dbc50702b3c7f29c0ceb7
> '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
> Test selectSubnetRelayLinkaddr: please start comments with a capital
letter (s/source of the packet should have/Souce of the packet should
have/)
>
> Test selectSubnetRelayInterfaceId: when adding to the relay options (in
the code for case 1), use of std::make_pair() is generally clearer (and
easier to type) than explicitly creating a "pair" object.
Done.
>Also, how often is this insert (where both the option and the type of
option have to be specified) is used in the code? Rather than expose the
options_ in the relay object, would it not be better to provide a method
takes one argument (the option) and performs the insert?
Because right now OptionCollection is a std::multimap, a basic STL type.
This data will soon be exported via hook to various languages. I have a
hope that there are C++ to Python converters for standard STL types
already defined and I hope to use them.
What you are proposing would give us really marginal benefic, but would
bring in quite a lot of pain later. I very much prefer for the code to
stay as it is.
> '''src/lib/dhcp/pkt6.cc'''[[BR]]
> Pkt6::getAnyRelayOption: please start comments with capital letters.
>
> Pkt6::getAnyRelayOption: suggest putting the check whether relay_info_
is empty before the declaration of start/end/direction: this moves those
declarations close to where they are first used.
>
> Pkt6::getAnyRelayOption: in the loop, rather than have the additional
check for i == end in the loop, use the STL iterator trick: define the end
to be one element before/after the action end. For example, int the
RELAY_SEARCH_FROM_CLIENT code, set end is set to -1. The "for" loop is
thus modified to:
> {{{
> for (int i = start; i != end; i += direction) {
> }}}
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.
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.
> Pkt6:copyRelayInfo: please start comments with capital letters.
Done.
> Pkt6:copyRelayInfo: would prefer that something like "info" instead of
"x" is used for the temporary variable.
Done.
> Pkt6:copyRelayInfo: what is "relay-repl info"?
The comment was confusing. Hopefully it is clearer now.
> Pkt6:copyRelayInfo: s/Is there interface-id option in this nesting level
if there is,/Is there interface-id option in this nesting level? If there
is,/
Fixed.
> '''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 :)
(hint: see parameter type passed to copyRelayInfo())
> The description of RELAY_SEARCH_{FIRST,LAST} is not completely clear.
Does they mean that there is no search through the relays, that only the
first or last relay is checked for the option?
It is difficult to convey the search patterns in a short name. I've
renamed it to RELAY_GET_{FIRST,LAST}. Also updated the decriptions a bit.
Hopefully it is more understandable now.
> Suggest slight rephrasing of the getAnyRelay method description:
> {{{
> /// @brief Return first instance of a specified option
> ///
> /// When a client's packet traverses multiple relays, each passing relay
may
> /// insert extra options. This method allows the specific instance of a
given
> /// option to be obtained (e.g. closest to the client, closest to the
server,
> /// etc.) See @ref RelaySearchOrder for a detailed description.
> }}}
>
> 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'll get into
the problem that returned reference could be potentially used past the
Pkt6 lifetime. Will you return a copy? That's inefficient and even useless
when you want to modify something in the packet. It is much simpler to
just keep it public. It will also make our life easier when implementing
hooks.
I personally think about Pkt{4,6} not as a proper class, but rather a
structure with some extra methods specified for convenience. The major
argument for making members non-public is to have control over changes, so
we maintain internal data consistency. However, internal consistency is
our explicit non-goal: we want hooks to be able to modify anything, be it
reasonable change or not.
> '''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
> getAnyRelayOption: the tests should be extended to check that searching
for an option that does not exist (using all four search options) returns
an empty pointer.
Done. Also, expanded checks to see that the getAnyRelayOption will not
return user options.
> '''src/lib/dhcpsrv/cfgmgr.cc'''[[BR]]
> CfgMgr::getSubnet6: inverting the logic of the initial test could remove
one return statement:
> {{{
> if (iface_id_option) {
> <for loop goes here>
> }
>
> // Either the option wasn't given or we couldn't find a
> // subnet with a matching ID
>
> return (Subnet6Ptr());
> }}}
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?
>
> CfgMgr::getSubnet6: the comment before the for loop is incorrect - if
there are one or more subnets, we need to choose the proper one. And if
more than one have a matching interface ID, only the first is returned.
Copy-paste error. I have updated the comment.
> '''src/lib/dhcpsrv/dhcpsrv_messages.mes'''[[BR]]
> DHCPSRV_CFGMGR_SUBNET6_IFACE_ID: s/in subnet6 definition/in the subnet6
definition/
>
> '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''[[BR]]
> Line 51: s/paramaters/parameters/ (This was the line where a trailing
space was removed, which is why I noticed it.)
Fixed.
> Test subnet6Interface: with one subnet, should also check that looking
for an incorrect interface ID will return an empty subnet pointer. (The
check is made when multiple subnets are added. It could be that a single
subnet is a special case and matches anything. IIRC there is somewhere
else in the DHCP code that has similar logic.)
Added. I think we'll need to revisit that special case logic once we have
better per interface control implemented (right now we open up sockets on
every interface present in the system).
> '''doc/guide/bind10-guide.xml'''[[BR]]
> I have a number of corrections and suggestions to the new documentation.
The suggested text is below:
> {{{
> ...
> }}}
Text updated.
Also updated code in several places to use make_pair rather than longer
pair<a_type,b_type>(a,b) definition.
The ticket is back with you.
--
Ticket URL: <http://bind10.isc.org/ticket/2898#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list