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:  vorner
  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 muks):

 * owner:  muks => vorner


Comment:

 Hi vorner

 Replying to [comment:10 vorner]:
 > 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?

 This was unintentional. It was used simply as a tiny temporary name, such
 as `i` for example. I'll change them to `r` or `rp`. :)

 > 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?

 I hope I follow what you are asking for correctly. I see two cases of it:

 * In this one, I think `setTTL()` is called eventually on the `RRset`
 passed to `Message::addRRset()`.
 {{{
 bin/auth/query.cc-        message.addRRset(section,
 bin/auth/query.cc:
 boost::const_pointer_cast<AbstractRRset>(rrset));
 bin/auth/query.cc-        added_.push_back(rrset.get());
 }}}

 * In this one, `RRset::addRRsig()` is not const and would modify the
 `covered_rrset`.
 {{{
 lib/datasrc/memory_datasrc.cc-        // cases.
 lib/datasrc/memory_datasrc.cc:
 boost::const_pointer_cast<AbstractRRset>(covered_rrset)->addRRsig(sig_rrset);
 lib/datasrc/memory_datasrc.cc-
 }}}

 >
 > 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));
 > }}}

 This was existing code from before #2165. This code has been replaced now
 with stripRRsigs(). :) I avoided making changes unless they affected
 something (caused tests to fail or resulted in incorrect behavior) so that
 side effects were not introduced by mistake.

 > 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?

 This is so that the files are present in the tarball for any downloader to
 view/use. In projects I have worked in, such extra stuff in the tree
 became a part of the tarball. As you see, it was added to `EXTRA_DIST` on
 purpose. The real question here is if the test files should be in the tree
 anymore. When it goes from the tree, it can go from the tarball too.

 > 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?

 `ZoneFinder` is probably not the best place for all of that code, but it
 checks the `options` argument against `FIND_DNSSEC` which is why it's
 there. Otherwise we'll have to check these inline in code and
 conditionally call `stripRRsigs()` as it doesn't make sense for
 `RRset::stripRRsigs()` to take a bool argument (whether to strip or not).
 A part of it can be moved to `RRset`, but if we want to use it optimally
 by not creating `RRsetPtr` unless required, it'll result in ugly API as
 `AbstractRRset::stripRRsigs()` could either return `(this)` or a new
 `RRset` depending on whether RRSIGs are present.

 The code has been moved to `zone_finder.cc`. It's used by users of
 datasrc, so unless it becomes a problem I think we can keep it as it is.

 > 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?

 Nice one. :) I added a test for it first, and then updated the code to not
 render the signatures if truncation would happen with the records firstly.

 > 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.

 API functions are properly documented in the header files.
 `RRset::toWire()` inherits from `AbstractRRset::toWire()` and says so in
 its API documentation. `AbstractRRset::toWire()` has detailed
 documentation. It returns the number of records rendered.

 > 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.
 > }}}

 This comment has been updated. :)

 > 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?

 This test doesn't make sense anymore as the RRSIGs are no longer "inline"
 in `Message`'s rrsets vector, and the test measures vector index
 distances. I couldn't quickly think of a way to modify it in place to get
 the same effect. It could mean writing fresh tests for such calls. I've
 created #2223 and added a TODO pointing to that bug in the comment. :)


 Replying to [comment:11 jinmei]:
 > I happen to notice one other thing.
 >
 > In `DatabaseClient::Finder::getRRsets` I believe you can simply
 > ignore any RRSIGs if sigs is false.  Also, if we have a simple test
 > method to tell if `RRsigStore` is empty, this part:
 >
 > {{{#!cpp
 >     if (sigs) {
 >         // Add signatures to all found RRsets
 >         for (std::map<RRType, RRsetPtr>::iterator i(result.begin());
 >              i != result.end(); ++ i) {
 >             sig_store.appendSignatures(i->second);
 >         }
 >     }
 > }}}
 >
 > can be like this:
 >
 > {{{#!cpp
 >     if (!sig_store.empty()) {
 >         // Add signatures to all found RRsets
 >         for (std::map<RRType, RRsetPtr>::iterator i(result.begin());
 >              i != result.end(); ++ i) {
 >             sig_store.appendSignatures(i->second);
 >         }
 >     }
 > }}}
 >
 > This works whether or not `sigs` is true, and in case the zone isn't
 > signed we can avoid iterating over the other types of RRsets for
 > nothing.

 This has been fixed too. :)

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