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-20130514
         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

 The branch is still not finished, but I'd appreciate some comments and
 input.

 Replying to [comment:7 jinmei]:
 > - First is `ZoneData::create()`.  Any of `ZoneTree::create`,
 >   `ZoneTree::insert`, and direct `MemorySegment::allocate()` can
 >   result in `MemorySegmentGrown`.  In this case it's probably better
 >   to catch the exception and retry in `ZoneData::create()` (and
 >   document that this factory doesn't throw `MemorySegmentGrown`)
 > - Regarding this, we need some additional tricks for general exception
 >   safety: `SegmentObjectHolder` cannot simply call destroy() with the
 >   stored `obj_`, because it may be remapped by the time of the call to
 >   destroy().
 > - `loadZoneDataInternal` also has to be aware that the address
 >   returned by `holder.get()` might be remapped after the completion of
 >   load.

 I'm trying to address them using the suggestions below. But I'm not having
 too much luck with that.

 > - not critical, but is there a reason for extracting `addInternal` as
 >   a separate method?  It's pretty small and only called from add().

 I don't know, it just seemed more readable this way to me. If you like, I
 can put it back.

 > - whether it's in add() or the constructor, there's a very slight
 >   chance that `setNamedAddress()` itself causes address remapping.
 >   In #2850 we are discussing making it safer/more convenient.
 >   You may want to use the result (and in any case you may have to
 >   update this branch if it causes interface change), or otherwise
 >   you'll have to call `getNamedAddress()` immediately after
 >   `setNamedAddress()` to make sure the address is really the latest
 >   one.

 OK, once the branch is in master, I'll go through it and see what needs to
 be updated. I'd like to avoid too many merges now.

 > - Do we really need to expose zone_data_ as a protected member
 >   variable?  See below for the relevant discussion, but even if we
 >   really need to allow a derived class to access/modify `ZoneData`
 >   directly, I'd like to protect integrity as much as possible.  That
 >   is, unless we need to allow derived classes to modify zone_data_
 >   (the pointer value itself, not the zone data it points to), I'd only
 >   allow them to access it via an accessor:

 Changed, as by the suggestion below.

 > - according to the code it should be possible to use the parameter
 >   type for ZoneDataUpdaterTest as const:
 > {{{#!cpp
 > class ZoneDataUpdaterTest : public ::testing::TestWithParam<const
 SegmentCreator*> {
 > }}}
 >   Although it somehow didn't compile in my environment.

 It seemed to me that the gtest actually passes it as const, that's why it
 wouldn't compile for you as double-const I guess.

 >   and we shouldn't need cast
 > {{{#!cpp
 > INSTANTIATE_TEST_CASE_P(TestSegment, ZoneDataUpdaterTest,
 >                         ::testing::Values(static_cast<SegmentCreator*>(
 >                             &test_segment_creator)));
 > }}}
 >   But these don't seem to work in my environment either.

 The cast is needed. It seems the `::testing::Values` is some templated
 thing and it decides what templated type it should use by the values
 passed. So if I didn't have the static_cast, it used wrong type of
 template parameter and it didn't match the one wanted by the test suite.

 > - `ZoneDataUpdaterTest`: is there a reason `mem_sgmt_` is a shared
 >   pointer, not scoped?

 The reason is, I pass it as a return value from the creator and copy it.
 Wouldn't a scoped pointer destroy it once the returned instance was
 destroyed?

 > - `MappedSegmentCreator::cleanup`:
 `boost::interprocess::file_mapping::remove`
 >   is probably better than unlink.  It would be more portable (at least
 >   to the extent of the theory that Boost is expected to be portable),
 >   and it also seems to have some consideration on mmap specifics (and
 >   so safer).

 I can't seem to find the function in the documentation. But I found this:

 {{{
 To remove the file from the filesystem you can use standard C std::remove
 or Boost.Filesystem's remove() functions.
 }}}

 So, unlink seems to be the same kind of function as they propose.

 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?

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