BIND 10 #2723: Improve code to handle NULL and too short DUIDs and client-ids

[BIND 10 Development]

#2723: Improve code to handle NULL and too short DUIDs and client-ids
-------------------------------------+-------------------------------------
            Reporter:  jwright       |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  Unclassified  |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130228
           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 tmark):

 * owner:  tmark => tomek


Comment:

 Overall, very thorough work and new unit tests appear to cover the bases.
 Two minor comments:

 Under commit 6cbb6ed662f7ee7b959865eb741c6c0029d0afd8:

 :
 +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc
 @@ -353,9 +353,9 @@ public:
              // fields doesn't matter if type is set to MYSQL_TYPE_NULL,
              // but let's set them to some sane values in case earlier
 versions
              // didn't have that assumption.
 -            static my_bool no_clientid = MLM_TRUE;
 +            client_id_null_ = MLM_TRUE;
              bind_[2].buffer = NULL;
 -            bind_[2].is_null = &no_clientid;
 +            bind_[2].is_null = &client_id_null_;
          }

          // valid lifetime: unsigned int

 -------------------------------------------------------------------
 Comment:
 Depending on one's interpretation of the MySQL, documentation,
 we should set always set is_null to the appropriate value for each
 bind "field".  This leaves nothing to chance.

 For the input (insert), we should just use &MLM_TRUE or &MLM_FALSE.

 If you're going to use client_id_null_ for the insert bind, you should
 use it in both cases.  Currently, it gets set to TRUE but not FALSE
 so its value could be inconsistent.

 If I insert one that is null (client_id_null is set TRUE), followed
 by one that isn't, then client_id_null may still be TRUE. Harmless
 but inaccurate.
 -------------------------------------------------------------------


 Under commit b312e9ab66e413ee0708fc2db23d27e7f4b7b251:
 :

 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
 +++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
 @@ -160,7 +160,15 @@ public:
          EXPECT_EQ(subnet_->getT2(), lease->t2_);
          EXPECT_TRUE(false == lease->fqdn_fwd_);
          EXPECT_TRUE(false == lease->fqdn_rev_);
 -        EXPECT_TRUE(*lease->client_id_ == *clientid_);
 +        if (lease->client_id_ && !clientid_) {
 +            ADD_FAILURE() << "Lease4 has a client-id, while it should
 have none.";
 +        }
 +        if (!lease->client_id_ && clientid_) {
 +            ADD_FAILURE() << "Lease4 has no client-id, but it was
 expected to have one.";
 +        }
 +        if (lease->client_id_ && clientid_) {
 +            EXPECT_TRUE(*lease->client_id_ == *clientid_);
 +        }
 ------------------------------------------------------------------
 Comment:
 Shouldn't the 3 new ifs above be  if-elseif-elseif?  Only one of them
 can be true.
 ------------------------------------------------------------------

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