BIND 10 #2435: implement datasrc version of RRsetCollection
BIND 10 Development
do-not-reply at isc.org
Thu Jan 10 14:00:04 UTC 2013
#2435: implement datasrc version of RRsetCollection
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: data source | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130122
Sub-Project: DNS | Resolution:
Estimated Difficulty: 4 | 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]:
> '''generic'''
>
> - shouldn't the copyright year of new files be 2013?
This was updated, but now the new files have been removed in later
commits.
> '''rrset_collection.h'''
>
> - maybe a matter of taste, but I'd generally like to avoid the
> abbreviation of "DS" to mean "data source". "DS" is too short and
> could mean anything, and, in particular, it can be easily confused
> with the delegation signer in the context of DNS:
> {{{#!cpp
> class DsIter : public RRsetCollectionBase::Iter {
> }}}
This was updated, but the class was removed in later commits.
> - the relationship between the lifetimes of the updater and the
> collection should be documented.
This is now documented.
> - should we bother to even define `DsIter`? If both getBeginning()
> and getEnd() just throw, this class can never even be instantiated
> in practice.
Done.
> '''rrset_collection.cc'''
>
> - I wouldn't leave the TODO open. Although it wouldn't be critical
> for our intended usage in practice, implementing it shouldn't be
> difficult. e.g. we can pass the RR class on construction as the
> user of the collection should know it; alternatively, we can check
> rrset->getClass() for the return value of find() and if doesn't
> match return NULL instead.
The class is checked now.
> - It doesn't seem to handle other minor cases correctly: what if the
> finder returns DNAME? Or CNAME? Or delegation? And any other
> possible cases? What if the wildcard flag is set even if you
> specify NO_WILDCARD?
These are implemented now, but I don't know what is meant if the wildcard
flag is set even when `NO_WILDCARD` is specified. Can the `ZoneFinder`
return that?
> '''database_unittest'''
>
> - Why does it only test it with SQLite3? In general, our primary
> intent in this test .cc is to use the mock.
It tests with both the SQLite3 accessor and the mock accessor now. But in
one case alone (when testing the `DataSourceError` exception, we use the
`MockAccessor` which is arranged to throw it for some test queries).
> - It doesn't test if no-wildcard and glue-ok are really effective.
These are tested now
> - It doesn't test DNAME, CNAME, etc, cases (see the comments on the
> implementation)
These are tested now. In the case of `DNAME`, please check the test
(`below.dname.example.org` marked TODO) as it does not seem to be correct
to me (or maybe I am mistaken).
Replying to [comment:9 jinmei]:
> A few more things:
>
> - `ZoneFinder::find()` throws an exception if the queried name is "out
> of zone". Propagating it for the collection class is probably not a
> good behavior because conceptually a collection does not have to
> form a "zone". Likewise, I suspect the database version of
> `ZoneFinder::find()` could throw `DataSourceError` for various
> reasons, from lower level DB error to some kind of broken setup like
> multiple CNAMEs. What should we do in such cases, though?
> Propagating that particular exception is probably not a good idea,
> because the base class interface belongs to libdns++ and shouldn't
> know details of datasrc. But returning NULL wouldn't be good either
> (unlike the case of querying "out-of-zone" name) since this is
> indeed an "error", not necessarily that the requested RR doesn't
> exist.
This is implemented now with a `FindError` exception defined in the base
class.
> - what if the requested type is RRSIG? This case can be tricky.
> - what if it's NSEC3?
> - what if it's ANY? Or other meta types such as AXFR?
These are not handled specially right now. It's documented in the base
class.
> And, one other thing. I wonder it might make more sense to have
> `ZoneUpdater` create an appropriate `RRsetCollection`, just like it
> exposes an interface of local `ZoneFinder`. It allows specific
> updater implementations to customize the behavior, and maybe make it
> easier to support iteration.
This is done now.
Replying to [comment:10 jinmei]:
> If you choose to define `RRsetCollection` to be created from updater,
> it looks just confusing to have a separate definition for
> `datasrc::RRsetCollection`. What we should do (or at least what I
> envisioned) is to think about pros and cons of both approaches, pick
> one and only one of them and implement it.
I've moved the `datasrc::RRsetCollection` definition inside `database.cc`.
> Also, if the decision is to extend the updater, we should generally
> need more doc than this:
> {{{#!cpp
> /// Return an RRsetCollection for the updater.
> virtual isc::dns::RRsetCollectionPtr getRRsetCollection() = 0;
> }}}
> (unless it's already planned). Sometimes a one-liner brief doc is the
> best we can have, but that should be very rare except for trivial
> getter/setter.
Done.
--
Ticket URL: <https://bind10.isc.org/ticket/2435#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list