BIND 10 #2320: Allocation engine v4: Address assignment
BIND 10 Development
do-not-reply at isc.org
Sat Jan 5 21:29:57 UTC 2013
#2320: Allocation engine v4: Address assignment
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: task | Status:
Priority: medium | reviewing
Component: dhcp4 | Milestone:
Keywords: | Sprint-DHCP-20130122
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
-------------------------------------+-------------------------------------
Changes (by stephen):
* owner: stephen => tomek
Comment:
Reviewed commit c1fd5c76dc679f0156748f66f58f3d7220703332
'''src/bin/dhcp4/dhcp4_messages.mes'''[[BR]]
I've done a bit of word-smithing with some of the text in the file and
pushed the result. However, one point:
DHCP4_LEASE_ADVERT_FAIL/DHCP4_LEASE_ALLOC_FAIL: I removed the "Each
specific failure is logged in a separate log entry" text. I interpreted
it to mean that each occurrence of this condition would cause the message
to be logged (in which case the text is not really necessary). An
alternative interpretation is that additional messages will indicate the
reason. If the latter is the case, please correct the text.
'''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
Line 51: Comment: just the presence of the !RequirementLevel enum
indicates that there is a lot of commonality between the V4 and V6
servers. After delivery, we will have a refactoring task to extract the
common bits into the dhcpsrv library.
'''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
Lines 270/271:
{{{
setMsgType(answer, DHCPNAK);
answer->setYiaddr(...);
}}}
Two separate ways of fields in a packet. It seems more logical that
setMsgType() should be a method on the Pkt4 class.
Line 273: No need for the "else" clause - the debug message can go in the
main processing path.
Line 296-299. This can be shortened to
{{{
bool fake_allocation = (question->getType() == DHCPDISCOVER);
}}}
(Parentheses added to clarify the expression.)
Line 309: I thought that IA_NA was a DHCPv6 construct. (Looks like a cut
and paste error in the comment.)
setMsgType(): Question: is the logic such that the code may attempt to set
the message type in a packet in which a type is already set? If so, would
an attempt to set a different type indicate a logic error?
Line 247: getNetmaskOption(): Need documentation of the "subnet"
parameter.
Line 319: Question - is another ticket needed to handle the addition of
any options defined in the configuration database?
processRelease(): some comments in the code would be useful: in
particular, what is the significance of the "if" tests?
Line 439: Strictly speaking, the return of "false" from the call to
deleteLease() just means that the lease was not in the database: it does
not mean that the deletion operation failed. Such a condition merits a
warning message, not a debug message.
Line 439: Continuing from the last comment, the call to deleteLease()
should be enclosed in a try...catch block. An exception is generated if
the deletion fails. In fact, given that the getLease4() call at line 406
could also generate an exception, unless exceptions are being caught at a
higher level, the entire content of this method should be in a try...catch
block.
'''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
Please start all comments with a capital letter.
Even test classes and methods should be Doxygen-documented.
Line 195: Would prefer that something other than "tmp" is used as a
variable name (e.g. idopt (for ID option)). "tmp" should also be changed
at line 206.
Line 474 (and others): the "ASSERT_NO_THROW" should not have spaces after
the leasing "(" and before the trailing ")".
Line 481 (and other lines where the same comment appears): Passing a
DISCOVER to the server should result in an offer, not an advertise.
Line 920: Comment is incomplete.
Line 872 (and elsewhere): What does the comment "Generate client-id also a
duid" mean? I thought a DUID was a V6 idea.
Line 993: s/compination/combination/
'''src/bin/dhcp4/tests/dhcp4_unittests.cc'''[[BR]]
The commented-out code is not needed: see the documentation of the B10_*
environment variables in src/lib/log/README. (And yes I know, this
documentation should be put in Doxygen - it's on the "to do" list.)
'''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
Minor change made and pushed.
'''src/lib/dhcp/hwaddr.h'''[[BR]]
Even in structs, methods should be documented using Doxygen.
Should the class name be !HwAddr?
'''src/lib/dhcp/pkt4.cc'''[[BR]]
pack(): as 16 is a magic number regarding hardware lengths, it would be
better if it were defined as a public constant in the HWaddr class.
Line 129: spaces around the "?" and ":" operators.
Line 126: the "if" check does not seem to write out anything if hw_len is
more than 16: is this correct?
Line 179/180: is MAX_CHADDR_LEN the maximum hardware address (see comment
about pack()).
Line 268: would not using hwaddr_.reset() be better than constructing an
intermediate !HWAddrPtr?
Line 375: spaces arounf "!=".
'''src/lib/dhcp/tests/hwaddr_unittest.cc'''[[BR]]
toText test: comment ("last digit different") does not make sense here.
'''src/lib/dhcpsrv/alloc_engine.cc'''[[BR]]
Line 270, allocateAddress4: If the check is not necessary, why do it?
(The answer is that that is a "belt and braces" check.)
Line 276: s/check/Check/
Line 279: s/we/We/
Line 280: We also deal with female clients :-) s/his/its/
Probably don't need the blank lines at lines 281 and 283.
Line 298: s/continue/continue the/
Lines 295-299 - same comments as the previous "if" block.
Line 308: s/check/Check/
Line 315: s/the hint/The hint/
Line 337: s/number/the number/
Line 376: s/continue/Continue/
Probably don't need a blank line at line 548
'''src/lib/dhcpsrv/alloc_engine.h'''[[BR]]
AllocEndgine::Allocator::pickAddress: needs Doxygen description of
arguments and return value.
Line 200: s/sending/the sending of a/
Line 202: s/allocation/the allocation/
Line 205: s/still/the still/
Line 204: s/creates/Creates/
Line 247: s/i.e./e.g./ (The former suggests what follows is the only case,
the latter suggests that what follows is one of many possible cases.)
Line 281: s/reuses/Reuses/
'''src/lib/dhcpsrv/mysql_lease_mgr.cc'''[[BR]]
Line 415: We could store T1/T2 in the MySQL database if there is a need.
'''src/lib/dhcpsrv/subnet.cc'''[[BR]]
getPool(): How about providing a base class getPool() requiring a single
argument. Then the derived classes getPool() definitions supply the
appropriate default argument and just call the base class. In other
words:
{{{
class Subnet {
PoolPtr getPool(isc::asiolink::IOaddress hint);
};
class Subnet4 {
PoolPtr getPool(isc::asiolink::IOaddress hint = IOAddress("0.0.0.0"))
{
return (Subnet::getPool(hint));
}
// .. and the same for Subnet6 except with a hint of "::".
// (This approach would also allow "hint" to be declared "const".)
}}}
'''src/lib/dhcpsrv/tests/alloc_engine_unittest.cc'''[[BR]]
Would like the test classes and methods to be documented.
It might be easier (are more understandable) to split the !AllocEngine
unit tests into two files, one for V4 and one for V6.
Please start comments with a capital letter (lines 541 onwards).
Lines 702/703/711: s/*/ * /
Line 715: If you are only needing to check whether an address has been
seen or not, and don't need to associate any value with it, a set is
probably better than a map.
'''Running Unit Tests'''
I got an error running the unit tests:
{{{
[ RUN ] Pkt4Test.fixedFields
pkt4_unittest.cc:223: Failure
Value of: memcmp(dummyChaddr, &pkt->getHWAddr()->hwaddr_[0], 16)
Actual: -1
Expected: 0
[ FAILED ] Pkt4Test.fixedFields (0 ms)
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/2320#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list