BIND 10 #2522: support generic version of rdata::createRdata(text) in RP, MINFO, TSIG RDATA

[BIND 10 Development]

#2522: support generic version of rdata::createRdata(text) in RP, MINFO, TSIG
RDATA
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130528
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:15 pselkirk]:

 > Here is the proposed changelog entry:
 >
 > {{{
 > 613.?   [func]          pselkirk
 >         libdns++: All Rdata classes now use the generic lexer in
 >         constructors from text.  This means that the name fields in such
 >         RRs in a zone file can now be non-absolute (the origin name in
 that
 >       context will be used), e.g., when loaded by b10-loadzone.  Note
 >         that the existing string constructors for these Rdata classes
 also
 >         use the generic lexer, and they now expect an absolute name
 (with
 >         the trailing '.') in the name fields.
 > }}}
 >
 > Should this include all related tickets and commits, or just the ones
 not covered by prior changelog entries?

 I'd leave it to you.  But in reality no one would care about each
 specific ticket, so I'd simply say something like "multiple tickets"
 like this:
 {{{
 495.    [func]          team
         b10-auth now handles reconfiguration of data sources in
         background using a separate thread.  This means even if the new
         configuration includes a large amount of data to be loaded into
         memory (very large zones and/or a very large number of zones),
         the reconfiguration doesn't block query handling.
         (Multiple Trac tickets up to #2211)
 }}}
 I also suggest using 'team' like this example since this is a summary
 entry for various tickets done by multiple developers.

 Comments on the latest branch:

 '''others'''

 - I believe we can make more cleanups in
   rrparamregistry-placeholder.cc.  `OldRdataFactory` should be able to
   be removed, and I suspect we don't need
   `AbstractRdataFactory::create()` any more.
 - not clarifying privatizing impl class/struct in this ticket is okay,
   but would you create a ticket for that?  probably not only for the
   rdata implementations but throughout the source tree.

 '''tsig_250.cc'''
 - why do we use uint32_t and perform range check explicitly?
 {{{#!cpp
         try {
             error = boost::lexical_cast<uint32_t>(error_txt);
         } catch (const boost::bad_lexical_cast&) {
             isc_throw(InvalidRdataText, "Invalid TSIG Error");
         }
         if (error > 0xffff) {
             isc_throw(InvalidRdataText, "TSIG Error out of range");
         }
 }}}
   can't lexical_cast reject integral values > 0xffff? with uint16_t?
   I actually remember some (buggy?) implementations of lexical_cast
   show such behavior, but if that kind of oddity is the reason, I
   think we should leave a comment about it.
 - description for 'from string' constructor: the revised text looks
   good, but I guess we should keep synopsis like this:
 {{{#!diff
 -/// \c tsig_str must be formatted as follows:
 -/// \code <Alg> <Time> <Fudge> <MACsize> [<MAC>] <OrigID> <Error>
 <OtherLen> [<OtherData>]
 -/// \endcode
 }}}
   because otherwise some of the item names in the following part
   ("Error", "MAC" etc) are "undefined".

 '''rdata_minfo_unittest.cc'''

 - If we want to change the type of  too_long_label to `std::string`,
   it should be defined in the fixture; initializing such a
   non trivial type in a namespace scope tends to cause static
   initialization fiasco:
 {{{#!cpp
 const string too_long_label =
     "0123456789012345678901234567890123456789012345678901234567890123.";
 }}}
   From a quick look it should be defined in the fixture safely, so I
   made the change myself.

 '''rdata_minfo_unittest.cc'''
 - badText: I think the 'missing origin' case needs to check the case
   where one of the two names are absolute; otherwise the missing origin
   for the second name wouldn't be tested in effect.  This should be
   quite trivial, so I made the change myself.  same for RP.

 '''rdata_tsig_unittest.cc'''

 - string cannot be initialized at a namespace scope (see above).
 - isn't it missing tests for revised origin behavior (for key names)?
 - multi line case doesn't seem to be tested either (also for others)

 '''sshfp_44.cc'''

 - BIND 9 seems to allow multiline or space-separated fingerprint.  not
   clear from the RFC, but we should probably be compatible with BIND 9.
   (and clarify that in doc)
 - s/contrained/constrained/?
 {{{#!cpp
 /// The Algorithm and Fingerprint Type fields must be within their valid
 /// ranges, but are not contrained to the values defined in RFC4255.
 }}}
 - if you switch to this type of pimpl, you'll need to define
   `operator=()`; otherwise it would eventually cause duplicate free.
   (check that by writing a test)

 '''rdata_sshfp_unittest.cc'''
 - assuming we allow space-in-fingerprint or multi line input, we'll
   need test cases for them.

-- 
Ticket URL: <https://bind10.isc.org/ticket/2522#comment:16>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development