BIND 10 #2436: update datasrc::ZoneLoader so it performs post load checks using validateZone()

[BIND 10 Development]

#2436: update datasrc::ZoneLoader so it performs post load checks using
validateZone()
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130122
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 vorner]:

 > > - The `Name` can be a reference (avoiding copy) here:
 > > {{{#!cpp
 > >         const dns::Name zone_name(updater_->getFinder().getOrigin());
 > > }}}
 > >   (although the saved copy overhead may be marginal in this context).
 >
 > Can it be really? The `getOrigin()` returns an object, not reference. As
 it is
 > a return value, it is temporary, so it'd be taking reference of a
 temporary. I
 > know it is not allowed taking pointer of such value, so I thought it is
 the
 > same with reference. Or is there some hidden trick in C++?

 I don't know if it's a "trick", but, yes, it can.  According to C++ 2003
 spec: (in Section 12.2 "Temporary objects") "There are two contexts in
 which temporaries are destroyed at a different point than the end of
 the full-expression....The second context is when a reference is bound
 to a temporary. The temporary to which the reference is bound or the
 temporary that is the complete object to a subobject of which the
 temporary is bound persists for the lifetime of the reference except
 as specified below."  (the exceptions don't apply to our case).

 You can easily confirm it (at least that it works that way with your
 compiler) by writing a simple test program.  Besides, there are
 already many such cases in our code.

 > > '''zone_loader_unittest'''
 > >
 > > - This doesn't have to be `BasicRRset`.  A mere `RRset` suffices:
 > > {{{#!cpp
 > >         RRsetPtr newRRset(new isc::dns::BasicRRset(rrset.getName(),
 > > }}}
 >
 > Actually, I don't think „mere“ is in place here, since RRset inherits
 from
 > `BasicRRset`. I used `BasicRRset` as it is the most primitive class that
 > suffices. I don't really need the signatures offered by full RRset.

 Ah, right, I actually referenced rrset.h but was somehow confused
 about the relationship between these two.

 > > - in copyValidation, shouldn't even prepareSource() fail once we
 > >   complete #2499?
 >
 > Yes, it would. That's what the TODO says, then some new way would have
 to be
 > used to load the data.

 In that case, the TODO isn't clear enough to me.  I'd rephrase it like
 this:

 {{{#!cpp
     void prepareSource(const Name& zone, const char* filename) {
         // TODO:
         // Currently, source_client_ is of InMemoryClient and its load()
         // doesn't perform post load checks.  It will change in #2499, at
         // which point the loading may start failing depending on details
 of
         // the test data.  If and when that happens we probably prepare
 the
         // data in some other way (using sqlite3 or something).
         source_client_.load(zone, string(TEST_DATA_DIR) + "/" + filename);
     }
 }}}

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