BIND 10 #2414: Use AllocationEngine to actually handle v6 clients' messages

BIND 10 Development do-not-reply at isc.org
Fri Nov 2 12:55:29 UTC 2012


#2414: Use AllocationEngine to actually handle v6 clients' messages
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121101
                  Component:  dhcp6  |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 DHCP_DB_BACKEND_STARTED: Second sentence would probably be better
 expressed as "It indicates what database backend is being used to store
 lease and other information."

 DHCP_LEASE_ALLOC(_FAIL): is it possible to distinguish between the
 advertised or allocated message - either by a different code or by means
 of another field in the message?

 DHCP6_SUBNET*: the message should start with a lower-case character.

 DHCP6_SUBNET_SELECTION_FAILED: suggest:
 "This warning message is output when a packet was received from a subnet
 for which the DHCPv6 server has not been configured.  The cause is most
 likely due to a misconfiguration of the server.  The packet has been
 ignored."
 (''Note - Is the last sentence true?'')

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 Dhcp6Srv constructor: Not clear about the comment "used for testing
 purposes": that looks as if it should apply to the check of
 "countIfaces()", not the check of "port".

 Dhcp6Srv constructor: "used for testing..." should be "Used for testing".

 "if (port) {".  As port is numeric, would prefer "if (port > 0) {".

 The comments should make it clear that the test usage is with the port
 equal to zero.

 The "todo" indicates that the lease manager should be implemented at the
 point of the "todo".  But the code instantiates the memfile lease manage
 later on - where is the correct position?

 run(): what is the timeout measured in?

 setServerID(): comment: the logical place to store this information is the
 database.

 setServerID(): comments should start with a capital letter.

 setServerID(): "&srvid[0]+8" should be "&srvid[0] + 8".

 setServerID(): call to fillrandom() should have a space after the comma.

 copyDefaultOptions: use the "!OptionPtr" typedef instead of the explicit
 type of boost::shared_ptr<Option>. (Incidentally, both addOption() methods
 expect the argument to be passed by value, not reference: this will incur
 the overhead of incrementing and decrementing the reference count of the
 pointed-to object.)

 addRequestedOptions: use the "!OptionPtr" typedef instead of the explicit
 type of boost::shared_ptr<Option>.

 handleIA_NA: need more comments to explain what is happening.

 assignLeases: need more comments to explain what is happening.

 processXXX: a copy why "copyDefaultOptions" is immediately followed by
 "appendDefaultOptions" (which from their names, appear to add two sets of
 default options to the created packet) would be helpful.

 '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 Trivial point: start all "@brief" descriptions with a capital letter.

 selectSubnet: need "@param" describing the "question" argument.

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 Would appreciate some documentation about the methods of the Dhcp6SrvTest
 class (i.e. they should include a header, although perhaps not a Doxygen
 one.)

 A number of the tests have identical function headers.

 Solicit tests: is it worth checking that a sequence of solicit messages
 result in advertise responses with different addresses?

 '''src/lib/dhcp/Makefile.am'''[[BR]]
 OK.  The memfile backend is probably better put in the main dhcp directory
 (with appropriate "TODO"s about not being finished.)

 '''src/lib/dhcp/addr_utilities.cc'''[[BR]]
 {first,last}AddrInPrefix4() already have a check for prefix length.
 (Although it should precede the assignment of the "addr" variable - which
 is redundant if the exception is thrown - and there should be a space
 around the ">" in lastAddrInPrefix4.) For this reason, I suggest the
 prefix lengths checks be removed from {first,last}!AddrInPrefix(), and
 checks added to the V6 versions (remembering to put spaces around the ">"
 in the comparison).

 (As an aside, the checks are more logical in the xxxPrefix{4,6}()
 functions: all the {first,last}!AddrInPrefix() functions should be doing
 is routing to the protocol-specific versions based on address family.)

 '''src/lib/dhcp/cfgmgr.cc'''[[BR]]
 getSubnet6(): Spurious space "if ( (subnets6_.size()...": there should be
 no space between the two opening parentheses.

 getSubnet6(): I think we've discussed this before, but what happens if the
 user has a configured subnet that does not match the configured single
 pool?  The logic here is that the single subnet is always chosen.  Also,
 suppose that there is a single V6 subnet that is not link local: it is
 rejected by the first check but could still be chosen in the subsequent
 iteration over the (single) subnet.

 '''src/lib/dhcp/duid.cc'''[[BR]]
 toText(): For consistency, either set the string stream's fill character,
 field width and output base with calls to "fill()", "width()", and
 "setf()" before the loop, or set them on a per-output call inside the loop
 with the manipulators hex, setfill and setw.

 '''src/lib/dhcp/duid.h'''[[BR]]
 It is not usual to write "operator==()" as "operator == ()" (same goes for
 the "not equals" operator.)

 ''' src/lib/dhcp/subnet.h'''[[BR]]
 get(): it is probably clearer to use the "make_pair" call:
 {{{
 std::pair<isc::asiolink::IOAddress, uint8_t> get() const {
     return (std::make_pair(prefix_, prefix_len_));
 }
 }}}
 (Also note that "get()" can be declared "const").

 toText(): this can be declared "const".

 '''src/lib/dhcp/tests/duid_unittest.cc'''[[BR]]
 toText() test: there is no need to create a DUID via "new" or used a
 scoped pointer to store it: it could be created as an automatic variable:
 {{{
 DUID duid1(data1, sizeof(data1));
 EXPECT_EQ("00:01:02:03:04:ff:fe", duid1.toText());
 }}}

 '''src/lib/dhcp/tests/lease_mgr_unittest.cc'''[[BR]]
 OK. (Looks as if I need to complete all the xxx6() methods in the MySQL
 lease manager code for it to be useful.)

 '''src/lib/dhcp/tests/subnet_unittest.cc'''[[BR]]
 toText() test: there is no need to create a Subnet4 via "new" or used a
 scoped pointer to store it: it could be created as an automatic variable.
 (See comment for duid_unittest.cc.)

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


More information about the bind10-tickets mailing list