BIND 10 #2107: redefine in-memory zone data

[BIND 10 Development]

#2107: redefine in-memory zone data
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 muks]:

 > I have not reviewed `ZoneData` completely yet, as once again #2165 took
 up time. But here are some initial review comments you can check
 meanwhile:

 Thanks for the review (and thanks for helping the review queue cleanup:-).
 I'm responding to the initial points while waiting for the full
 review.

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

 If you strongly prefer defining it as a (static) member function, I'm
 okay with that.

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

 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.

 > * MemorySegmentTest's API doc seems to be off by 1:
 > {{{
 > // A special memory segment that can be used for tests.  It normally
 behaves
 > // like a "local" memory segment.  If "throw count" is set to non 0 via
 > // setThrowCount(), it continues the normal behavior up to the specified
 > // number of calls to allocate(), and throws an exception at the next
 call.
 > }}}
 >
 > Say we call `setThrowCount(2)`. `allocate()` will then work once before
 > throwing.

 Ah, right.  Corrected.

 > Also, after that shouldn't it continue throwing always? It's

 That's one possibility, but in the tests it doesn't matter much
 because we always reset the counter anyway.  For now, I keep the
 current behavior, clarifying the behavior.  We can change it if a
 different behavior is more convenient for some other test cases.

 > probably overkill, but you can be sure of the behavior by adding
 > unittests for it too.

 This stuff is only for tests, and actually used in tests (though
 implicitly), so I'd keep it that way for now.

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

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

 > * There's a use after free in `rdataSetDeleter()`:
 > {{{
 >  rdataSetDeleter(RRClass rrclass, util::MemorySegment* mem_sgmt,
 >                  RdataSet* rdataset_head)
 >  {
 > -    for (RdataSet* rdataset = rdataset_head;
 > +    for (RdataSet* next, *rdataset = rdataset_head;
 >           rdataset != NULL;
 > -         rdataset = rdataset->getNext()) {
 > +         rdataset = next) {
 > +        next = rdataset->getNext();
 >          RdataSet::destroy(*mem_sgmt, rrclass, rdataset);
 >      }
 >  }
 > }}}

 Ouch, thanks for catching it.  Fixed.

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

 > * I have pushed some minor comment updates to the branch.

 That's looks good, thanks.

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

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