BIND 10 #2165: update Message::addRRset() to be unaware of signedness

[BIND 10 Development]

#2165: update Message::addRRset() to be unaware of signedness
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hello

 I'll answer both tickets here, I reviewed it as one diff.

 Regarding style, you use `rr` as the name for an RRset in many places.
 Isn't that misleading little bit? RR usually means single Resource Record.
 Or is it really so the RRset contains only one?

 It is not related to this ticket much, but it reminded me of the problem.
 There are many calls like this:
 {{{#!c++
 boost::const_pointer_cast<AbstractRRset>(rrset));
 }}}

 This is bad, but there was some problem with some method of rrset not
 being const even if it was a getter method and there was some technical
 reason for it inside the implementation. I think it was related to the
 signatures. Would you know by any chance if it is still a problem?

 In this code, it seems to me if the RRset has no RRSIG, it is still
 „stripped“. That is probably a needless work (query unittests, somewhere
 below line 711):
 {{{#!c++
 if (found_rrset != found_domain->second.end()) {
     ConstRRsetPtr rrset;
     // Strip whatever signature there is in case DNSSEC is not required
     // Just to make sure the Query asks for it when it is needed
     if ((options & ZoneFinder::FIND_DNSSEC) != 0 &&
         found_rrset->second->getRRsig()) {
         rrset = found_rrset->second;
     } else {
         RRsetPtr noconst(new RRset(found_rrset->second->getName(),
                                    found_rrset->second->getClass(),
                                    found_rrset->second->getType(),
                                    found_rrset->second->getTTL()));
         for (RdataIteratorPtr
              i(found_rrset->second->getRdataIterator());
              !i->isLast(); i->next()) {
             noconst->addRdata(i->getCurrent());
         }
         rrset = noconst;
     }
     return (createContext(options, SUCCESS, rrset));
 }}}

 You disabled the tests for the old API. If I leave aside the fact we still
 have the API in our tree, even if it is not used (I think it is not used),
 why are the files listed in EXTRA_DIST? If the test files are not
 compiled, is there a reason to include them in the tarball?

 The `stripRRsigs` function, should the whole (nontrivial) implementation
 be in header file? And, is the ZoneFinder the best place for it? Doesn't
 the function work on any RRset?

 Looking at the code of `toWire(AbstractMessageRenderer, …)`, can it happen
 that the real RR is not rendered, because it does not fit (64k long TXT
 record, for example), but then the signatures fit, so they are included?
 If so, is it OK?

 Both toWire methods would probably deserve a description about what is
 being returned. If I didn't read the code, I'd ask if it was number of
 bytes, number of records or some status code.

 This comment isn't true any more (or, it is probably true in some sense,
 but still very misleading) ‒ the removed one lowers the count by 2, not 1:
 {{{#!c++
 // Locate the AAAA RRset and remove it; this has one RR in it.
 }}}

 Regarding this:
 {{{#!c++
 #if (0)
         // Disabled by #2165. The RRSIG RRsets are no longer directly
         // stored in the Message's rrsets, so the iterator will not find
         // them. The expected text used in many tests are flattened,
         // where the RRSIGs are inline. In other words, RRSIGs may occur
         // between (expected_begin, expected_end) but not between
         // (actual_begin, actual_end).

         // make sure rrsets only contains expected RRsets
         EXPECT_EQ(std::distance(expected_begin, expected_end),
                   std::distance(actual_begin, actual_end));
 #endif
 }}}

 First, I don't like to see any code commented out without any FIXME or
 TODO mark besides it (these allow for easy grepping for problems).

 However, the more serious concern is, when do we fix the test to test what
 it did before? Is there a ticket for it? Will it be deprecated? If not,
 maybe rendering it to text, sorting both texts by lines and comparing as
 strings? Would that work? Or, we need to see they are the same size, so
 actually comparing only the lengths of the strings?

 Thank you

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