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

Comment (by 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.

 '''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);
 }}}

 - 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()`.

 '''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.
 - 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.

 '''memory_segment_mapped.h'''

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

 '''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).
 - if we want to keep the idea of `allocated_size_`, I'd like to check
   underflow in `deallocate`.

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