BIND 10 #2320: Allocation engine v4: Address assignment

[BIND 10 Development]

#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
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:6 stephen]:
 > '''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.)
 Thanks for the info. I didn't know about that. Removed commented out code
 and added pointer to the documentation.

 > '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 > Minor change made and pushed.
 Thanks.

 > '''src/lib/dhcp/hwaddr.h'''[[BR]]
 > Even in structs, methods should be documented using Doxygen.
 Added.

 > Should the class name be !HwAddr?
 Hardware is often abbreviated as HW (two capital letters). Personally I
 consider it an acronym rather than abbreviation. This is also similar to
 DUID, which is usually written uppercase. If you think this really is a
 problem, I can fix that.

 > '''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.
 There is MAX_CHADDR_LEN defined already. I have updated the code to use
 it.

 > Line 129: spaces around the "?" and ":" operators.
 Fix.

 > Line 126: the "if" check does not seem to write out anything if hw_len
 is more than 16: is this correct?
 Yes. This covers the case of infiniband, which has hardware address of
 size 20. There's RFC about it and it says that hardware should be set to
 zero and the actual hardware address should be sent as client-id option.
 We will get back to this once there will be a first user who is interested
 in running Kea on Infiniband hardware.

 > Line 179/180: is MAX_CHADDR_LEN the maximum hardware address (see
 comment about pack()).
 Yes.

 > Line 268: would not using hwaddr_.reset() be better than constructing an
 intermediate !HWAddrPtr?
 Done.

 > Line 375: spaces arounf "!=".
 Done.

 > '''src/lib/dhcp/tests/hwaddr_unittest.cc'''[[BR]]
 > toText test: comment ("last digit different") does not make sense here.
 Removed.

 > '''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/
 Done all.

 > Probably don't need a blank line at line 548
 With all earlier comments, I lost track which blank line you are referring
 to, so I just removed 2 blank lines that are not strictly needed there.

 > '''src/lib/dhcpsrv/alloc_engine.h'''[[BR]]
 > AllocEndgine::Allocator::pickAddress: needs Doxygen description of
 arguments and return value.
 Added.

 > 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/
 Done all.

 > '''src/lib/dhcpsrv/mysql_lease_mgr.cc'''[[BR]]
 > Line 415: We could store T1/T2 in the MySQL database if there is a need.
 We should discuss this and make a consistent decision - either all
 backends store it or none at all. We can talk about this after the
 release. For now let's keep it as it is.

 > '''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".)
 > }}}
 Interesting concept, but I think it wouldn't work. I need a pointer to
 Subnet, because that was the whole point of using common code for both
 Subnet4 and Subnet6. Let's assume that I have Subnet* pointer to Subnet6
 and call getPool() without any parameters. It may work. But what would
 happen if Subnet* points to Subnet object? Then you can't call getPool()
 without parameters. And you can't detect which of those cases occur in
 compile time. So if it even compiles, if would sometimes work and
 sometimes not.

 But it's interesting problem. I'll get back to it after doing all other
 review items.

 > '''src/lib/dhcpsrv/tests/alloc_engine_unittest.cc'''[[BR]]
 > Would like the test classes and methods to be documented.
 Documentation added.

 > It might be easier (are more understandable) to split the !AllocEngine
 unit tests into two files, one for V4 and one for V6.
 I'm hoping to have more tests reusing the same methods. It will be easier
 to keep those tests together, especially that large parts of the
 allocation engine are common.

 > Please start comments with a capital letter (lines 541 onwards).
 > Lines 702/703/711: s/*/ * /
 Done.
 > 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.
 Using set now in both v4 and v6 tests.


 > '''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)
 > }}}
 Ok. getHWAddr()->hwaddr_ is now a vector and has variable length up to 16.
 Comparing the whole 16 byte range was buffer overrun. Fixed that.

 This concludes this part of the review. The only outstanding thing to do
 is to address your comment about default value in derived classes.

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