BIND 10 #2431: support flexible ordering/appearance of TTL, type and class in dns::MasterLoader
BIND 10 Development
do-not-reply at isc.org
Mon Dec 17 06:49:57 UTC 2012
#2431: support flexible ordering/appearance of TTL, type and class in
dns::MasterLoader
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: libdns++ | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20121218
Sub-Project: DNS | Resolution:
Estimated Difficulty: 5 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| loadzone-ng
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by muks):
* owner: muks => jinmei
Comment:
Hi Jinmei
Replying to [comment:7 jinmei]:
> '''master_loader.cc (major)'''
>
> Overall it seems to do what it's expected to do, but I have several
> suggestions and concerns.
>
> - I'd like to avoid calling ungetToken() when possible. It's not
> super cheap for strings. On a closer look, I found we should be
> able to use the specific-token-type version of `getNextToken` and
> avoid ungetToken() (for valid input). At least there should be a
> string token for RR type, so, from the candidate of TTL/class
> through RR type, we can safely use this version of `getNextToken`.
> - Secondly, considering named-signzone output (always add explicit
> TTL, and add it first; omit class for some), I think we should check
> TTL first (BIND 9 checks the class first, but I'm not sure why).
> Depending on the overhead of the case the try fails (see below),
> this can affect the overall performance substantially.
> - IMO the code added in this branch makes `loadIncremental` too
> big and unreadable, and should better be delegated to a helper
> method.
> - On a related note, for RR class we only have to do check when
> explicitly specified; because we know what we should use:
> zone_class_.
>
> I'm attaching a diff implementing these ideas. Please take a look.
> If you think it makes sense, please incorporate it with adding more
> comments and more cleanups.
The updated code looks fine to me. I've added comments. There was a syntax
error in one of the statements, so I have to ask, did write all this
directly in the browser? If so, that's remarkable.
> Finally, a possibly controversial point. As you may have noticed in
> your review on #2429, I'm concerned about exception overhead when the
> try-and-error detection for TTL (and RRclass):
>
> {{{#!cpp
> try {
> rrclass.reset(new RRClass(rrparam_token.getString()));
> rrparam_token = lexer_.getNextToken();
> } catch (const InvalidRRClass&) {
> // If it's not an rrclass here, continue and try again
> // after the TTL below.
> }
>
> ...
> try {
> rrclass.reset(new
RRClass(rrparam_token.getString()));
> rrparam_token = lexer_.getNextToken();
> } catch (const InvalidRRClass&) {
> // If it's not an rrclass here, use the zone's
class.
> rrclass.reset(new RRClass(zone_class_));
> }
> }}}
>
> This is particularly so for RR class as it's often omitted in actual
> zone files (in that case we'll encounter two exceptions in your
> original code). I'm also concerned about the overhead of `new`ing
> the RRClass for every RR. One may argue it's premature optimization,
> but if you agree with me, I'd suggest doing something similar to what
> I did in #2431: introduce a "no throw" (and still minimizing
> resource allocation) version of `RRClass` factory:
> {{{
> RRClass*
> RRClass::createFromText(const string& rrclass_txt, RRClass*
placeholder);
> }}}
> and maintain a placeholder object within `MasterLoader` so we only
> have to `new` it once in the lifetime of the loader.
>
> And, in that case, my specific suggestion is to merge the branch
> without this particular optimization (while addressing other issues),
> leave the ticket open, and complete it in the second phase. (I don't
> like to delay our meta goal for this optimization, but I'm afraid the
> optimization part won't be taken if we defer it to a separate ticket.)
I refactored some code so that `RRParamRegistry::textToClassCode()` onward
doesn't throw. These are the three commits for it:
{{{
* 693ff3e [2431] Avoid redundant construction in parseRRParams()
* 3febd2c [2431] Add RRClass.fromText()
* 3ee0cf2 [2431] Refactor code so that RRParamRegistry::textToClassCode()
does not throw
}}}
Please see if these can go in. If this change is not okay, we can merge
everything up to `3ee0cf2^`.
> '''master_loader.cc (minor)'''
>
> - reference to exceptions in `catch` should be constified. I've
> noticed this is a common point I'd have to in your code. Maybe you
> want to include it in your personal (imaginary) check list. That
> can be even be statically checked by a simple script. cppcheck also
> generally complains about it, so running cppcheck before asking for
> a review would also be a good practice. This is trivial and I fixed
> it my self.
> - also, variables are not necessary in `catch` if you don't need them
> {{{#!cpp
> } catch (const InvalidRRClass& e) { // <= no need to define
'e'
> // If it's not an rrclass here, continue and try again
> // after the TTL below.
> }
> }}}
*nod*. I'll keep this in mind. Thank you for pushing a commit for these.
> '''master_loader_unittest.cc'''
>
> - I'd suggest providing expected error message in `error_cases` like
> the TTL-related cases do. Invalid input could fail in various ways,
> so if we don't check the actual message tests can still pass even if
> the error is detected in a different way than we actually intend.
> - ""Invalid Rdata" is not really accurate:
> {{{#!cpp
> { "www A IN 3600 192.168.2.8", NULL,
> "Invalid Rdata (incorrect order of class, TTL and type)" },
> }}}
> In fact, the "Rdata" is valid here. If we want to keep the "Invalid
> XXX" phrase, it should be "RR". But I'd rather simply say
> "Incorrect order of class, TTL and type". Same for other similar
> cases.
From the parser's point of view, it is invalid Rdata as everything after
the type (`A`) is passed to the `IN/A` constructor. But I've updated the
message anyway as the suggested message is just as good.
> - `ttlClassTypeOrder`: it repeats a certain amount of copy of the main
> .cc source, and doesn't seem to be good in terms of DRY. Can't we
> simply refer to the main source and/or be more concise here?
> {{{#!cpp
> // We test the order and lack of TTL, class and type. Both TTL and
> // class are optional and may occur in any order if they exist. TTL
> // and class come before type which must exist.
> //
> // [<TTL>] [<class>] <type> <RDATA>
> // [<class>] [<TTL>] <type> <RDATA>
> }}}
The comment is now updated, and the test was also renamed.
> - There are some other invalid cases where we should test, like:
> {{{
> ;; omit TTL and class, and missing RDATA
> www A
>
> ;; other variations of missing token (check if it covers all cases):
> www
> www 3600
> www IN
> www 3600 IN
> www IN 3600
> www 3600 A
> www IN A
> www 3600 IN A
> www IN 3600 A
> }}}
Done.
--
Ticket URL: <http://bind10.isc.org/ticket/2431#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list