BIND 10 #374: Writable datasources: Basic support for writables in C++ datasrc library
BIND 10 Development
do-not-reply at isc.org
Tue Nov 9 14:39:09 UTC 2010
#374: Writable datasources: Basic support for writables in C++ datasrc library
------------------------------+---------------------------------------------
Reporter: jelte | Owner: UnAssigned
Type: enhancement | Status: reviewing
Priority: major | Milestone:
Component: data source | Resolution:
Keywords: | Sensitive: 0
Estimatedhours: 0.0 | Hours: 0
Billable: 1 | Totalhours: 0
Internal: 0 |
------------------------------+---------------------------------------------
Comment(by jinmei):
Review is still ongoing, but in case Jelte has spare time or needs to
kill night time due to jetlag, I'm providing partial feedback.
'''general design'''
I personally still believe it's better to move DNS protocol specific
behavior out of the data source module as you perhaps felt. I've
commented on some specific points in this context below, too. But
assuming this is an initial attempt that will be revisited, I'm okay
with completing this work without touching this level of design issue.
Likewise, I still believe the current class hierarchy is not well
defined. The fact that many derived methods end up just returning
'not implemented' means (IMO) the relationship between the base and
derived classes is not a reasonable "is-a" relationship. But, again,
I'm okay with completing this work without touching this level of
topic.
'''general comments about the code'''
- in general it would be better to pass reference instead of a
(shared) pointer if it cannot be NULL in that context. this is
good in that we wouldn't worry about the (buggy) case where a NULL
is passed by accident, and in case it's a boost shared pointer used
for a public method, it's good in that we can reduce dependency on
boost things. I've commented on this topic in some specific points
below, too.
- there are some too long lines. if we follow BIND 9's guideline, it
would be better to fold lines so that they don't exceed 79
characters.
- it seems many variables could be defined as const. I'm pointing
out specific cases as I add comments below (and probably in
subsequent review comments), but it's quite likely to be
incomplete, so it would be nice if you can perform double check.
'''general comments about protocol processing for dynamic update and
IXFR'''
In general, it seems the current implementation needs more checks to
make it protocol compliant (see specific comments for some specific
issues). It's also not clear how much of the protocol processing
should be done in the C++ code, assuming we'll write python daemons
for these protocols.
If we can cleanly separate dynamic update and IXFR part of the code,
maybe we should merge the general part (possibly with the part
necessary for AXFR and zone loading) first, and then revisit the
protocol processing part with more unit tests (or even reconsider
whether to provide C++ code for them). Or, perhaps we could integrate
these parts as a "work in progress" item for now (after review, of
course) and not actually use them in our server programs, and
revisit/redesign them later.
'''data_source.cc'''
- equalRRsets()
- using keyword 'static' to indicate a function to be file local is
obsolete in C++. the more 'politically correct' way is to define
an unnamed namespace:
{{{
namespace {
bool
equalRRsets(ConstRRsetPtr a, ConstRRsetPtr b) {
...
}
}
}}}
- since 'a' and 'b' are assumed to be non null (shared) pointers,
I'd declare them as 'const RRset&' (and adjust the caller side
accordingly). Then we wouldn't have to worry about a buggy case
where it's a null.
- I'm afraid the current implementation of 'equality' check is
incomplete. Consider the following corner cases (some of which
may not be so rare)
- RRsets with different TTLs
- there's a duplicate RDATA (e.g. there are two A RRsets, one
contains a single '192.0.2.1', and the other contains two
'192.0.2.1's.
- RDATA is equal as a set, but the ordering is different.
FYI, BIND 9 takes care of the ordering issue. ignoring TTLs is
probably okay in this context (and I know it's even 'necessary')
especially because it's a private helper function. but I'd add an
explicit note that TTL is ignored in the definition of 'equal'.
- haveRRset()
- is it okay to assume the IN class for ANY/NONE?
- as a public method, the logic is too dynamic update-specific and
counter intuitive. this could actually be non public, non method
function?
- since (it seems) 'rrset' couldn't be a null (shared) pointer, I'd
declare it as a 'const RRset&'. See the comment for
equalRRsets(). and , this would be especially good for public
interfaces because it makes it less dependent on boost definitions.
- updateCheckPrerequisite()
- I'd declare prereq as 'const RRset&' (see above)
- what about case RRSIG? (shouldn't we cover type-covered? FYI,
BIND9 does something special in case of updating RRSIG)
- does this code cover empty non-terminal cases?
- updateProcessUpdate()
- there's indentation inconsistency for the line of the second
parameter ('update')
- I'd declare update as 'const RRset&' (see above)
- I'm not so sure if we can omit pre-scan for in memory data source.
- (comment) RDLEN check must be in dns::Message (otherwise it would
fail to parse the request), but double check is probably necessary,
too
- indent for the 'update' parameter
- update_type should better be defined as const
- this condition is insufficient (NONE should also be checked)
{{{
if (update->getClass() != RRClass::ANY()) {
}}}
- meta RR types should be checked for the case of class = NONE
- about "other special handling", we should probably refer to BIND 9.
It performs more checks. (bind9/bin/named/update.c:update_action())
- callbackHelperRRsetIterator()
- many questions including the design, but anyway...
- is there any specific reason for using the postfix form of ++
(which is inefficient)?
{{{
(*cur)++;
}}}
- can cur/end be validly NULL? if there's no valid scenario for
that, I'd rather throw an exception or assert() it.
- callbackHelperRRsetVector()
- I'd not rely on implicit conversion from a pointer to bool...
{{{
if (v && i && *i < v->size()) {
}}}
- ...but, more fundamentally, I'd also question whether we ever
allow NULL for 'v' or 'i' (see also a similar comment on
callbackHelperRRsetIterator) or out-of-range index for 'v'
(except, perhaps for the end of the vector)
- doIXFR()
- I'd consider a state mismatch an exception:
{{{
if (transaction.getState() != DataSrcTransaction::RUNNING) {
}}}
- I'd defer the declaration of 'result' until around here:
{{{
RRsetPtr next_rrset, last_rrset;
}}}
- final_soa/first_soa could/should be of ConstRRsetPtr. Same for
next_rrset and last_rrset. Furthermore, final and first could even
be of 'const ConstRRsetPtr'.
- this should be !first_soa:
{{{
RRsetPtr first_soa = nextRRset(arg1, arg2);
if (!final_soa) {
}}}
(btw: this might suggest unit tests are not sufficient.)
- (a high level comment) this code assumes if the first two RRs are of
SOA it's IXFR, but,
technically, it's not really correct because an AXFR could also
consist of just two SOA RRs (although, of course, the resulting
zone would become bogus as it wouldn't have an NS). For that
matter, I'm not really sure how much of this level of protocol
processing is assumed to be done in the C++ code. Assuming most of
protocol level consideration for XFRs should go to python, ideally
we should be able to concentrate on operations at the level of data
source abstraction, i.e., simply adding/deleting DB rows or
replacing a table, without worrying about their DNS protocol
implications.
- (aside from the high level comment) I'd treat protocol-level
errors such incorrect sequence of SOAs as an exception, or at least as
a
different type of error than W_xxx (because these errors are not
really a data source level error).
- the first while loop seems to be a duplicate of the internal of
Sqlite3DataSrc::replaceZone(). Can't we unify the code?
- this comment is incomplete?
{{{
// We don't delete the actual SOA itself,
}}}
- I'm not sure if this behavior is reasonable:
{{{
// check if rrset exists, if not, something is very wrong,
abort
}}}
RFC1995 doesn't seem to talk about this case, and FYI BIND 9 is
generous about such a case:
{{{
} else if (result == DNS_R_UNCHANGED) {
/*
[...]
* It may happen when receiving an IXFR
* from a server that is not as careful.
* Issue a warning and continue.
*/
}}}
- return value is ignored here:
{{{
addRRset(transaction, final_soa);
if (result != W_SUCCESS) {
}}}
- the final return seems to be redundant:
{{{
return (W_NOT_IMPLEMENTED);
}}}
- doUpdate()
- a high level comment: like doIXFR(), I'd wonder how much of
protocol processing should be in the C++ lib.
- shouldn't we care about state mismatch like doIXFR()?
(interestingly, doIXFR() doesn't care about the case of Opcode
mismatch. so the Opcode version of this question should go to
doIXFR(). and, I'd treat an Opcode mismatch as an exception, too)
- 'question' could/should be of 'const ConstQuestionPtr'.
- I don't understand the comment 'should we do transaction here?'
- (aside from the high level comment above) I'd treat protocol-level
errors such as zone name mismatch as an exception a
different type of error than W_xxx (see the same comment to doIXFR()).
- couldn't/shouldn't we use the postfix form of ++?
{{{
it++) {
}}}
(I'd personally rather use an STL algorithm or boost foreach, but
wouldn't insist on it)
- cur_prereq could/should be ConstRRsetPtr, or could even be
omitted:
{{{
Rcode prereq_result = updateCheckPrerequisite(transaction, *it);
}}}
(but I'd even go further: passing a 'const RRset&' instead of a
shared pointer. see the comment for updateCheckPrerequisite())
- is it okay to clear the message in case of error? i.e. shouldn't
we use makeResponse()?
- same comments apply to the update section. in addition, why
don't we update 'msg' in case of error?
- (comment) BIND 9 doesn't seem to do anything with the additional
section.
- ~DataSrcTransaction()
- we don't have to call getState() (although the overhead would be
very marginal)
- I think we should be more careful about cases where
rollbackTransaction() fails. When that happens it means something
catastrophic, right? So, IMO we should have rollbackTransaction()
report the type of error in as detailed as possible (whether it's
via a return code or an exception), and deliver its implication to
the user at all cost (for example, by pre-allocating necessary
resources to log this event without triggering an exception).
Also in this context, I'd request rollbackTransaction() to limit
the possible exceptions within it so that this destructor can
catch any possible exceptions more selectively (in fact, as far as
I can see Sqlite3DataSrc::rollbackTransaction() should never throw
an exception). (btw: in case I'm not clear I understand we cannot
let an exception be propagated from a destructor)
(to be continued)
--
Ticket URL: <http://bind10.isc.org/ticket/374#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list