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:  tmark
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  Unclassified  |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130328
           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 tomek):

 * owner:  tomek => tmark


Comment:

 Replying to [comment:4 tmark]:
 > Overall, very thorough work and new unit tests appear to cover the
 bases.
 Thanks.

 > 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.
 We can't. MLM_TRUE and MLM_FALSE are defined as const. MySQL structures
 are used for both read and write, so the pointer must be to non-const
 my_bool. Hence the client_id_null_ field.

 > 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.
 Before every operation the whole bind_ structure is initialized to zeros
 using memset. Setting up is_null to MLM_FALSE is technically redundant. It
 has the benefit of reminding us that there is such a field. On the other
 hand it is marginally slower. Our performance measurements indicate that
 MySQL is a the major bottleneck for now, so I'm a bit hesitant to add
 extra redundant code that will make it slower.

 For now I've added the code to set the field to MLM_FALSE, but it is
 commented out.
 Would that be a reasonable compromise?

 > 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.
 No, in the second insert the is_null pointer will be set to NULL.

 > Under commit b312e9ab66e413ee0708fc2db23d27e7f4b7b251:
 > +        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.
 They are now.

 If those answers are satisfactory, just say that the code is ready to
 merge and reassign the ticket back to me.

 Thanks for the review.

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