BIND 10 #1113: RR type implementation: MINFO

[BIND 10 Development]

#1113: RR type implementation: MINFO
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:  minor  |  Sprint-20110816
                  Component:         |            Resolution:
  libdns++                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 First off, I've fixed a couple of trivial things and pushed the
 changes directly.

 '''implementation'''
 - I suggest using \exception for exceptions throughout the
   implementation.  From a quick look one of the constructors and
   toWire() will have to be updated.
 - this is not correct:
 {{{
 +    /// We use the default copy constructor and assignment operator.
 }}}
   Because the copy constructor is actually defined explicitly.  Even
   if it's true the position of the documentation doesn't seem to be
   correct (did you check the doxygen output?).  As for the assignment
   operator, you'll need to define it (if you need it) because by
   default it's inherited from the base class, which is private.
   Further, these are not tested in the unittests, so the discrepancy
   isn't properly caught.  If this is because this branch wasn't
   developed test driven (I'm saying this because this type of
   uncovered test cannot happen in theory in TDD) please revisit the
   way of developing things.

 - The "from parameter" constructor isn't tested.  Again, this
   type of thing shouldn't happen if it was developed test-driven.
   BTW, I'm not sure if we want to have this constructor at least at
   the moment.

 - IMO, we don't need '%' in the documentation of for the "from
   parameter" constructor:
 {{{
     /// The parameters are a straightforward mapping of %MINFO RDATA
 }}}

 - toWire(): is this correct?
 {{{
 /// As specified in RFC1035, the rmailbox and emailbox fields (domain
 names)
 /// will be compressed.
 }}}
   where in RFC1035 is it specified? (I have no doubt these should be
   compressed though).

 '''unittest'''
 - now that we've merged #904, I strongly suggest extending gen_wiredata
   (in the latest master it's in lib/util/python) to support MINFO and
   using it for generating wire-format test data.  hardcoding all hex
   like:
 {{{
 const uint8_t compressed_wiredata_minfo[] = {
     0x04, 0x72, 0x6f, 0x6f, 0x74, 0x07, 0x65, 0x78, 0x61, 0x6d, 0x70,
     ...
 }}}
   is difficult to create or maintain and is not human readable.

 - not a big deal, but there's inconsistency between rmailbox vs
   rmailbx (with or without 'o') in comment lines and test strings.
   same for emailbox.

 - I'd avoid using namespace level statically-initialized objects:
 {{{
 string minfo_txt("root.example.com. emailbx.example.com.");
 string minfo_txt2("rmailbx.example.com. emailbx.example.com.");
 string too_long_label("012345678901234567890123456789"
     "0123456789012345678901234567890123");
 ...
 const generic::MINFO rdata_minfo(minfo_txt);
 const generic::MINFO rdata_minfo2(minfo_txt2);
 }}}
   due to the possibility of causing initialization fiasco.
 - "from text" test: what if it has three fields?
 - "from text" test: the case where the rmailbox is bogus should also
   be tested.
 - I'd prefer RRType::MINFO over RRType("MINFO") as the former should
   be faster.  Same for RRClass.  (for the RRType we may want to use
   both so that we can implicitly test them).

 - In createFromWire test, it's not obvious why we use test data file named
   "cname" for testing MINFO:
 {{{
     EXPECT_THROW(rdataFactoryFromFile(RRType("MINFO"), RRClass("IN"),
                                       "rdata_cname_fromWire", 48),
 }}}
   we need a comment about why it's here.

 '''other'''
 As we talked at the biweekly call, please consider picking up review
 before proceeding to another RDATA task (you seem to have picked up
 #1114 while, e.g, #1067 was in the review queue at that time)

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