BIND 10 #2292: eliminate const_cast from domaintree.h

[BIND 10 Development]

#2292: eliminate const_cast from domaintree.h
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20121009
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  data   |           Sub-Project:  DNS
  source                             |  Estimated Difficulty:  4
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 vorner]:

 > > Actually, I thought this was misuse of the read-only interface and
 > > wanted to correct at this opportunity.  If we want to possibly modify
 > > a found node, we should use insert().
 >
 > Actually, I didn't know such a wide scope was meant by the ticket (so I
 didn't
 > even look how much the method is used). It seems to be possible when
 changing
 > stuff that far away.

 Yeah I admit the ticket description wasn't clear.  My bad.  But I like
 the revised version very much, overall:-)

 > I hope the current changes are not problematic (I removed the add method
 of the
 > in-memory data source). Also, the history isn't really clean now (the
 template
 > parameters were introduced and later removed in this branch). Should I
 try to
 > rebuild the history in some cleaner way? It shouldn't be that much work
 I
 > think, if we care about how the history looks like.

 I personally don't care about the history, since in the end the entire
 diff for the branch isn't that big.  Regarding the add() method I
 guess we need some discussions.  See below.

 '''general'''

 I made a few minor cleanups and pushed them.

 '''zone_table.h/cc'''

 - `ZoneTable::addZone()`: I suggest passing `ZoneData*` instead of
   an object holder:
 {{{#!cpp
 ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
                    const Name& zone_name, ZoneData* zone_data)
 }}}
   (`InMemoryClientImpl::load()` should release the zone data from the
   holder at the call to addZone()).  I intended SegmentObjectHolder()
   to be pretty local, like boost::scoped_ptr although it has
   `release()`, and didn't expect it to appear in inter-class
   interfaces.  (maybe we should make it a derived class of
   boost::noncopyable).  I also think it's clearer that the ownership
   of the zone data has been passed to the `ZoneTable` if we explicitly
   release() it at the time of call to `addZone()`.  This means
   `ZoneTable::addZone()` stores the passed pointer to a separate
   holder, which could be considered a bit of waste, but I think the
   construction/destruction of the holder is negligible and acceptable.
 - `ZoneTable::addZone()`: we probably want to keep something like
   `AddResult` and return the previous zone data, if any.  As we
   implement background zone loading, we'll need to separate the
   "build", "swap", and "release" steps of zone data so that only the
   "swap" step has to be in a critical section.

 '''domaintree_unittest'''

 - This comment doesn't seem to be very correct:
 {{{#!cpp
     // the child/parent nodes shouldn't "inherit" the callback flag.
     // "cdtnode" may be invalid due to the insertion, so we need to re-
 find
     // it.
 }}}
   The pair of "cdtnode" and "insertion" sounds awkward:-), and I think
   regarding the comment "dtnode" was actually correct.  To clarify
   the point, we might say:
 {{{#!cpp
     // the child/parent nodes shouldn't "inherit" the callback flag.
     // "dtnode" may have been invalidated due to the insertion, so we
     // need to re-find it.
 }}}
   ...Hmm, and, actually, our latest implementation should ensure
   "dtnode" should still be valid in this scenario.  So I think it's
   okay to skip this find and just do
 {{{#!cpp
     EXPECT_TRUE(dtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK));
 }}}
   or check it explicitly just in case:
 {{{#!cpp
     // the child/parent nodes shouldn't "inherit" the callback flag.
     // "dtnode" should still validly point to "callback.example", but we
     // explicitly confirm it.
     EXPECT_EQ(TestDomainTree::EXACTMATCH,
 dtree.find(Name("callback.example"),
                                                      &cdtnode));
     ASSERT_EQ(dtnode, cdtnode);
 }}}

 '''memory_client_unittest'''

 I'm afraid we cannot simply remove all of these tests.  Things like
 loadRRSIGsRdataMixedCoveredTypes() or addOutOfZoneThrows() still seem
 to be necessary (or at least they cannot be removed due to this
 ticket).  Also, we need *some* interface to add a particular RRset to
 an existing zone anyway in order to support incremental updates after
 ixfr or DDNS.

 #2268 revised the load/add interface quite substantially, so maybe we
 merge this branch with #2268, and update some of these tests using the
 revised add interface while keeping the constness of the domain tree
 interface?

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