BIND 10 #1207: Enable the data source factory

[BIND 10 Development]

#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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:15 jinmei]:
 > >
 > > 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.
 >

 crap, I tried with the wrong configure argument. As I said on jabber,
 turns out it doesn't even build on my system (looked into it a little bit
 more, looks like we need to add more explict LDADD/LIBADD to our
 makefiles. For now I think it's best to use your diff to at least get it
 working on some systems (and mainly the buildbot).

 >
 <snip>
 >
 > 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.
 >

 OK, that patch looks fine, applied it.


 > > > - 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...
 >

 applied, so moot now :)

 > > > - 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.
 >

 OK

 > > > '''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
 > }}}
 >

 fixed

 > >
 > > 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());
 >     }
 > }}}
 >

 Yes, apparently we do not misuse it there though.

 > 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).

 On that, I have been thinking that, unless we go for a more substansive
 API rehash real soon, one wild idea is that perhaps the config data
 validation code (that matches data to its spec) should actually fill in
 defaults, so that code like this would never have to take these corner
 cases into account (though direct callers that move around validators such
 as unit tests would of course need to be careful in the data they
 provide). Would lose potential information though (it may appear user-set,
 not default), but not sure if that is ever used on this end.

 For values that can be NULL due to them being optional I do not know if
 there is a real solution; if they are allowed to be null, we'll always
 have to take into account that they can be :)

 BTW, I added the branch to the build queue for a one-shot test build on
 our farm.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1207#comment:17>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development