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
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:4 stephen]:
 > Reviewed commit df3ee4b9e04459e2ba6dbc50702b3c7f29c0ceb7
 >
 > '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 > Subnet6ConfigParser::createSubnet (line 1484): at this point the
 exception declaration is "const !DhcpConfigError&", but a few lines later
 the same exception is caught via "!DhcpConfigError".  The catch clauses
 should be consistent.
 Done.
 >
 > Subnet6ConfigParser::createSubnet (line 1502): in the isc_throw
 statement when both the interface ID and interface are not empty, convert
 "len" to an int via the "static_cast<int>" construct.
 Done.

 > Subnet6ConfigParser::createSubnet (line 1532): as the previous check has
 extablished that either iface is not empty or ifaceid is not empty, but
 both cannot contain something at the same time, this "if" could be an
 "else if".  The effect is the same, but use of "else if" serves to
 document to the reader that it is "either-or".  In fact, going beyond
 that, can't these two checks be combined into a single "if-else if-else
 if" block with the check that leads to the isc_throw statement:
 > {{{
 > if (!iface.empty() && !ifaceid.empty()) {
 >    :
 > } else if (!iface.empty()) {
 >    :
 > } else if (!ifaceid.empty()) {
 >    }
 > }
 > }}}
 I left the code as is for simplicity reasons. Both interface and
 interface-id are optional. Allowed combinations are: none, interface
 defined, interface-id defined. Illegal combination: both interface and
 interface-id defined.

 We check if illegal conditions occur and throw exception. Once we check
 that this is not the case, then and only then we create subnet6 object. I
 don't want to create subnet object first and then throw it away if we
 detect later that invalid conditions occurred.

 Sure, we could continue the line:
 {{{
 if (!iface.empty() && !ifaceid.empty()) {
   isc_throw(...)
 }
 }}}
 with else clause and put the rest of the method in this clause, but it
 would give us little and only increase indentation, which would make the
 code more difficult to read.

 x>
 > (Incidentally, before merging this, consult with Thomas - he has changed
 this area of the code.)
 >
 >
 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > dhcp6Srv::copyDefaultOptions: please start comments with a capital
 letter.
 >
 > DhcpSrv::selectSubnet: the first block of the "if" can be simplified to:
 > {{{
 > Subnet6Ptr subnet = ...
 > if (!subnet) P
 >    // No subnet found, try to find it based on remote address.
 >    subnet = ...
 > }
 > return (subnet);
 > }}}
 > Similar logic can be used in the "else" clause to avoid the
 > {{{
 > if (subnet) {
 >    return (subnet);
 > }
 > }}}
 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).

 > '''src/bin/dhcp6/tests/config_parser_unittest.cc'''[[BR]]
 > class Dhcp6ParserTest: it would be helpful to comment the member
 variables: they are used in various tests and sometimes it is not clear
 what is being done.
 Done.


 > Tests subnetInterfaceId, interfaceIdGlobal,
 subnetInterfaceAndInterfaceId: several points:
 > * Please start comments with capital letters.
 > * "config" can be declared as a "const string".
 > * Is there a need to output the config to cout during the test?
 > * Suggest that declaration of "status" (or "new_config" if renamed) be
 moved to after the declaration of "config" so that it is closer to the
 point of first use.
 Done.

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

 > '''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/)
 Done.

 Changes pushed. The review will continue tomorrow.

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