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      |
-------------------------------------+-------------------------------------
Changes (by zzchen_pku):

 * owner:  zzchen_pku => jinmei


Comment:

 Replying to [comment:5 jinmei]:
 > First off, I've fixed a couple of trivial things and pushed the
 > changes directly.
 Thanks.
 > '''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.
 Done.
 > - 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.
 Removed the constructor , will add it if we need it later.
 > - 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).
 Updated comments.
 > '''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.
 Done, added MINFO class and generated testdata file.
 > - 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.
 Okay:) updated.
 > - 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.
 Updated.
 > - "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).
 Done, added missed cases.
 > - 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.
 Updated.
 > '''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)
 Okay, I'll consider it next time.

 Hopefully I haven't missed anything, thanks for your review.

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