BIND 10 #3194: Vendor options support is needed in Kea
BIND 10 Development
do-not-reply at isc.org
Wed Oct 23 11:16:40 UTC 2013
#3194: Vendor options support is needed in Kea
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: enhancement | Status:
Priority: high | reviewing
Component: dhcp | Milestone:
Keywords: | Sprint-DHCP-20131016
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity:
Total Hours: 0 | Medium
| Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: => tomek
* status: new => reviewing
Comment:
The commits on branch trac3194 had wrong descritpions (starting with the
wrong branch name). Therefore I moved all the commits from the trac3194 to
the new branch trac3194_1 and I made just a single commit out of them. I
reviewed this commit on the trac3194_1 branch:
0764a74e461d71c8d3dcb25dc2399a4d18d553f8
Then, I made some changes on the branch which I am hoping you'll review.
Please note that there is one unit test is now failing because I am
expecting ORO to be returned as OptionUint8Array, not Option. This however
will not work until we merge the master to this branch. I haven't merged
it yet to avoid messing this up further.
'''src/bin/dhcp4/config_parser.cc'''
'''src/bin/dhcp6/config_parser.cc'''
findServerSpaceOptionDefinition: There is not much sense to call
SubnetConfigParser::findServerSpaceOptionDefinition if option space is
"dhcp4". Therefore, the function could look like this:
{{{
virtual OptionDefinitionPtr findServerSpaceOptionDefinition (
std::string& option_space, uint32_t option_code) {
OptionDefinitionPtr def;
if (option_space == "dhcp4" &&
LibDHCP::isStandardOption(Option::V4, option_code)) {
def = LibDHCP::getOptionDef(Option::V4, option_code);
} else if (option_space == "dhcp6") {
isc_throw(DhcpConfigError, "'dhcp6' option space name is
reserved"
<< " for DHCPv6 server");
} else {
// Check if this is a vendor-option. If it is, get vendor-
specific
// definition.
uint32_t vendor_id =
SubnetConfigParser::optionSpaceToVendorId(option_space);
if (vendor_id) {
def = LibDHCP::getVendorOptionDef(Option::V4, vendor_id,
option_code);
}
}
return (def);
}
}}}
It seems to me that there are no tests for parsing vendor options for
DHCPv4.
'''src/bin/dhcp4/dhcp4_srv.cc'''
There is nothing to do here. I did some modifications to the
appendRequestedVendorOptions to use OptionUint8Array instead of generic
option and some other trivial changes.
Also, I removed the bits that parsed vendor options from the callback
function (unpackOptions) because this logic had been moved to option
definition.
'''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
The Dhcpv4Srv::vendorOptionsDocsis is a good and simple test. But, we
should probably consider extending it to test multiple options, not just
one. It could also be extended to cover attempts to get options from non-
existent option spaces etc.
I wonder if vendor options tests shouldn't be made a separate class of
tests (perhaps in a separate file) as I presume there will be many
different tests that we may create here.
For the sake of this ticket though, todo is sufficient.
'''src/bin/dhcp6/dhcp6_srv.cc'''
A really minor thing: Please consider swapping the following code:
{{{
uint32_t vendor_id = vendor_req->getVendorId();
}}}
with the check if option ORO is present. It is likely that the ORO is
absent and in such case you don't need to initialize vendor_id. But, since
it is a tiny performace penalty I leave it up to you.
Also, I removed the bits of code in the callback function (unpackOptions)
which are responsible for parsing a vendor option as this logic had been
moved to OptionDefinition.
'''src/bin/dhcp6/tests/config_parser_unittest.cc'''
Do you think it makes sense to do similar tests for options defined with a
subnet, rather than global only?
'''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
Dhcpv6SrvTest::docsisVendorOptionsParse: It would be a great help if you
used option names instead of raw codes: 1, 36, 35...
Dhcpv6SrvTest::docsisVendorORO: A comment !''Check if the packet
contain!'' seems to be missing the second part (what does it contain?).
Also, it should be !''contains!''.
Please consider using ASSERT_NO_THROW instead of EXPECT_NO_THROW in this
statement:
{{{
EXPECT_NO_THROW(sol->unpack());
}}}
because further test doesn't make sense if unpack fails.
'''src/lib/dhcp/docsis3_option_defs.h'''
Error in last line: STD_OPTION_DEFS_H. It should be DOSCIS3_OPTION_DEFS_H
'''src/lib/dhcp/libdhcp++.cc'''
Can initStdOptionDefs4 use initOptionSpace just like other functions of
this kind?
unpackVendorOptions6 and unpackVendorOptions4: The comment near:
{{{
if (idx) {
}}}
is wrong. You're checking if index exists, not if there is an option
definition. Also, I am not sure if there is any need to make index a
pointer. Index is always present - this is guaranteed by the declaration
of the multi index container.
'''src/lib/dhcp/libdhcp++.h'''
All new functions and class members deserve doxygen comments.
getVendorOptionXDefs: Why do you have to return a pointer, instead of a
reference?
'''src/lib/dhcp/option_vendor.cc'''
pack: The format of the data fields following enterprise id is different
for V4 and V6 (if I understand that correctly). When calling packOptions()
for V4 you don't take into account the data-len field in V4 VIVSO option.
The same applies for len(). I may be wrong....
unpack: Please consider including the length of the provided buffer in the
exception string.
'''src/lib/dhcp/option_vendor.h'''
I would extend the comment for the class to indicate that the same class
represents an option for V4 and V6 and that the format of the option is a
bit different because the data-len field is present in V4.
Constructor: it is not true that you throw InvalidDataType exception. The
same applies for pack() and unpack().
Please correct the descriptions for setVendorId and getVendorId.
The last line of the header is invalid.
'''src/lib/dhcpsrv/dhcp_parsers.cc'''
Unrecognized comment !''thomson!'' in dhcp_parsers.cc (line 1055).
optionSpaceToVendorId:
Why enterprise numbers are limited to four digits minimum? (line 1108)
Apparently, there are no tests for this function.
'''src/lib/dhcpsrv/dhcp_parsers.h'''
The doxygen description of the optionSpaceToVendorId lacks description of
the option_space parameter. And again, it would be good to explain why the
format is !''vendor-1234!'' or what is the minimum value (and why) of
!''1234!''.
'''src/lib/dhcpsrv/option_space_container.h'''
Comments like:
{{{
/// @param option_space name of the option space.
}}}
or
{{{
/// A map holding container (option space name is the key).
}}}
no longer apply because you may use different selector.
'''src/lib/dhcpsrv/subnet.h'''
The new functions deserve doxygen descriptions.
--
Ticket URL: <http://bind10.isc.org/ticket/3194#comment:1>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list