BIND 10 #2377: define dns::MasterLoad class
BIND 10 Development
do-not-reply at isc.org
Sat Dec 8 00:46:31 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):
Replying to [comment:18 vorner]:
> 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.
Fair enough, but in that case I'd suggest explicitly rejecting 0 as an
invalid parameter. Otherwise, a careless application like this would
result in an infinite loop:
{{{#!cpp
bool done = false
while (!done) {
done = loader.loadIncremental(0);
}
}}}
> > - 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.
I see some points to discuss here. First, I admit it's not clear to
me why the BIND 9 style banned the implicit testing on zero-vs-nonzero
in http://bind10.isc.org/wiki/BIND9CodingGuidelines either (including
the case for NULL vs non-NULL pointers). I also admit if it's about
intuitiveness, the implicit form can look more intuitive. Secondly,
I understand it may look inconsistent if we ban implicit testing on
zero-vs-nonzero while we allow using the type-conversion (to bool)
operator method for smart pointers because syntax-wise both look same.
Since I don't know the original rationale of the BIND 9 rule and I can
only guess it, I cannot be sure, but at least I see some cases where the
implicit style could cause real harm which could be avoided with the
explicit style: http://www.cve.mitre.org/cgi-
bin/cvename.cgi?name=CVE-2012-2122
And, in that sense, the shared pointer usage is totally different;
our code itself doesn't play the risky business (depending on how the
type-conversion parameter is implemented, of course). So we could say
the policy is consistent: we prefer safer (less error prone) way; if
it's safe enough we prefer intuitive syntax.
In the case of this code:
{{{#!cpp
const rdata::RdataPtr data(rdata::createRdata(...));
if (data != rdata::RdataPtr()) {
}}}
I have another concern with my usual tendency of optimizing things
prematurely: It involves the construction of another shared pointer
and a call to `operator!=()` while this one:
{{{#!cpp
const rdata::RdataPtr data(rdata::createRdata(...));
if (data) {
}}}
requires a single method call on `data`. A smart compiler might
produce the same binary code for both cases, and, in any event the
overhead shouldn't be a big issue in this context. But, when we can
use a simple, safe, and more efficient syntax (and in this case we
can), I'd love to use it.
As such I'd personally propose:
- following the BIND 9 guideline for integers and bare pointers
- allowing the use of type conversion operator for smart pointers
But, if this looks so confusing syntactically (which is understandable
in some sense), an alternative would be to consistently use explicit
testing with get() for smart pointers:
{{{#!cpp
shared_ptr<T> tp;
// if (tp) { ... } // banned
if (tp.get() != NULL) { ... } // OK
}}}
Maybe the bikeshed of the week for the next team call?
> > - 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?
In this ticket, in the sample code snippet of the task description:
{{{#!cpp
// Various checks (skip for now except this)
// If it's SOA, check if it's zone_origin.
}}}
> 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.
Now, on the revised branch:
'''master_loader.cc'''
- stream version of `MasterLoader` constructor doesn't seem to be
exception safe. We need something like:
{{{#!cpp
auto_ptr<MasterLoaderImpl> impl(new MasterLoaderImpl...);
impl->pushStreamSource(stream);
impl_ = impl.release();
}}}
- About this point:
>> Especially when `MANY_ERRORS` isn't specified, I think we should tell
>> the caller it fails when it fails, [...]
>> the `MasterLoader` class. I prefer throwing an exception
I'd also like to somehow indicate the error to the caller when it
happens in the "lenient (MANY_ERRORS)" mode. Maybe record the
first or last error message in the class so the caller can inspect
it?
'''master_loader_unittest.cc'''
- prepareZone(): if you still want to keep it in the class, I'd
suggest making it static or at least a const member function to
clarify that it doesn't depend on instance-specific information of
the class.
- checkRR(): I didn't notice this before, but I believe this can be
EXPECT_EQ:
{{{#!cpp
ASSERT_EQ(type, current->getType());
}}}
(and in which case it's generally preferred)
- 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
if (token.type == isc_tokentype_eof) {
if (read_till_eol)
WARNUNEXPECTEDEOF(lctx->lex);
...
}
}}}
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.
--
Ticket URL: <http://bind10.isc.org/ticket/2377#comment:19>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list