BIND 10 #2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version

[BIND 10 Development]

#2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           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 => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:15 jinmei]:
 > Replying to [comment:12 jinmei]:
 >
 > The revised version looks pretty good.  I have a few more, relatively
 > minor things to discuss though.
 >
 > '''domaintree.h'''
 >
 > - A minor point: I don't think DomainTreeNodeChain::level_count_ has
 >   to be uint32_t because the structure size concern doesn't apply
 >   here.

 Changed back to `size_t`. :)

 > - I'd note largestNode() never throws.

 Added.

 > - I intended to suggest making some comments about why we use uint32_t
 >   for node_count_, but the revised code doesn't have it.  I've added
 >   my suggested text at commit 40d4887.

 Looks good.

 > - I have sympathy with keeping the now unused getLargestInSubTree() and
 >   its tests (within if 0), but I'd like to clean them up unless we know
 >   we need it soon (and I don't think that's the case).  If we really
 >   worry about the risk of reimplementing it from the scratch if and
 >   when we see the need for it, I'd just leave one line comment that we
 >   previously implemented it experimentally and removed it as we didn't
 >   see the need for it at that time.

 Removed.

 >
 > '''memory_client.cc'''
 >
 > - what's the purpose of the changes at
 >   e05f1d87c3764033b920fa8a227cfd05dc3ca3b8?
 > {{{
 >     [2218] Call setSigned(true) when NSEC3Data is set
 > }}}
 >   The commit log doesn't explain the reason, and no test fails even
 >   without the changes.  It's probably more about the semantics of "is
 >   signed" in general - from a quick look, we don't seem to have a
 consistent
 >   interpretation between the zone finder, (document on possible
 >   implementation of) zone data, and your implementation of the memory
 >   client.  Maybe it's a subject of a separate ticket.  But in any
 >   event, if you want to introduce a change like this (that changes the
 >   state of the data), I think there should be some corresponding test
 case that
 >   would fail without the change.  So we must either provide a test or
 >   defer the change to the later ticket.

 From a reading of ZoneData's api doc, I thought `setSigned()` would also
 be called with `true` whenever NSEC3 data existed. I've added tests to
 assert this.

 > - is there a test for this?
 > {{{
 > +            if ((salt_len > 0) &&
 >                  (std::memcmp(&salt_data_2[0], salt_data, salt_len) !=
 0)) {
 >                  isc_throw(AddError,
 >                            "NSEC3 with inconsistent parameters: " <<
 > ...
 > +                    if ((salt_len > 0) &&
 > }}}
 >   the code is obviously correct, but in general it's better to confirm
 >   it by tests.

 Added.

 > '''memory_client.h, cc'''
 > - these comments seem to have to be updated:
 > {{{#!cpp
 >     /// This derived version of the method never throws an exception.
 >     /// For other details see \c DataSourceClient::findZone().
 > }}}
 >   at least the first line isn't correct (it's not "derived" any
 >   more).  I'm also not sure if the second line makes sense.

 Updated.

 > - I'd suggest changing the return type of findZoneData() to
 >   `const ZoneData*`.  That's sufficient for the tests (which seem to
 >   be the only use case for it), and will help when we eventually work
 >   on #2292 (where we should also change zone_data to a const pointer).
 >   I'm attaching a diff to implement it.

 The patch looks good. I've committed it and also fixed some more new tests
 for it.

 > - memory_client.h shouldn't rely on datasrc/zonetable.h, which will be
 >   gone when we complete the migration (the diff in the previous bullet
 >   makes this cleanup).

 Nod.

 >
 > '''domaintree_unittest.cc'''
 > - largestNode test should at least include a test case for an empty
 >   tree.  I guess there could be some more test cases ideally, but I
 >   don't have a specific idea, so it's probably okay with just adding
 >   the empty case.

 Added.

 >
 > '''zone_finder_unittest.cc'''
 >
 > - does addZoneData() need everything for the NSEC3 case?  It seems to
 >   be almost a duplicate of the main code, which is not generally good.
 >   For example, if the test data aren't expected to contain
 >   inconsistent NSEC3 param, it's probably better just to skip the
 >   check.

 Let's keep this for now as the worst thing it does is check more. :) I
 want to finish up the other loose ends and merge this.

 > '''zone_finder.cc'''
 >
 > I have a few more suggestions
 > - InMemoryZoneFinder::getOrigin() is expensive and should better be
 >   avoided whenever possible.  It looks like possible to use the label
 >   sequence of the origin node and use LabelSequence based interfaces
 >   in both cases.

 Done. I've also made it use the previous-chain relative `find()` which was
 added in a recent bug to search relative to the origin node.

 > - it should be possible to define salt and hash outside of the loop
 > {{{#!cpp
 >     for (unsigned int labels = qlabels; labels >= olabels; --labels) {
 >         const std::vector<uint8_t> salt(nsec3_data->getSaltData(),
 >                                         (nsec3_data->getSaltData() +
 >                                          nsec3_data->getSaltLen()));
 >         const std::auto_ptr<NSEC3Hash> hash
 >             (NSEC3Hash::create(nsec3_data->hashalg,
 >                                nsec3_data->iterations,
 >                                salt));
 > }}}
 > - If we adopt the suggested interface for NSEC3Hash::create (below),
 >   we could omit the construction of salt:
 > {{{#!cpp
 >         const std::auto_ptr<NSEC3Hash> hash
 >             (NSEC3Hash::create(nsec3_data->hashalg,
 >                                nsec3_data->iterations,
 >                              nsec3_data->getSaltData(),
 >                              nsec3_data->getSaltDataLen()));
 > }}}

 Done.

 > - boost::scoped_ptr would be slightly better than auto_ptr because we
 >   don't have to copy hash.

 Done.

 >
 > '''nsec3hash.h,cc'''
 >
 > - A minor, and possibly controversial point: for the new signature of
 >   factory, it might be better to take `const uint8_t*` and `size_t`
 >   instead of `vector<uint8_t>`, considering this is a relatively lower
 >   level primitive when performance is important.  If we require a
 >   vector, the caller needs to prepare for it, which might be expensive
 >   depending on the context (see above).
 >

 It's been changed now. `NSEC3Hash` was internally refactored for it.

 > - I'd describe the parameters more specifically for this
 > {{{#!cpp
 >     /// \brief Factory method of NSECHash from args.
 >     ///
 >     /// This is similar to the other version, but uses the arguments
 >     /// passed as the parameters for hash calculation.
 >     static NSEC3Hash* create(uint8_t algorithm, uint16_t iterations,
 >                            const std::vector<uint8_t>& salt);
 > }}}
 >   because it's not as obvious as the other two cases.

 Done.

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