BIND 10 #2095: Define and implement RdataEncoder class

[BIND 10 Development]

#2095: Define and implement RdataEncoder class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120731
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  2.96
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * totalhours:  0 => 2.96


Comment:

 Replying to [comment:13 jinmei]:
 > Replying to [comment:12 vorner]:
 > >  * The code in `updateOtherData` is confusing. I'm not really sure how
 to improve that, maybe placing the variable length thing into an else
 branch instead of using continue could be an option.
 >
 > I agree it's not very understandable.  In fact I didn't really like
 > that implementation either.  On closer look I found it could be much
 > more simplified, and I did it at commit 8560883.

 It is much better now. However, there some problems introduced here:
 {{{#!c++
 /// <0 or more fixed/var-len data fields>,
 /// <0 or more domain name fields>,
 /// <0 or more fixed/var-len data fields>,
 /// <0 or more domain name fields>,
 }}}

 I don't think this is accurate. There is 0 or 1 data fields. And there's
 always 1 or more name fields.

 Also, here:
 {{{#!c++
         // Then, we still have a field in the spec, and it must be a
 domain
         // name field.  Since we know we've passed any prior data field,
 the
         // next field must be a domain name as long as it exists;
 otherwise
         // it's a bug in the spec (not a bogus input).  So we assert()
 that
         // condition.
         if (current_field_ >= encode_spec_->field_count) {
             isc_throw(BadValue,
                       "RDATA encoder encounters an unexpected name data: "
 <<
                       name);
         }
 }}}

 The comment talks about assert, but I see none. Also, the comment looks
 slightly repetitive to me?

 > > Also, I'd like to see the thing does not go outside the buffer that is
 passed to it. Would it be possible to do a test with a canary vale after
 the data and see it survived?
 >
 > you mean as a result of encode()?  I think the assert() check would
 > catch any such bugs, but I've added some explicit check in the
 > unittests with canary data (commit 84e7463).

 It's funny to see you placing an already `dead` canary there, but
 otherwise OK.

 So, except for the comments, I think it can be merged.

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