BIND 10 #1207: Enable the data source factory
BIND 10 Development
do-not-reply at isc.org
Thu May 17 23:31:50 UTC 2012
#1207: Enable the data source factory
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
stephen | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20120529
medium | Resolution:
Component: data | Sensitive: 0
source | Sub-Project: DNS
Keywords: | Estimated Difficulty: 5
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:14 jelte]:
> > - It's known that tests using dynamic modules don't work with static
> > link, at least not on some systems. I've not checked that myself,
> > but you may want to check it. Look for the keyword
'USE_STATIC_LINK'
> > in auth/tests/
>
> Hmm, i don't see anything fail on my system. I hope static linking does
not disable dlopen completely, does it?
> (btw, i am revisiting this comment, and now wonder whether the last
changes made actually fix this issue, or make it worse)
The current branch indeed fails in some tests with static link on my
environment. I needed to apply the attached path to make everything
pass. This is also related to the idea of non dynamic load version of
data source client container. For unit tests we may just want to use
a normal link version of well known backends without involving dynamic
loading. Unless the test itself tries to check the dyload it should
be sufficient.
> > '''auth_config.cc'''
> > - It seems we can actually even eliminate the need for
> > `MemoryDatasourceConfig` completely.
>
> oh right, good point.
>
> Moved the special case into DatasourcesConfig. This also allowed for the
removal of a few other special casing; createAuthConfigParser no longer
needs the "datasources/memory" check, and no longer needs the bool
internal argument, so the overloaded method itself isn't needed anymore.
I'll discuss this first. It looks good, and taking even a closer
look, I think we can make it even more independent from the specific
type of "memory". In fact, the part that creates the client container
is not in-memory specific except that it hardcodes "memory". Also,
I don't think we have to need the help from createAuthConfigParser()
to reject non supported types; basically the factory constructor
should be able to reject these, so we can simply try to create a
container type-independent manner and let the factory constructor
throw for unsupported ones. Right now, we still cannot completely
rely on it because it would accept "sqlite3", too, but we're almost
there, so I suggest we write the code that way and just temporarily
reject unsupported ones using hardcoded checks.
I'd also localize the RR class check. This part will soon be
generalized, and at that point there should basically be no reason for
auth to reject uncommon classes, so the check itself will unnecessary.
On a related note, the class wide `rrclass_` variable doesn't seem to
be a good one, because it applies to all possible data source clients
while in general they can be different per config item.
Taking these into account, I'd suggest the attached patch.
Functionality wise it's no different from the previous version, but I
think it makes the code even closer to the type-independent version.
> > - Do we need the cast here?
> > {{{#!cpp
> > (void)server_.getInMemoryClientContainer(rrclass_);
> > }}}
> >
>
> Not necessarily, but I tend to use that construct to show the return
value is intentionally ignored. Granted, using an old-style cast :) (tbh,
I think such use shows that we need a separate call to check for the
rrclass, btw). If you dislike it much, I can drop the cast :)
First off, recall the previous point. If we do the class check within
build(), we don't even have to "abuse" getInMemoryClientContainer in
the first place. The rest of the comment is for the case we don't do
that...
First, I agree that the real issue is to abuse a get method for the
purpose of check that would take place in that method. But I suspect
when we free datasource-type independent this particular type of check
will be gone, too, so, in that sense this is not a big issue anyway.
As for the cast itself in this specific case keeping this in my mind,
I do not insist anything special. If you like to keep a cast, please
do. In that case my only suggestion is to use a C++-cast (static_cast
in this case), but I'd leave it to you even about that.
> > - If possible (i.e, if it doesn't break other tests), I'd suggest
> > moving the RR class check in `MemoryDatasourceConfig::build()`
> > to the in-memory factory. That way we can make auth_config even
> > more type independent, and we can unify
`getInMemoryClientContainer()`
> > to `getInMemoryClient()`.
>
> The factory itself shouldn't need or want class information, so maybe I
misunderstand you here.
The in-memory factory will need to be aware of the class (see the top
of applyConfig(), for example). But...
> I think the class check itself is not needed, actually, afaict it is
currently only there because we can only set one in-memory datasource
right now, and we want it to be IN (so actually, we could simply do one
check upon first encountering the config item 'class'). But whether or not
we even check that, this might become moot anyway if we generalize it (as
the whole method that checks it right now would disappear).
you're right on this point, and also see above (in the context of my
proposed patch). So, in short, we can just forget this block of
review comment.
> > - On top of the previous one, `getInMemoryClient()` probably doesn't
> > have to throw when it's given an unsupported RR class; it can simply
> > return NULL. `LoadZoneCommand::validate()` will throw on it itself.
>
> it already does; in the current split case where we have a separate
getInMemoryClient() and getInMemoryClientContainer() it does not, but IMO
the throw there provides better error reporting than failing later on an
empty shared pointer. (but see previous point).
> > '''auth_srv_unittest'''
Just noticed now, "A the" seems to be a typo:
{{{#!cpp
/// A the possible methods to throw in, either in FakeClient or
/// FakeZoneFinder
}}}
> > '''memory_datasrc_link.cc'''
> > {{{#!cpp
> > if (!config->contains("zones")) {
> > // Assume empty list of zones
> > //addError(errors, "No 'zones' element in memory backend
config");
> > //result = false;
> > } else if (!config->get("zones") ||
> > }}}
> >
> > - in the previous code, is it possible that get() returns NULL while
> > contains doesn't?
>
> Yes there is a semantical difference; contains returns true or false
signaling whether any value with the given key exists. get() returns the
value if it exists, but the value *itself* can also be null (technically,
since zones is a non-optional value, this should never happen, but like
the below points, the way the api is and is used, it's hard to get to that
default, and i suspect we actually do misuse the flexibility in a number
of tests).
Hmm, okay, tricky. So isn't this code technically buggy, too?
{{{#!cpp
isc::dns::RRClass rrclass = RRClass::IN();
if (config_value->contains("class")) {
// This assumes get("class") is non NULL.
rrclass = RRClass(config_value->get("class")->stringValue());
}
}}}
Anyway, I'm afraid this interface is pretty easy to misuse. I don't
know how realistic it is, but I think we should revise this part of
the API (not in this ticket, though).
--
Ticket URL: <http://bind10.isc.org/ticket/1207#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list