BIND 10 #1605: introduce special RRset for in memory data source

[BIND 10 Development]

#1605: introduce special RRset for in memory data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120306
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 First off, I made a few trivial changes directly to the branch
 (0169ac7).  I believe they are okay.

 '''About general design'''

 - I'd like to hide the definition (or ideally even the existence) of
   `RBNodeRRset` more reliably.  Conceptually, this class should be a
   hidden client of the in-memory data source implementation and should
   never be used by anyone else directly (are we on the same page about
   this?).  I see it'd still be good if we test the class directly, and
   for that purpose we need to make it visible at least to the test,
   so, as a compromise for that specific purpose (and basically only
   for that purpose) I'm okay with defining it in a separate header
   file.  But I'd like to make it very clear that no other applications
   than tests should use it directly via a comment, and probably by
   introducing a separate (sub)namespace such as "internal" or
   "detail".

   On a side note, assuming it's conceptually hidden and only visible
   to others as a const abstract object as the result of find() (and
   its variants) method, not supporting methods that modify the object
   should be justifiable.  Maybe we want to note that fact.

 - Also considering that `RBNodeRRset` is conceptually hidden, breaking
   const_cast in addRRsig/removeRRsig would also be justifiable.  But
   to prevent accidental misuse by a broken application more reliably,
   I'd rather keep them "not supported", and instead introduce a
   separate (conceptually private) non virtual methods, say,
   `addRRsigInternal` and `removeRRsigInternal`.  The in-memory data
   source implementation and tests will use the latter.

 - Whether it's virtual or not, we could avoid using const_cast in
   add/removeRRsig if we copy the given `RRset` on construction.
   That's also safer in that we can prevent a surprising possible bug
   where a passed RRset is reused for some other purpose with a (valid)
   assumption that it's not magically modified.  In practice, however,
   such a bug is probably unlikely to happen because the `RRset` would
   come from the masterLoad() function, which immediately gives up the
   ownership of the created `RRset`.  Also, not so far in future we'll
   need to stop retaining the full copy of original `RRset` anyway
   (firstly for memory consumption reasons, and possibly also for
   further response optimization), so this safe measure is probably a
   short term thing.  In the end, I'd leave the decision on this point
   to you.

 '''rbnode_rrset.h'''

 - This comment is a bit ambiguous to me:
 {{{#!c++
 /// - Calls that modify the associated RRSIGs of the RRset are allowed
 (even
 ///   though the pointer is to a "const" object).
 }}}
   That could also be interpreted as modifying the content of the
   associated RRSIGs.  To be more accurate, I'd say e.g. "Calls that
   add or remove the associated RRSIGs...".  (but see also the general
   design discussion about breaking constness).

 - I'd make the constructor 'explicit'.

 - getUnderlyingRRset() can be non virtual, and I think it's better.

 '''rbnode_rrset_unittest'''

 - Regarding type parameterisation: we can still do it partially by
   extracting common patterns with template specialization or just by
   defining separate tests for separate behaviors as we do for
   NSEC/NSEC3.  But the `RBNodeRRset` class will evolve more
   substantially in near future as we add more optimization logic
   there, and it may or may not make the test sharing more difficult,
   so until it's clear it's probably okay to keep the tests separate.
   (This is just a comment).

 - toWireRenderer: I'd suggest getting data (and length) directly from
   the renderer:
 {{{#!c++
     EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, renderer.getData(),
                         renderer.getLength(), &wiredata[0],
 wiredata.size());
 }}}
   It will be more robust after #1697 (which is ongoing).  And,
   whatever happens in #1697, using renderer should always be correct.
 - With the suggested change above, toWireRenderer and toWireBuffer
   could be unified into a single template function (if you want).

 - About the added test wire data: if you share test data, I'd rather
   add a data path and really share the data file than copying it.  You
   should be able to add a path here:
 {{{#!c++
     isc::UnitTestUtil::addDataPath(TEST_DATA_DIR);
 }}}
   (I noticed you modified rrset_toWire2.  I was not sure if it was due
   to an inevitable difference between the two rrset classes or just
   for convenience.  If it's the latter, I'd consider using the same
   data in the same test pattern)

 - checkSignature: I'd consider using rrsetCheck (defined in
   lib/testutils/dnsmessage_test.h) or Rdata::compare() to compare two
   RRs Comparing toText() result may not be very reliable.
 - addRRsigConstRdataPointer: what's the purpose of intermediate 'data'
   variable?  It can be ConstRdataPtr from the beginning:
 {{{#!c++
     ConstRdataPtr data = createRdata(rrset_siga->getType(),
                                      rrset_siga->getClass(),
                                      RRSIG_TXT);
     rrset_a.addRRsig(data);
     checkSignature(rrset_a);
 }}}

 '''memory_datasrc_unittest'''

 - Like checkSignature above, I'd suggest considering rrsetsCheck() to
   compare two sets of RRsets.  You can build two vector<RRset>s and
   pass them to rrsetsCheck().

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