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

 Hi Jinmei

 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:

 * Why is `findRdataSetOfType()` not a member of `RdataSet`?

 * There should be unittests for `SegmentObjectHolder`,
   `RdataSet.getNext()`, etc. to statically enforce the API and also
   check the implementation.

 * 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. Also, after that shouldn't it continue throwing always? It's
 probably overkill, but you can be sure of the behavior by adding
 unittests for it too.

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

 * In `RdataSerializationTest.readerResult`, `~Name::Name()` is called
   immediately when this statement is executed:
 {{{
     LabelSequence seq(Name("example.org"));
 }}}

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

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

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

 * Can we not merge `DomainTree` and `RBTree` by working out the small
   differences, as they are largely similar?

 I'll comment again when the rest of the review is done.

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