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:

 Replying to [comment:31 jinmei]:
 > '''zone_table_segment_mapped.cc'''
 >
 > - `getHeaderHelper()`: I would skip `getNamedAddress` for non writable
 >   segments.  (I thought I suggested it, but it looks like not that
 >   clear)  Otherwise we need to do the expensive operation for every
 >   query lookup.

 This has been done now.

 > '''memory_segment.h'''
 >
 > - I'd avoid repeating the same text about valid names for all methods.

 This is done.

 > - Likewise, I'd simply say e.g. "name is invalid (see above which
 >   names are invalid)" than repeating specific cases:
 > {{{#!cpp
 >     /// \throw InvalidParameter name is NULL, empty ("") or begins with
 >     /// an underscore ('_').
 > }}}

 I just made them refer to the private method's doc. We use this
 internally anyway.

 > - very minor point, this organization looks a bit awkward:
 > {{{#!cpp
 >         if (!name) {
 >             isc_throw(InvalidParameter, "NULL is invalid for a name.");
 >         }
 >         if (*name == '\0') {
 >             isc_throw(InvalidParameter, "Empty names are invalid.");
 >         } else if (*name == '_') {
 >             isc_throw(InvalidParameter,
 >                       "Names beginning with '_' are reserved for "
 >                       "internal use only.");
 >         }
 > }}}
 >   why is the only one `else if`?

 This has been updated.

 > '''memory_segment_mapped.cc'''
 >
 > - `allMemoryDeallocated`: I'd note in comment that reserveMemory
 >   should succeed as we've already allocated the space.

 Added.

 > - I'd make sure memory relocation doesn't happen in `reserveMemory` if
 >   called from `allMemoryDeallocated` (it shouldn't happen for the same
 >   reason as above).  passing an argument to prevent looping, etc.
 >   note also that if we abort there we shouldn't need try-catch in
 >   `allMemoryDeallocated`; no other part within the try block can
 >   throw.

 There is a more basic question that I have. In case of read-only
 segments, with our current design `allMemoryDeallocated()` will not
 work.

 > '''zone_writer.cc'''
 >
 > while almost out of scope for this ticket, there's still one more
 > glitch in `ZoneWriter::install`: `zone_data_` can be relocated on
 > `MemorySegmentGrown`.  Further, in general, we should protect
 > `zone_data_` immediately after the call to `load_action_`.  This will
 > be beyond a trivial change, and I don't want to delay this task even
 > further, but at the same time I don't want to leave the flaw longer.
 >
 > So I'm attaching a proposed patch to address this point.  In some
 > sense it only does some straightforward thing: maintain a holder
 > within the writer and capsule zone_data in the holder as long as
 > necessary.  But I wanted to keep the details (especially the holder
 > class) hidden in .cc so I moved local members into pimpl struct.  So
 > the size of the change is a bit larger.
 >
 > If you can review it and agree with it, please incorporate it into the
 > branch.  If it becomes controversial, I suggest deferring it to a
 > separate urgent task.

 This was reviewed and pushed.

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