BIND 10 #2836: update in-memory zone loading so it can work with shmem segment

[BIND 10 Development]

#2836: update in-memory zone loading so it can work with shmem segment
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130528
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:18 jinmei]:
 > Part of #2850 has been merged to master.  I believe all that this
 > branch needs are now there, and won't require future changes due to
 > the rest of #2850 discussions.  That is, you should now be able to
 > merge/incorporate it for this branch safely.

 I did a merge with master as the first commit today. It's large and with
 some conflicts, but it contains mostly just updates to the change of
 interface.

 > Ah, okay.  Actually scoped pointer is even non copyable.  So it can't
 > be used as a return value of a function.  In this test usage, however,
 > `SegmentCreator::create()` seems to be able to just return the result
 > of `new` and have the caller store it in a scoped_ptr.  But that's not
 > a big deal and mostly a matter of taste.

 I just feel calmer when I don't see raw pointers being returned, as it is
 not always clear who should delete it if anybody.

 > > I also pushed the test that crashes for me. Does it crash for you too?
 Does valgrind work for you? If so, could you run it through it?
 >
 > That test crashed in my environment, too (though in a different place
 > of the code).  I've found one issue and committed a fix to that (my)
 > problem at 575854a507052ad7de51aa259fd5d8f04156d15d on branch
 > trac2836-2.  Hopefully it will fix yours, too.  Note that we may or
 > may not use the 5758... fix as it is (see below).

 I incorporated changes inspired by yours, and the separate set as well.
 The test now passes, so I hope it works as it should. But I'm still not
 100% sure there's no other bug like this.

 >   An incomplete insert() (due to exception in the middle of it) could
 >   cause a new empty node as a result of `nodeFission()`, which will
 >   now be found by find() if the domain tree was constructed
 >   accordingly.  I suggest combining and updating several related
 >   paragraphs as follows:

 ACK.

 > - maybe we'd note getNextHolderName() is not thread safe (but should
 >   be okay as a memory segment isn't expected to be modified by multiple
 >   threads at the same time in the first place)

 Actually, it's little bit worse, because the constructor of the holder is
 not reentrant this way. I don't know if it is a problem. If it is, we may
 want to either think of better way or create some kind of locking.

 > - depending on how we revise `SegmentObjectHolder`, `result.zone_data`
 >   could also be relocated on return of `addZone`.

 I don't see how that could happen or how to protect against that. Can you
 explain where it would relocate?

 > '''zone_finder_unittest.cc'''
 > - what's the purpose of changing `updater_` to (scoped) pointer?

 So I can release it during a test. As a result of the data being stored in
 the updater for whole lifetime, it is not possible to have two updaters at
 once and there's actually a check for it. And some tests load a zone from
 file or somewhere, which creates its own updater. So I need to remove the
 one created by the test fixture.

 IMO this should be all the functionality for the branch. Or I hope so.
 Many thanks for pointing me to the problem of the test.

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