BIND 10 #2377: define dns::MasterLoad class

[BIND 10 Development]

#2377: define dns::MasterLoad class
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  7             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 I'm running out of work time for this week, so I'm giving the review back,
 hoping you can review at least the things I changed (so I have something
 to
 work on on Monday and we don't delay this ticket). I note on the things I
 didn't handle yet below (what isn't mention should be handled).

 Replying to [comment:13 jinmei]:
 > One quick thing: I originally intended to support the "initial space"
 > handling to detect the owner name in this ticket:

 I'll update the ticket in a short while. I'd like to merge this branch
 ASAP, as so many other tickets depend on it.

 Replying to [comment:16 jinmei]:
 > - We might give the count_limit of 0 a special meaning: "don't break",
 >   and then we can specify it in load():
 > {{{#!cpp
 >     void load() {
 >         const bool completed = loadIncremental(0);
 >         assert(completed);
 >     }
 > }}}

 I don't think it is a good idea. It would make the already simple to
 understand
 function even simpler, but at the cost of making the already complicated
 function more complicated and also introducing illogical/exceptional
 behaviour
 on the interface.

 > - It doesn't seem to implement this part of BIND 9's implementation:

 I didn't handle this yet.

 > - This block of code seems to behave differently from BIND 9.  Is that
 >   intentional?

 I didn't handle this yet.

 It is not intentional. I tried to read the bind9 code and got scared of
 it.
 It's a 1200-lines long function full of GOTOs. So I gave up trying to
 understand how it works, hoping I can implement it in simpler/easier to
 read
 way with OOP and other C++ advantages. But, naturally, there are
 differences.
 I'm going to try to read the bits you indicate at least and try to get
 through
 them.

 > - can't `data` be of `ConstRdataPtr`?
 > {{{#!cpp
 >                 const rdata::RdataPtr data(rdata::createRdata(rrtype,
 rrclass,
 > }}}
 >   (with adjusting the signature of add callback accordingly)

 In theory, it could. But I don't see an advantage ‒ once the Rdata is
 handled
 over to the callback, it is no longer used by the loader. The Rdata
 ownership
 is effectively passed onto the callback, so let it do whatever it likes
 with
 it. I don't know what uses someone might find for the loader, so I don't
 want
 to limit it needlessly.

 > - we can use type conversion to bool for shared pointer:
 > {{{#!cpp
 >                 if (data) { // different from using a ptr value as
 boolean
 >                     add_callback_(name, rrclass, rrtype, ttl, data);
 > }}}

 I'm not sure our style is being consistent on auto-conversions to bool.
 So,
 primitive types are not OK, but objects are, even if they have it through
 `operator <unspecified integral>`, using the implicit conversion of
 primitive
 type to bool?

 I myself find it quite intuitive, to use a pointer as bool (implicitly
 meaning
 „is there something pointed to“), and in many cases with ints too (`if
 (count)`, but not `if (strcmp())`). But I know these are not allowed, so
 following from that, I concluded if `if (pointer)` is counter-intuitive to
 read
 for some, `if (shared_ptr)` would be too.

 But if you say it is allowed, I'm all for it.

 > - the ticket description suggests checking the owner name of SOA.  It
 >   doesn't necessarily have to be done in this ticket, but we need to
 >   do it at some point.  If you chose to defer it, please at least leave
 a
 >   comment about that.

 Here, in the ticket, or somewhere in the code?

 I believe most or all of these check should actually go to some
 `validate()`
 function we want to do later. I'm not sure the loader itself is the best
 place
 to be crowded by such checks (expecting it to get even more complicated
 with
 all the $<something> handling and stuff).

 > - maybe a matter of taste, but prepareBrokenZone() doesn't even seem
 >   to have to be a member function of `MasterLoaderTest`.

 I just wanted to have all the auxiliary functions together.

 > - is this case tested?
 > {{{
 >                             case MasterToken::END_OF_FILE:
 >                                 // TODO: Try pop in case this is not the
 only
 >                                 // source
 >                                 return (true);
 > }}}
 >   In either case, we should probably run lcov (if you didn't) as this
 >   implementation is quite complicated and it's not immediately clear
 >   whether all cases are covered in tests.

 I don't usually use lcov (it has been broken on my system for some longer
 period of time and I didn't get the habit).

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