BIND 10 #2377: define dns::MasterLoad class
BIND 10 Development
do-not-reply at isc.org
Fri Dec 7 06:03:13 UTC 2012
#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
-------------------------------------+-------------------------------------
Comment (by jinmei):
'''master_loader_callbacks_test.cc'''
- I'd use isc::util::rrsetCheck() to compare RRsets:
{{{
EXPECT_EQ(expected_rrsets_.front().get()->toText(),
rrset.toText());
}}}
(at least in theory 2 equivalent RRsets can produce different
outputs via toText())
'''master_loader.h, cc'''
- I guess we'd also like to provide another version of constructor for
`MasterLoader` that takes `istream` instead of file name.
`MasterLoaderTest` can benefit from that, and I believe there are
many similar use cases.
- I'd explain a bit more about what "lenient mode" is and/or refer to
the related part of the class (e.g. the constructor)
{{{#!cpp
MANY_ERRORS = 1 ///< Lenient mode.
}}}
- 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);
}
}}}
- Especially when `MANY_ERRORS` isn't specified, I think we should tell
the caller it fails when it fails, either by the a result code or by
exception. Maybe what you had in your mind is that our datasrc
doesn't need it because it records the error via its callback, back
I don't think we should rely on such practice at the caller side in
the `MasterLoader` class. I prefer throwing an exception, as this
class is for general use, and exceptions are the common way of
reporting an error throughout this library.
- I'd explicitly include <string> in master_loader.cc.
- `MasterLoaderImpl::pushSource()`: we'll eventually do this operation
for $INCLUDE, too, and in that case this form of reporting wouldn't
be ideal:
{{{#!cpp
callbacks_.error("", 0, error);
}}}
We'd like to refer to the point (file, line) where $INCLUDE is given.
We could revise the code later when we support $INCLUDE, but if we
can make it possible from the beginning that would be better.
- `MasterLoaderImpl::pushSource()`: related to the previous point,
if this failure is for $INCLUDE and MANY_ERRORS is specified, we'd
like to keep going; setting ok_ to false with the current
implementation of `loadIncremental` would prevent that.
- `MasterLoaderImpl::pushSource()`: it probably makes sense to pass an
empty string and 0 for the line number in the case of failure of
opening the top level file, but I think we document it as part of
the API contract. Then the callback user can change the behavior
accordingly (e.g., not including the form of "filename:line" in the
log)
- Mainly for my own personal convenience, may I ask for defining at
least `MasterLoaderImpl::loadIncremental()` outside of the class?
i.e.
{{{#!cpp
class MasterLoader::MasterLoaderImpl {
public:
bool loadIncremental(size_t count_limit); // declaration only
};
bool
MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
// definition is here
}
}}}
Somehow my editor becomes very heavy if I try to edit a C++ source
file containing a very large structure/class. I expect if we
continue the current style the body of the class will be getting
bigger and eventually reach this point.
- It doesn't seem to implement this part of BIND 9's implementation:
{{{#!c
if (token.type == isc_tokentype_eof) {
if (read_till_eol)
WARNUNEXPECTEDEOF(lctx->lex);
...
}
}}}
- This block of code seems to behave differently from BIND 9. Is that
intentional?
{{{#!cpp
bool empty = true;
do {
const MasterToken& empty_token(lexer_.getNextToken());
if (empty_token.getType() == MasterToken::END_OF_FILE)
{
// TODO: Check if this is the last source,
possibly pop
return (true);
}
empty = empty_token.getType() ==
MasterToken::END_OF_LINE;
} while (empty);
// Return the last token, as it was not empty
lexer_.ungetToken();
}}}
I guess a port of the corresponding part of the BIND 9 version would
look like this:
{{{#!cpp
// We should also specify MasterLexer::INITIAL_WS (see my previous
comment
// for the ticket)
const MasterToken& token = lexer_.getNextToken(MasterLexer::QSTRING);
switch (token.getType()) {
case MasterToken::END_OF_FILE:
if (read_till_eol) {
callbacks_.warn(...);
}
// TODO, possibly pop source
return (true);
case MasterToken::END_OF_FILE:
read_till_eol = true;
continue;
case MasterToken::ERROR:
if ((options_ & MANY_ERRORS) == 0) {
return (true);
}
callbacks_.error(...);
read_till_eol = true;
continue;
case MasterToken::STRING:
case MasterToken::QSTRING:
break;
default: // buggy case
assert(false); // or throw UnExpected or something
}
}}}
As we discussed in a different ticket, I don't necessarily insist we
should do the same, but I also believe it's generally wise to follow
the existing practice unless we have a specific reason not to do
so. In this specific case, at least I see one advantage in the
latter version: we don't have to do ungetToken() and re-get it.
re-getting a string token is a bit more expensive because we go
through the string characters and copy them twice.
- At the risk of told premature, I'd like to suggest having a
placeholder std::string object in `MasterLoaderImpl` and keeping
using it when a string token is needed. Then `getString()` would
look like:
{{{#!cpp
string str_token_;
...
const string& getString() {
lexer_.getNextToken(MasterToken::QSTRING).getString(str_token_);
return (str_token_);
}
}}}
- getString(): if we keep returning a string object, `const` isn't
necessary.
- This `StringRegion` can be a reference (avoiding copy):
{{{#!cpp
const MasterToken::StringRegion
name_string(lexer_.getNextToken(MasterToken::QSTRING).
getStringRegion());
}}}
(the referred object is guaranteed to be valid until next call to
getNextToken() etc)
- getString() accepts quoted strings, but BIND 9 doesn't accept them
for these:
{{{#!cpp
const RRTTL ttl(getString());
const RRClass rrclass(getString());
const RRType rrtype(getString());
}}}
- can't `data` be of `ConstRdataPtr`?
{{{#!cpp
const rdata::RdataPtr data(rdata::createRdata(rrtype,
rrclass,
}}}
(with adjusting the signature of add callback accordingly)
- 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 think we should do #2518 and catch `DNSTextError` and (basically)
propagate others (which really shouldn't happen and wouldn't be
recoverable in this context):
{{{#!cpp
} catch (const isc::Exception& e) {
// TODO: Do we want to list expected exceptions here
instead?
}}}
Until then, I'd simply catch all `isc::Exception`s.
- The condition of `(options_ & MANY_ERRORS) != 0` would be frequently
checked as we implement more cases, so I think it makes sense to
introduce a member variable `many_errors_` and initialize it in the
constructor (and refers to it thereafter).
- 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.
'''master_loader_callbacks.h'''
- doc for `AddRRCallback` should be updated, too.
- maybe a matter of taste, but prepareBrokenZone() doesn't even seem
to have to be a member function of `MasterLoaderTest`.
- shouldn't we clean up broken.zone either in the destructor or
via CLEANFILES? (note that if we extend the loader class to accept
a stream directly this can be moot)
- checkRR: technically, it's better to compare RDATA as Rdata objects
(using compare()) or to compare two RRsets using
isc::util::rrsetCheck().
- I'd add some brief comments for the members of `ErrorCase`.
- our implementation (as does BIND 9) accepts quoted owner name. it
should be tested.
- I'd test some cases getNextToken() results in ERROR, e.g.
{{{
unbalanced)paren 3600 IN A 192.0.2.1
www 3600 unbalanced)paren A 192.0.2.1
}}}
- 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.
- Assuming we change the behavior so we won't accept quoted string for
class, type, etc, (to be compatible with BIND 9, see the
corresponding comment above), I'd test those cases, too.
--
Ticket URL: <http://bind10.isc.org/ticket/2377#comment:16>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list