BIND 10 #2377: define dns::MasterLoad class

[BIND 10 Development]

#2377: define dns::MasterLoad class
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:21 vorner]:

 > > > Here, in the ticket, or somewhere in the code?
 > >
 > > In this ticket, in the sample code snippet of the task description:
 >
 > No, I mean where should I leave the note.

 I meant in the code.

 > > > I believe most or all of these check should actually go to some
 `validate()`
 > > > function we want to do later.
 > >
 > > Right, but...
 > >
 > > > 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).
 > >
 > > for this particular case it will be much difficult to check later,
 > > because we need to go through the entire zone to see if there's any
 > > SOA RR.  I guess that's why BIND 9 does this check in the loader code.
 >
 > Well, that is not entirely true. For one, the check could be done in the
 > addRR callback. And, won't validate iterate the whole zone because of
 some
 > other checks anyway?

 could be, but I'd oppose to requesting the addRR callback to do it
 because multiple callbacks need to do the same check (even we are now
 going to have two ourselves).

 The post-load validator will have an "advanced" check mode that will
 iterate over the entire zone, but that's an optional mode disabled by
 default (at least in BIND 9).  The check for the owner name of SOA is
 basically (see below) always performed.  Note that the "advanced"
 checks that involve full iteration cannot effectively be done within
 `MasterLoader`, because they generally check RRs of two different
 owner names (e.g. NS and corresponding AAAA/A RRs).

 > Actually, I'd very like the MasterLoader to be able to load anything
 that is at
 > least slightly syntactically correct, even if it makes no sense as a
 zone.

 Actually, I'd like that, too.  So, what I really intended was to
 introduce an option to `MasterLoader` that allows skipping of this
 check.  And, I wasn't aware of that when I first made this comment,
 but BIND 9 actually works that way:
 {{{#!c
                 if (type == dns_rdatatype_soa &&
                     (lctx->options & DNS_MASTER_ZONE) != 0 &&
                     dns_name_compare(ictx->current, lctx->top) != 0) {
                         char namebuf[DNS_NAME_FORMATSIZE];
                         dns_name_format(ictx->current, namebuf,
                                         sizeof(namebuf));
                         (*callbacks->error)(callbacks, "%s:%lu: SOA "
                                             "record not at top of zone
 (%s)",
                                             source, line, namebuf);
 ...
                 }
 }}}

 > > - I was not sure about the status/plan on this point: "It doesn't seem
 > >   to implement this part of BIND 9's implementation:"
 > > {{{#!c
 > > 99if (token.type == isc_tokentype_eof) {
 > > 999if (read_till_eol)
 > > 9999WARNUNEXPECTEDEOF(lctx->lex);
 > > ...
 > > 99}
 > > }}}
 > >   I saw some comment in the test that may be related to this topic,
 > >   but cannot find a change on the main code or mention in your
 > >   response.
 >
 > It was one of the comments I didn't have time for on Friday, maybe I
 didn't say
 > it explicitly enough. It is handled now.

 It seems it only handles the case where EOF is encountered in error
 handling.  If I read it correctly, this case is also handled at the
 end of successful load (only with an abrupt EOF) in BIND 9.

 I'd also make the warning message as compatible with BIND 9 as
 possible unless there's a reason for not doing so:
 {{{#!cpp
                         callbacks_.warning(lexer_.getSourceName(),
                                            lexer_.getSourceLine(),
                                            "file does not end with
 newline");
 }}}

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