BIND 10 #3147: PD: MySQL LeaseMgr support for Lease6 type PD

[BIND 10 Development]

#3147: PD: MySQL LeaseMgr support for Lease6 type PD
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcpdb        |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130918
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 91c0c0605d92d36bf5e51a3b41548455542a26e3
 >
 > '''src/lib/dhcpsrv/dhcpsrv_messages.mes'''[[BR]]
 > Either:
 >  "s/, lease type %2/and lease type %2/"
 > in DHCPSRV_MYSQL{ADD,GET,UPDATE}_ADDR6 or
 >  "s/and lease type %n/, lease type %n/"
 > in DHCPSRV_MYSQL_GET_IAID_[SUBID_]DUID (to get a consistent style in
 recording the lease type in a message.)

 I elected the version without "and".
 >
 > If we go with the current message naming convention, the last two
 message should be renamed to have _TYPE appended to the symbol.  However,
 I think that makes the name too long, so leaving them as is should be OK.

 Yes, this would be strictly in keeping with the approach but as you point
 out readability is an issue too.
 I left the names alone.

 >
 > '''src/lib/dhcpsrv/mysql_lease_mgr.cc'''[[BR]]
 > GET_LEASE6_ADDR query: "and" should be in capital letters to conform to
 the style of other queries.
 >

 Oops.  Fixed.

 > I note that in all cases, when the MYSQL_BIND "buffer" variable is
 assigned, the rvalue is cast to "char*".  I'm not certain why I did that
 (I wrote the original code); looking at the
 [http://dev.mysql.com/doc/refman/5.5/en/c-api-prepared-statement-data-
 structures.html MySQl documentation] and the definition of MYSQL_BIND in
 mysql.h, the "buffer" variable is a "void*".  There is no need to change,
 as the variable is automatically cast to the correct type, but maybe
 create a ticket to correct this in the code?
 >

 I have created ticket 3169 to address your concerns.  Though, with your
 thoroughness I'm fairly certain that whoever takes on this ticket will
 find out in short order why you did it this way.

 >
 > '''src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc'''[[BR]]
 > lease6LeaseTypeCheck header: s/duid/DUID/, s/iad/IAID/.  The same change
 is needed in the comment that starts "Make Two" (and s/Two/two/).
 >
 > lease6LeaseTypeCheck test: see to be some debug code still in the test
 writing to stdout.

 Dagnabit.  Removed.


 >
 > lease6LeaseTypeCheck test: Why is "tick" so-named? In any case, it is
 not really needed; it seems that the reason for two loops is the need to
 alternate the subnet ID and set the lease type in groups of two. the code
 is easier written with a single loop using integer division and remainder:
 > {{{
 >     vector<Lease6Ptr> leases;
 >     for (int i = 0; i < 6; ++i) {
 >          Lease6Ptr lease(new Lease6(*empty_lease));
 >          lease->type_ = leasetype6_[i / 2];
 >          lease->addr_ = IOAddress(straddress6_[i);
 >          lease->subnet_id_ += (i % 2);
 >          leases.push_back(lease);
 >          EXPECT_TRUE(lmptr_->addLease(lease));
 >     }
 > }}}
 > (The same applies to the other loop where "tick" is used.)

 Done.

 >
 > lease6LeaseTypeCheck: Need spaces around binary operators, e.g. s/i*2/i
 * 2/, s/i+1/i + 1/ etc.

 Got it.

 >
 > lease6LeaseTypeCheck: In general use pre-increment rather than post-
 increment for counters unless there is a real need. (Doesn't make much
 different for an int" but could be significant on a more complex
 iterator.)
 >

 Done.

 > lease6LeaseTypeCheck: as six leases have been created, wouldn't it be
 logical to check all the leases instead of just half of them? (The
 verification loops only check the first three leases.)

 Done.

 >
 > lease6LeaseTypeCheck: When getting a collection of leases by type, DUID
 and IAID, an assumption seems to be made that the order of the returned
 leases within the Lease6Collection is the same as the order in which they
 were added to the database.  Although this works, there is no default
 order in the records returned by a SELECT statement, and it is feasible
 that the order used may change between MySQL releases.  (That's why the
 call to the STL sort routine appears elsewhere in the test code, e.g.
 getLease4Hwaddr.)
 >
 Had not thought of that.  Done.
 >
 > '''!ChangeLog'''[[BR]]
 > s/paramater/parameter/
 >

 {{{
 6xx.    [func]     tmark

 MySQL backend used by b10-dhcp6 now uses lease type as a
 filtering parameter in all IPv6 lease queries.
 (Trac# 3147,  git TBD)
 }}}

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