BIND 10 #2850: Define and implement ZoneTableSegmentMapped

[BIND 10 Development]

#2850: Define and implement ZoneTableSegmentMapped
-------------------------------------+-------------------------------------
            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:  5             |                 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 muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:28 jinmei]:
 > I think we are almost there, but there are still some things to do...
 >
 > First, I suggest merging trac2836.  It's now ready for merge in
 > master, so the merge should be safe, and we'd probably like to use
 > some of the latest changes in that branch to address some remaining
 > issues for this branch.

 I created a `trac2850_4` branch rebased on top of `master` with
 `trac2836` merge.

 > '''zone_table_segment_mapped.cc'''
 >
 > - processHeader: With the latest version of #2836 (ready for merge),
 >   `ZoneTable::create` is now much more safe.  So I think we should
 >   make the cleanup now, not leaving it as "FIXME".  the `else` block
 >   should now be able to go as follows:
 > {{{#!cpp
 >         while (true) {
 >             try {
 >                 SegmentObjectHolder<ZoneTable, int> zt_holder(segment,
 0);
 >                 zt_holder.set(ZoneTable::create(segment, rrclass_));
 >                 void* ptr = segment.allocate(sizeof(ZoneTableHeader));
 >                 ZoneTableHeader* new_header = new(ptr)
 >                     ZoneTableHeader(zt_holder.release());
 >                 segment.setNamedAddress(ZONE_TABLE_HEADER_NAME,
 new_header);
 >                 break;
 >             } catch (const MemorySegmentGrown&) {}
 >         }
 > }}}
 >   To make it workable, however, we'd need one more small trick: extend
 >   `ZoneTable::destroy` with an additional dummy parameter:
 > {{{
 >     /// \param unused Ununsed parameter, provided so it's compatible to
 >     /// SegmentObjectHolder.
 >     static void destroy(util::MemorySegment& mem_sgmt, ZoneTable*
 ztable,
 >                         int unused = 0);
 > }}}

 These have been done.

 > - on second thought, I now think the idea of `resetHeader()` may not be
 >   a good idea, especially by seeing how it's used in `ZoneWriter`.
 >   It'd be quite easy to forget calling it or to call it incorrectly,
 >   and it's difficult to make it exception safe (if `load_action_()`
 >   throws `segment_` could refer to the wrong address for the header).
 >   So my revised suggestion is to reset/reget it in getHeader() if it's
 >   writable; otherwise we should be able to believe the cached value is
 >   always valid.  Then update `ZoneWriter` not to call `resetHeader()`.

 Thas code has been updated.

 > '''memory_segment.h'''
 >
 > - I think empty and reserved names should be rejected by all
 >   `xxxNamedAddress` methods (we don't like apps to allow clearing a
 >   reserved named address, for example).  So I'd unify a set of checks
 >   on invalid names in a separate helper method (can be static private)
 >   and call it from all the methods.  update the doc accordingly
 >   including the parameter description for `name` and when
 >   `InvalidParameter` is thrown.

 This has been done.

 > - regarding documentation on invalid names, "Note that" sounds
 >   awkward:
 > {{{#!cpp
 >     /// Note that names beginning with an underscore (such as
 >     /// \c "_example") are reserved for internal use by this class. If
 such
 > ...
 >     /// Note that empty names (\c "") are not allowed too. If an empty
 name
 > }}}
 >   as this is actually the only place the reader could know they are
 >   invalid.  It would be more sensible if we specify which names can be
 >   used by apps explicitly (just removing "Note that" may be enough).
 >   Also note that if we make the change of the previous bullet we'll
 >   need to refer to it from the description of all three methods.

 This has been done. As the validation method is private, I've also
 inlined its documentation in the three methods that use it.

 > '''memory_segment_mapped.h'''
 >
 > - it's awkward to see allocated_size_ is defined here while using
 >   pimpl.

 This was updated as suggested, but see below.

 > '''memory_segment_mapped.cc'''
 >
 > - I realized my initial proposal using `allocated_size_` was a bit
 >   naive.  For example, it could incorrectly return false from
 >   read-only segment or a read-write segment opened with some content.
 >   to address this, we'd need something like another special named
 >   address remembering the latest value of `allocated_size_`.  but to
 >   this end we might rather want fall back to clear-then-reset
 >   approach.  I think we could still say it reserves the const-ness
 >   property and exception-free ness (in the very worst case it should
 >   abort).

 This has been updated to the clear-then-reset approach.

 > - if we want to keep the idea of `allocated_size_`, I'd like to check
 >   underflow in `deallocate`.

 This is not necessary now.

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