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:  4             |                 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):

 '''others'''

 - In my understanding, we'll complete the conversion work for all
   currently supported RR types with this task and the latest master,
   except the very trivial ones for `hs_4::A` and `ch_3::A`.  These
   missing ones should be trivial (for now, because they would be just
   empty definition).  So I suggest completing these too, and update
   the gen-rdatacode script by removing new_rdata_factory_users and
   updating generate_rrparam (we shouldn't need `OldRdataFactory`
   any more).
 - on a related issue, we'll need a changelog entry, saying we've
   completed the conversion (not only for RP, MINFO, TSIG).
 - I made a few trivial style fixes.

 '''tsig_250.cc'''

 - we can now probably remove these:
 {{{#!cpp
 #include <util/strutil.h>
 using namespace isc::util::str;
 }}}
 - `constructFromLexer`: we shouldn't need to create a root name as we
   have a constant:
 {{{#!cpp
     const Name& algorithm = createNameFromLexer(lexer,
 &Name::ROOT_NAME());
 }}}
 - But is it okay to use the root name unconditionally?  at least it
   doesn't seem to be compatible with BIND 9, so we'll need to adjust
   documentation.  We should also clarify that in the from-lexer
   constructor (that we ignored any non NULL name passed).
 - time_str, mac_txt, error_txt can be a reference.

 '''tsig_250.h'''
 - is there a reason for making `TSIGImpl` public?  Although wouldn't
   matter much in practice, it would be generally better to be hidden
   as much as possible due to its nature.  So in that sense I thought
   the previous definition makes more sense.

 '''minfo_14.cc'''

 - from lexer constructor: maybe a matter of taste, but in such a
   simple case we don't have to use `constructFromLexer`.  and that would
   be slightly more efficient as we don't have to copy the dummy names:
 {{{#!cpp
 MINFO::MINFO(MasterLexer& lexer, const Name* origin,
        MasterLoader::Options, MasterLoaderCallbacks&) :
     rmailbox_(createNameFromLexer(lexer, origin)),
     emailbox_(createNameFromLexer(lexer, origin))
 {}
 }}}
   I'd leave it to you, but if you make this change, we probably should
   simply incorporate `constructFromLexer` inside the from-text
   constructor as it's so simple.  Same for rp_17.cc.

 '''rdata_minfo_unittest.cc'''

 There are some missing cases:
 - omitting origin (okay with lexer if origin is given)
 - multi line
 ...hmm, I realized some of these cases are covered in
 `createFromLexer`, but I personally think we should deprecate this
 test case with the migration to the with-lexer constructor, and unify
 these cases into `createFromText` or `badText` tests.

 I think we should clarify this case is only for "from text":
 {{{#!cpp
     // number of fields (must be 2) is incorrect
     EXPECT_THROW(generic::MINFO("root.example.com. emailbox.example.com. "
                                 "example.com."),
                  InvalidRdataText);
 }}}

 '''rdata_rp_unittest.cc'''

 most (or maybe all) comments for MINFO tests apply here, too.

 '''rdata_tsig_unittest.cc'''

 - I made some minor suggested changes (commit 7482587).  Please check.
   (...btw did you intentionally change the scope of the unnamed
   namespace?  I reverted it in the commit, but at that point I didn't
   realize it was changed in this branch)
 - badText: depending on whether to allow non absolute names we might
   update the tests.
 - I'd note this is 'from text' only:
 {{{#!cpp
     // too many fields
     EXPECT_THROW(any::TSIG("foo 0 0 0 0 BADKEY 0 0"), InvalidRdataText);
 }}}
 - shouldn't we insert a space before the first '0'?
 {{{#!cpp
     checkFromText_TooLongLabel(string(too_long_label) + "0 0 0 0 BADKEY
 0");
 }}}
 - `badText`: for numeric error code and other len, we should probably
   check negative, too.
 - same comment for `createFromLexer` applies
 - we may also need some more cases such as multi-line input.

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