BIND 10 #2238: Pool/Lease Configuration - IPv6
BIND 10 Development
do-not-reply at isc.org
Fri Sep 21 12:42:26 UTC 2012
#2238: Pool/Lease Configuration - IPv6
-------------------------------------+-------------------------------------
Reporter: | Owner: tomek
stephen | Status: reviewing
Type: task | Milestone: Sprint-
Priority: | DHCP-20121004
medium | Resolution:
Component: | Sensitive: 0
dhcpconf | Sub-Project: DHCP
Keywords: | 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 b8f538b9fcf70394e5737269d4bed660721c245d
'''!ChangeLog'''
Suggest change to the entry:
{{{
4XX. [func] tomek
A new library (libb10-dhcpsrv) has been created. At present, it
only holds the code for the DHCP Configuration Manager. Currently
this object only supports basic configuration storage for the
DHCPv6
server, but that capability will be expanded.
(Trac #2238, git TBD)
}}}
'''src/lib/asiolink/io_address.h'''[[BR]]
smallerThan(): correct name, but "lessThan()" strikes me as a better one.
(Many languages use "lt" or "le" for "<" or "<=" comparisons.)
smallerThan(): OK, although a small restructuring of the code can reduce
the number of comparisons in the (probably more common) case of address
families being the same:
{{{
if (this->getFamily() == other.getFamily()) {
if (this->getFamily() == AF_INET6) {
return (<V6 expression>);
} else {
return (<V4 expression>);
}
}
// Family equality has already been considered
return (this->getFamily() < other.getFamily());
}}}
(... and the code is slightly shorter.)
'''src/lib/asiolink/tests/io_address_unittest.cc'''[[BR]]
Would suggest that the new test be named "lessThanEqual" (or something
like that): there is already test for addresses being equal, and "compare"
implies that it includes an equality check.
'''src/lib/dhcp/addr_utilities.cc'''[[BR]]
Some comments would be useful. I worked out that the logic is to copy the
entire prefix, if the mask is not a multiple of 8 bits, use the number of
bits modulo 8 as an index into the bitMask array to get the mask at that
point, then zero out the rest of the array. But it did take me a few
minutes to work that out.
Note that the code has "len / 8" at some points and "len/8" at others.
Trivial point: when doing operations on the "packed" array, using
"sizeof(packed)" as the maximum index always seems clearer. Also, I know
that 16 is the length of a V6 address, but it is a "magic number" and a
comment when "packed" is declared might be appropriate.
Would it be useful to name these functions to include the protocol family?
There might be a requirement in the future to work out the first and last
addresses in a V4 address given in CIDR notation.
'''src/lib/dhcp/addr_utilities.h'''[[BR]]
In the comment for firstAddrInPrefix, there is a typo: the first address
for a prefix length of 120 in 2001:db8:1:dead:beef ends be00, not bee0.
'''src/lib/dhcp/cfgmgr.cc'''[[BR]]
Since the address utilities compare two addresses of any family, it
strikes me that the "first > last" test can be done just once in the
"Pool" constructor, and removed from the Pool4/Pool6 constructors. (Tests
on the address family etc. would have to stay in Pool4/6.) OTOH, having
all the tests in one place does have merits. I'll leave the decision up
to you.
Pool6 constructor (first one): typo in comment: "1something"
Subnet::inRange(): see comments for cfgmgr.h.
Subnet::inRange(): Not usual to have space after/before opening/closing
parentheses (e.g. "((first <= addr) ..." is preferred to "( (first <=
addr)".
Subnet6 constructor: message associated with !BadValue exception could
more accurately reflect the reason it was thrown, e.g. "non IPv6 prefix
specified in subnet6".
Subnet6::addPool6: as this is being done during configuration, run-time is
not too important. Suggest keeping the pools sorted by order of first
address, and in getPool6(), using a binary_search to locate the relevant
pool.
CfgMgr::getSubnet6: not clear about the logic: if there is only one
subnet, we use it regardless of the hint. If there is more that one, we
use the hint to locate it and if we can't find it,
'''src/lib/dhcp/cfgmgr.h'''[[BR]]
Typo: "specifes"
More common to use "operator=()" than "operator = ()". Similarly "operator
T()" (if the template parsing allows it).
Space between operators in Triplet constructor ("min_>def")
Pool::getId() needs an "@return" in the header.
Can Pool::inRange() be declared "const"?
Typo "indentify".
Pool6: presume that the attributes are declared protected for testing. If
so, there should be a comment to that effect.
Although all these classes are related to the configuration, I suggest
separating out the "Pool" classes into a separate file on the grounds that
it will be easier to find and edit them as the code grows. Similarly for
the Subnet classes.
Can Subnet::inRange() be declared "const"?
Subnet6 methods should be documented with Doxygen headers.
!CfgMgr Header: referring to the example you give, an alternative idea for
inheritance (and I'm not saying change your idea, I'm just raising the
possibility of an alternative) is to implement global parameters in a
separate singleton class, and subnet-specific parameters in the Subnet6/4
class. When a parameter is requested, the Subnet4/6 method calls the
singleton-class method if it doesn't have the data. The idea here is to
simplify the configuration processing - global parameters go into the
singleton class, subnet-specific parameters into instances of the
Subnet4/6 classes. So, for example, if a global parameter is changed,
only the singleton class needs to be updated: there is no need to check
all instances of the Subnet4/6 classes to see if they have values that
need updating.
!CfgMgr: as we are having separate processes for IPv4 and IPV6, does it
make sense to have separate configuration managers for these processes (in
which case, should the class be CfgMgr6)?
subnets6_: regarding the comment on this attributes, a simple vector is
fine. However, if we alter inRange() to return -1/0/1 depending whether
an address is below the start of range, in the range, or above the range
(or create another comparison function that does it) and keep the subnets
vector sorted by order of start address, we could use the STL
binary_search algorithm when locating a subnet.
'''src/lib/dhcp/tests/addr_utilities_unittest.cc'''
I think that limit cases (e.g. "::/1", "::/128") should also have tests.
280 0 src/lib/dhcp/tests/cfgmgr_unittest.cc
"min", "max" and "value" can be declared as "const uint32_t".
If you've used symbolic names for the limits in Triplet, suggest that you
use the same symbols for the expected results in the appropriate part of
the tests (e.g. expected value for x.getMin() is "min").
operator test - these tests doesn't actually check operator=() - they
check the constructor. The construct:
{{{
Triplet<uint32_t> y = x
}}}
... is another way of writing
{{{
Triplet<uint32_t> y(x);
}}}
To test the operator=() function, you need something like:
{{{
Triplet<uint32_t> y(0);
y = x;
}}}
constructor_prefix_len test: Should also check for invalid prefix lengths.
'''src/lib/dhcp/tests/run_unittests.cc'''[[BR]]
Do you really need to remove logger support? Although libdhcp++ will not
including logging (as it is intended to be used by external programs),
libb10-dhcpsrv doesn't have that restriction as it is for used by the
server images. At some point we may want to add logging to it.
'''Response to 'Things to consider' in comment:2'''
> this ticket covers configuration storage only. It is intended to store
v6 server configuration. The actual configuration parser (that translates
user's configuration into structures defined) is part of a separate
ticket, see #2269.
One of the comments above suggests separate CfgMgr4 and CfgMgr6 classes.
> Currently classes Pool, Pool6, Subnet, Subnet6 are stored in the same
files as !CfgMgr?. As they are part of the !CfgMgr?, they may be possibly
defined as internal classes (e.g. CfgMgr::Pool, rather than just a Pool).
Alternatively, they could be moved to separate files. Finally, they can be
left as they are now.
As noted above, I suggest moving them to separate files. The files are
smaller to edit, and the name of the file clearly indicates the contents.
> If would be nice if Pool and Subnet were abstract classes.
Unfortunately, I couldn't find any method that would be required in both
derived versions (Pool4 and Pool6) and take a parameter that is common for
both derived classes.
The identification of the pool/subnet could be handled in a base class.
Also, utility functions such as an address being in a given range,
checking that "first < last", etc.
> I'm not sure if pointers to Pool and Subnet are really needed. I would
like to hear reviewer's opinion on that.
Leave the definitions in for now. If it turns out that they are not used,
they can always be removed (or moved to the test files where they are
used).
> Parameter inheritance is planned to be implemented as part of the
configuration parser logic and the inheritance will be done at
configuration phase. That means that Subnet6 objects will be instantiated
with their "ultimate" values with inheritance already applied if
necessary. That will make configuration phase slightly slower, but will
make the actual operation faster.
See comments above. The overhead of splitting the global/specific
parameters when accessing the values is only a few instructions (probably
just one "if" if the accessor functions are inline). Opposed to this is
more complicated parser logic (and reconfiguration logic) if objects are
instantiated with their "ultimate" values.
--
Ticket URL: <http://bind10.isc.org/ticket/2238#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list