BIND 10 #2107: redefine in-memory zone data

[BIND 10 Development]

#2107: redefine in-memory zone data
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            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:10 jinmei]:
 > > * Why is `findRdataSetOfType()` not a member of `RdataSet`?
 >
 > As a static member function?  At least it cannot be a non static
 > member because it has to handle the case where 'this' is NULL.
 > Regarding whether it should be a static member function or non member
 > function, it's just my personal preference: I generally prefer making
 > a function non member of a class unless it needs to refer to private
 > information of the class in order to minimize things that depend on
 > the class internal.  On the other hand, it could be considered part of
 > the "group" of `RdataSet`, so I see it may be better to define the
 > function in the `RdataSet` namespace (which happens to be a class in
 > this case).

 I see that you want to handle the NULL case too now. Regarding making it a
 static member function, I think it would still be good, and you can reduce
 its name to just `findOfType()`.

 > > * There should be unittests for `SegmentObjectHolder`,
 > >   `RdataSet.getNext()`, etc. to statically enforce the API and also
 > >   check the implementation.
 >
 > I'm not sure what 'statically enforce the API', but I added tests for
 > them.

 By this, I meant to check the API signature. :) A small unit test that
 calls the method would fail to compile if the signature of the method
 changes in an incompatible way.

 > As for getNext(), I realized the existing version should rather return
 > `const RdataSet*` and a separate mutable version should be provided
 > (compilers wouldn't complain, but returning a non const pointer from
 > the const member function essentially breaks the spirit of constness).
 > So I did that too.

 Nod.

 > > * glibc complains that the `memcpy()` in `LabelSequence::serialize()`
 > >   gets overlapping regions when `DomainTreeTest.nodeCount`
 > >   is run. Perhaps we should use `memmove()` or find out why
 overlapping
 > >   regions are passed.
 >
 > I believe it's a different issue, and was fixed in #2100 (already in
 > master).  The fix is at commit 48ff206.

 Nod.

 > > * In `RdataSerializationTest.readerResult`, `~Name::Name()` is called
 > >   immediately when this statement is executed:
 > > {{{
 > >     LabelSequence seq(Name("example.org"));
 > > }}}
 >
 > Right, it was an issue of #2096, I pointed that out there, and it was
 > fixed.  I'm not sure if I can cleanly merge that particular fix to
 > this branch, so unless it causes severe disruption, I'd leave it open
 > in this branch.  When this branch is ready for merge, #2096 should
 > have been merged with the fix.

 Nod.

 > > * `DomainTree::setData()` doesn't free the old data now, as can be
 seen in `DomainTreeTest.setGetData`. I think vorner, you and me reviewed
 this in a previous bug. We should follow some kind of consistent
 ownership. If `DomainTree` is supposed to own the tree data, `setData()`
 should have the responsibility to free it. If `DomainTree` doesn't own the
 data, then we should update `deleteHelper()` and other such methods to not
 delete the data.
 >
 > Actually, we (in this branch) have clearly switched to the model where
 > the application (not `DomainTree`) has the ownership of the data, by
 > enforcing it to pass a deleter to `DomainTree::destroy()`.  While it
 > might still look a kind of hybrid model in that the helper method
 > still needs to call the deleter explicitly, I think the ownership
 > policy is consistent API-wise.  I tried to clarify it in the
 > documentation at commit 4bd4bf9.
 >
 > Which model of ownership is better itself may be of a discussion, but
 > as explained in the review-request comment, the revised model seemed
 > to be the only practical choice in the case of `ZoneData`.

 Okay, in this case can we change `setData()` then so that it returns the
 old pointer, similar to `ZoneData::setNSEC3Data()`? It'll be useful for
 code like this too (though in this case, `next` is updated after the
 `setData()` which is bad):
 {{{
     rdataset_aaaa->next = node->setData(rdataset_aaaa);
 }}}

 > > * Can we not merge `DomainTree` and `RBTree` by working out the small
 > >   differences, as they are largely similar?
 >
 > What do you mean by merge?  The plan (in my understanding) is that
 > we'll deprecate the current `RBTree` in #2219, and I don't think we
 > need to heavily modify both implementations (because of a critical bug
 > fix like 48ff206) until then.

 By merge, I meant unify them into one class so we don't have to modify two
 red-black tree implementations. If we're going to completely deprecate
 `RBTree`, then that's fine.

 --------------------------------------------------------------------------

 Here's the rest of my review:

 * A copy of `getNext()` seems to have been left in as a comment:
 {{{
      /// get the same result by directly calling get() on \c next, it
 would
      /// help encourage the use of more efficient usage if we provide an
      /// explicit accessor.
 -    //const RdataSet* getNext() const { return (next.get()); }
      const RdataSet* getNext() const { return (next.get()); }
 }}}

 * I think `WILDCARD_NODE` is a better naming (sounds better) than
 `WILD_NODE`. Can we change that flag name in `ZoneData`?

 * Is it better to make the defintion of `ZoneData::isNSEC3Signed()` to:
 {{{
     bool isNSEC3Signed() const { return (isSigned() && nsec3_data_); }
 }}}

 * Can it be further clarified how the tree can be modifed (the first
 sentence here in `NSEC3Data`'s doc). What does "specific method" mean?
 {{{
 /// Modifying the tree must be done by specific method; the application
 /// cannot directly change the content of the tree in an arbitrary way.
 }}}

 * This is a trivial nitpick, but in `ZoneData::destroy()` is it better to
 destroy the NSEC3 tree first before the `ZoneData` tree?

 * Should `\return` be added to some of these `ZoneData` API doc for
 methods?

 * I think it's better to put the const and non-const versions of
 `ZoneData::getNSEC3Data()` next to each other in `zone_data.h`.

 * What is the main purpose of `ZoneDataTest.throwOnCreate` test? If it is
 to check that leaks don't occur, it should be renamed to say that.

 * `WILD_NODE` seems to be untested and unused for now.

 * In `ZoneDataTest.getSetNSEC3Data`, should we also check for the
 following (enforce it?):
 {{{
      EXPECT_EQ(nsec3_data, zone_data_->getNSEC3Data());
 +    EXPECT_TRUE(zone_data_->isSigned());
      EXPECT_TRUE(zone_data_->isNSEC3Signed());
 }}}

 * I have pushed some more comment updates and some minor code style
 changes to the branch.

 The rest of the stuff seems fine. :)

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