BIND 10 #2850: Define and implement ZoneTableSegmentMapped

[BIND 10 Development]

#2850: Define and implement ZoneTableSegmentMapped
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:  muks
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  data source   |                    Milestone:
            Keywords:                |  Sprint-20130514
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  5             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 This completes my review.  I made and pushed relatively small and
 minor changes (basically trivial cleanups and editorial/style matters)
 directly.

 '''zone_writer.h/cc'''

 - maybe a matter of taste, but we might pass a reference of
   `ZoneTableSegment` to the `ZoneWriter` constructor if it cannot be
   NULL.

 '''zone_writer.h'''

 - name is not described, install_action is not part of the parameters:
 {{{#!cpp
     /// \brief Constructor
     ///
     /// \param segment The zone table segment to store the zone into.
     /// \param load_action The callback used to load data.
     /// \param install_action The callback used to install the loaded
 zone.
     /// \param rrclass The class of the zone.
     ZoneWriter(ZoneTableSegment* segment,
                const LoadAction& load_action, const dns::Name& name,
                const dns::RRClass& rrclass);
 }}}

 - cleanup: I'm not sure about the intent of "generally" in the
   original base class version, but if that was simply because derived
   classes could behave differently, I guess we can simply remove this
   word:
 {{{#!cpp
     /// Generally, this should never throw.
     void cleanup();
 }}}
   In fact, I don't understand how it could throw now.

 '''zone_writer.cc'''

 - I'd include zone_table_segment.h explicitly as long as the .cc file
   directly refers to the definition.

 '''zone_table_segment_test.h'''

 - We shouldn't need getZoneWriter() method at all.

 '''zone_table_segment_mapped_unittest.cc'''

 - I guess some comments on the main code would require more tests
   (checking exception guarantee, invalid mode, etc)
 - a minor point, but why you define both the destructor and
   `TearDown`?  Especially when you have the former, there's basically
   no point to have `TearDown` unless, e.g., you want to check
   something with `ASSERT_xx`.
 - it's generally better to avoid initializing an object of a non
   trivial type in a namespace level:
 {{{#!cpp
 const std::string mapped_file = TEST_DATA_BUILDDIR "/test.mapped";
 }}}
   to avoid fiasco.
 - resetBadConfig: missing case: params is NULL.
 - reset test: it generally seems to lack (some) necessary checks for
   each mode:
   - For CREATE: we need to confirm it really creates a new file or at
     least removes any existing content, and the resulting segment is
     writable.
   - For READ_WRITE: in case the segment already exists we need to
     confirm it doesn't change existing content (except for the
     checksum), and the resulting segment is writable.
   - For READ_ONLY: we need to confirm it doesn't change any existing
     content, and the resulting segment is not writable.
 - I believe the previous cases can be tested by inspecting/tweaking the
   underlying memory segment.  Also, I'd separate these three cases.
   Each case is complicated enough, so mixing them would tend to cause
   missing cases, and even if it's comprehensive it's not clear for the
   reader of the test code (= me).
 - As for corrupted data, I think it's better to test it at some
   realistic level.  Data corruption will certainly happen some time,
   while quite rare, so it's better we can be sure it works as we
   expect when it finally happens.

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