BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121101
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpdb                             |           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      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:11 stephen]:

 > > !CheckTimeConversion: This test does not check that the conversion was
 done properly, just that it is reversible. The code could mess up months
 with minutes and days with seconds and the test would still pass.
 > > Please check boundary conditions. Verify that the expire time is
 properly calculated if...
 > I'm a bit wary of doing that because of DST issues - setting a "struct
 tm" and converting to a time_t requires knowledge of whether DST is in
 effect at that time.  And the reverse conversion will use the local tables
 to calculate DST.  However, I've modified the test to check that the
 MYSQL_TIME produced is the one that I would calculate by adding cltt and
 the valid lifetime and converting using localtime_r.  The reverse
 conversion checks that it reproduces the original time_t value.
 Ok. We'll have the opportunity to check that next week - Poland (and most
 of Europe as well) is moving to winter time.

 I'm afraid that is something we'll need to sort out before first release,
 because clients losing leases twice a year (and on end of February every 4
 years) would be very bad.
 The amount of coding here is minimal, but non-trivial research is needed.
 I think it is a worthy candidate for a separate ticket.

 > > Do we really need hwaddr in Lease6? I think we should remove it and
 the sooner we do it the better.
 > Probably not. Remove.  (Err.. BTW, you were the one who wrote
 lease_mgr.h and first put it in the Lease6 structure!)
 I honestly admit to my mistakes. At first I thought that the lease6
 structure would look  empty.

 > > that hardware address of 20 bytes can be stored and retrieved
 > A few lines ago, you suggested that the Lease6 hardware address should
 be removed.  This has been done.
 Ok, great.

 > > check if the backend refuses to accept larger values.
 > For what - apart from prefix_len, for everything the maximum value is
 set by the data type.
 Does the backend allow storing prefixes /129 and larger? That shouldn't be
 allowed.

 > > check that CLTT is able to properly store February 29th on leap years
 > > check that the backend will refuse dat Feb. 29th on regular year.
 > The backend accepts data in time_t format and will convert it to the
 appropriate local time.  Similarly, when it retrieves data, the result is
 converted to time_t format.  So these checks really boil down to whether
 MySQL can handle leap years, which is out of scope for these checks.
 Ok, so we're covered here. Great.

 > > check that prefix_len larger than 128 will be refused.
 > > check that IPv4 address will be refused in lease6.
 > These properly belong in the Lease6 class (and it is becoming a class,
 rather than a struct).  The database layer should assume that given a
 Lease6 object, the data in it is correct.  When it retrieved data, it
 should be that the Lease6 object being created from the data detects that
 it is incorrect.  Otherwise, knowledge of what constitutes valid data in
 Lease4/6 is split into several places.  A new ticket (#2405) has been
 created for it.
 Ok, fair point. The only reason I started lease4/6 as a structure is that
 I didn't want to spend many hours writing trivial getters/setters, tests
 for them, then dealing with lifetime issues for references returned to
 internal vectors etc.

 Ok, there's one more issue: BasicLease6 test started failing for me. Since
 the schema changed, I manually drop table'd all tables and then run
 dhcpdb_schema.mysql again.

 [ RUN      ] MySqlLeaseMgrTest.BasicLease6
 mysql_lease_mgr_unittest.cc:264: Failure
 Value of: second->cltt_
   Actual: -66464956333
 Expected: first->cltt_
 Which is: 234567
 [  FAILED  ] MySqlLeaseMgrTest.BasicLease6 (116 ms)

 I just looked at the BasicLease6 test and you seem to use
 EXPECT_TRUE(l_returned); If the lease is NULL, you print a failure, but
 continue to use the value. It should be ASSERT_TRUE.

 It fails with the l3 compare in line 367. I'll try to investigate it and
 see if I'll be able to fix it.

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