BIND 10 #2470: use MasterLoader in memory::ZoneDataLoader

[BIND 10 Development]

#2470: use MasterLoader in memory::ZoneDataLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         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):

 Thanks for the review.

 Replying to [comment:7 vorner]:

 > Regarding the changelog, maybe it's worth to have it. But I don't know
 if
 > user-facing channel like changelog should contain names of classes in
 the code.

 I do not necessarily think it's awkward to refer to C++ classes
 because BIND 10 also aims to provide library for developers, but in
 this context I tend to agree.  Is this better than the previous one?

 {{{
 526.?   [func]*         team
         The in-memory data source now uses a more complete master file
         parser to load textual zone files.  As of this change it supports
         multi-line RR representation and more complete support for escaped
         and quoted strings.  It also produces more helpful log when there
         is an error in the zone file.  It will be enhanced as more
         specific tasks in the #2368 meta ticket are completed.  The new
         parser is generally upper compatible to the previous one, but due
         to the tighter checks some input that has been accepted so far
         could now be rejected, so it's advisable to check if you use
         textual zone files directly loaded to memory.
         (Trac #2470, git TBD)
 }}}

 > The most significant problem with the branch is, the
 master_loader_callbacks.cc
 > isn't in the repository. Could you, please, commit it?

 As already commented, I've pushed it.

 > Also, I have several mostly minor comments on the code.
 >
 > The includes in `zone_data_loader.cc` contain `rrcollator.h` twice:
 > {{{#!diff
 > +#include <dns/master_loader.h>
 > +#include <dns/rrcollator.h>
 >  #include <dns/rdataclass.h>
 >  #include <dns/rrset.h>
 > -#include <dns/masterload.h>
 > +#include <dns/rrcollator.h>
 > }}}

 Good catch, removed the redundant one.

 > Also, the `rrcollator.cc` should include the `rrcollator.h` as the first
 header, not name.h, to ensure the `rrcollator.h` compiles without other
 dependencies.

 Hmm, the motivation looks reasonable, so I did it, but I have some
 comments.
 - in the general case it wouldn't be clear which ordering makes most
   sense for this purpose.  So there may be some implicit assumption
   that we do this in "some_module.cc" for "some_module.h", but it's
   not clear from the above comment.
 - likewise, the intent wouldn't be so obvious if we just reorder the
   header files.  Maybe some people notice the correlation of the
   prefix ("some_module"), but that's not super clear, and the
   background intent would be even less clearer.  So, if we do this
   practice I think we need to leave a comment to clarify the intent (i
   did it in this branch); otherwise someone may reorder them again as
   a cleanup, e.g., in the alphabetical order.
 - also related, if we do this, it should be consistently adopted
   project-wide, and should be documented in the style guideline.

 > Should this produce a warning of some kind?
 > {{{#!c++
 >     } else if (current_rrset_->getTTL() != rrttl) {
 >         // RRs with different TTLs are given.  Smaller TTL should win.
 >         current_rrset_->setTTL(std::min(current_rrset_->getTTL(),
 rrttl));
 >     }
 > }}}

 Yeah, I've realized we need it while working on #2429.  I implemented
 it, but there's one controversial point: I used
 `MasterLoaderCallbacks`, but the collator class cannot provide the
 source name or the line number because these parameters are lost when
 the RR callback is called.  If we want to include the correct values
 for these in the warning, we'll need some kind of extension in
 `MasterLoader`.

 > Looking at the RRCollator class makes me wonder how much performance we
 lose in the huge amount of pimpls we have. Is it really needed here? And
 in case it is needed, the RRCollator class should probably be made
 uncopyable, because this way we may get some nasty memory management bugs
 when someone copies it accidentally.

 First off, I don't think performance is not a big deal in this case
 for the expected usage.  Regarding whether we need it, it's hard to
 tell, and to some extent a matter of preference.  I realize I'm at the
 side of preferring pimpl in general in the project, because it helps
 reduce dependency a lot; in particular, it helps avoid leaking
 dependencies on std, boost or other external libraries via the public
 header file.  It then often helps reduce compile time (which can be a
 big plus for such a large package like BIND 10), and helps reduce the
 risk of seeing strange runtime behavior such as binary
 incompatibilities (you might remember we have had such weird things by
 leaking ASIO definitions through our header files).

 So, basically, unless the performance is a big concern (like in this
 case), I generally prefer pimpl.  That's certainly a "YMMV" thing
 though.

 > The brief here is missing text:
 > {{{#!c++
 >     /// \brief
 >     typedef boost::function<void(const RRsetPtr& rrset)>
 AddRRsetCallback;
 > }}}

 Oops, sorry.  I've completed the doc.

 > I don't see anything wrong about the name `finish`, but I think a more
 intuitive name could be `flush`.

 Okay, changed it to flush.

 > Is there a reason why these are not initialized in the initializer list?
 > {{{#!c++
 >     {
 >         a_rdata1_ = createRdata(RRType::A(), rrclass_, "192.0.2.1");
 >         a_rdata2_ = createRdata(RRType::A(), rrclass_, "192.0.2.2");
 >
 >         txt_rdata_ = createRdata(RRType::TXT(), rrclass_, "test");
 >
 >         sig_rdata1_ = createRdata(RRType::RRSIG(), rrclass_,
 >                                   "A 5 3 3600 20000101000000
 20000201000000 "
 >                                   "12345 example.com. FAKE\n");
 >         sig_rdata2_ = createRdata(RRType::RRSIG(), rrclass_,
 >                                   "NS 5 3 3600 20000101000000
 20000201000000 "
 >                                   "12345 example.com. FAKE\n");
 >     }
 > }}}

 Ah, good catch.  No, they can be initialized in the list, and that's
 better because we can make them const.  Changed it.

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