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