BIND 10 #2100: Update ZoneTable class

[BIND 10 Development]

#2100: Update ZoneTable class
-------------------------------------+-------------------------------------
                   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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 jelte]:

 Thanks for the review.

 > General:
 >
 > The unit tests didn't compile for me;
 > {{{
 > zone_table_unittest.cc: In member function ‘virtual void
 {anonymous}::ZoneTableTest_create_Test::TestBody()’:
 > zone_table_unittest.cc:83:16: error: variable ‘table’ set but not used
 [-Werror=unused-but-set-variable]
 > }}}
 >
 > I pushed a commit that removes that variable.

 Thanks for that - I noticed it in a later ticket and wondered why I
 used a separate variable.  Maybe it was a leftover of initial
 versions.

 > I also pushed a trivial commit that updates the copyright year in the
 new files to 2012 :)
 >
 > zone_table.h:
 >
 > class ZoneTable; IIRC, we had agreed that we'd use boost:noncopyable
 instead of making the copy constructor and assignment operators private.

 Good catches.  These look like because I began with a copy of the
 current zone_table implementation.  Fixed the latter too now.

 > For the rest it looks ok, though I did notice one mostly unrelated
 potential issue in labelsequence::serialize() which is triggered in the
 new unit tests (via resetLabels()); I think the memcpy there
 (labelsequence.cc:110) should be a memmove, as the regions are quite
 likely to overlap

 Hmm, good point too, and as I looked into it I realized it was more
 substantial than memcpy vs memmove.  If there's overwrap, it
 effectively breaks the integrity of the original object.  In our
 current usage it happens to be okay because we don't use the source
 `LabelSequence` after serialize() and the destructor (which is
 actually NO-OP) doesn't depend on the class integrity, but it's quite
 fragile.

 I think the right solution is to prohibit such an operation.  In the
 rbtree/domaintree implementation, we should make a safe copy first and
 then serialize the data from the copy.  It's a bit more expensive due
 to the additional copy, and the copy could be a waste as overriding
 serialize() is relatively rare operation, but I believe this overhead
 should be acceptable in the case of inserting a node.  And, in any
 case, breaking class integrity wouldn't be justified even for performance.

 So, I made these changes at commit 48ff206.  The change is not very
 big, but not straightforward either.  If you can check and approve
 it within the current ticket, that's fine, but since it's actually
 outside the scope of this particular task, I'm okay with discussing it
 in a new separate ticket.

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