BIND 10 #1292: Xfrin retransfer shared object "sqlite3_ds.so" not found

[BIND 10 Development]

#1292: Xfrin retransfer shared object "sqlite3_ds.so" not found
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20111206
  critical                           |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  High   |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  5      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:24 jelte]:

 First off, distcheck failed:

 {{{
 ERROR: files left in build directory after distclean:
 ./src/lib/datasrc/datasrc_config.h
 }}}

 > > > BTW, I have not checked, but if this works, i suspect it solves
 several other related tickets as well (#1385 and #1421, maybe more)
 > >
 > > For #1421, probably.  I suspect #1385 is a separate issue.
 >
 > he hasn't answered my question there yet, but i have seen this problem
 > too; when i had an older installed version that wasn't
 > binary-compatible with recent changes. Exact same symptoms, caused by
 > dlopen picking the wrong one. So if that is the case for him too, the
 > root cause is of course different (we need version check), but the
 > system should fall in that trap a lot less (theoretically, never,
 > even, unless you start mixing versions).

 I'm still not sure whether dlopen() helps #1385 - could it be an
 incompatible version of our own loadable modules?  Anyway, that's not
 a showstopper issue for this ticket.  As for #1385 itself maybe you
 should poke Kevin on jabber or give the ticket back to him.

 > > About the branch:
 > >
 > > '''general'''
 > >
 > > I'm not very happy about introducing yet another hardcode of things
 > > like B10_FROM_BUILD in our main source code. [...]
 >
 > Actually, I just don't see another way (yet? perhaps you have an
 > idea). Even if we have a configuration value, we'll still need to
 > override the default when running from source tree (either by the
 > caller or the callee, but i'd prefer to put it in as central a
 > location as possible). And not only for tests, also for run_bind10.sh.
 > I did not want to add yet another env var, and the location should
 > always be fixed from the build directory, so if we do need one, this
 > one seems best to me. This is a very similar problem to the database
 > file one.

 I've not fully thought about it either, but I can imagine something
 like this:
 - We (eventually) introduce more generic data source configuration
   where we can specify the default module path
 - We specify the installed version of module path for the installed
   version of the spec file for the data source config "module"
 - For in-source run environment, we might either provide a hacked
   version of spec file (if possible) or provide a template b10-config
   file that overrides the module path config to the in-source path
 - For unit tests, we can probably provide a utility module/helper
   library that tweaks the path and have tests that need to use
   data sources import/call it

 This is not a baked idea and may or may not be feasible, but in
 general if we can replace this kind of harcode in the core
 module/program with configuration and helper modules specific for
 tests (while reasonably avoiding duplicate setups for tests), I'd
 prefer that.

 Anyway, for the purpose of this ticket I'm okay with the current
 approach (even if the above idea makes sense it would require generic
 data source config framework, which would be way beyond the scope for
 this ticket).

 '''datasrc_config.h.in'''

 > > - Although this is independent from the topic of this ticket, I
 > >   personally think @prefix@/lib is not the right place for loadable
 > >   modules [...]
 >
 > ah, yes i asked around for better suggestions but hadn't gotten any,
 > [...]
 >
 > Oh, localstate wouldn't expand to libexec, but to var, btw. I like
 > libexec a bit more though (not that it's exec, but it's not really
 > var/ material either, and this way it is closer to the binary).
 > Anyway, I have no really strong opinion, and now that it is expanded
 > correctly, we can easily change it.

 Ah, yes, what I meant was libexec rather than localstatedir (maybe I
 incorrectly assumed the latter would expand to libexec when I made the
 comment).  The revised code on this point looks okay.

 And this means the existing modules could now be cleaned up, and to
 note that I think this ticket needs a changelog entry.

 > > '''bind10_src.py.in'''
 > >
 > > Why did you reintroduce start_zonemgr and start_stats? [...]

 > Oops. Merge error (started out in the existing branch, then figured
 > out there was no reason for that, and move changes to master. That
 > brought along some old code. Reapplying cleanly now).
 >
 > > We could leave this task to #1388 as originally planned, or complete
 > > that part within this ticket.
 >
 > Actually I prefer doing it here, so we can show that it works.

 Okay, then for the next step we should at least remove start_xfrin/out
 and update bob.spec and isc.bind10.special_component accordingly.

 I think that changes regarding this should also be noted in the
 changelog.

 '''factory.cc'''

 > > - Don't we need a test for getDataSourceLibFile?
 >
 > did we decide on a good approach for testing
 > anonymous-namespace-functions? We can of course make it a function in

 I don't even remember whether we have ever discussed this, but in
 general I don't think we can directly test things in an unnamed (aka
 "anonymous") namespace.  In this case I can think of two
 possibilities:

 - define it in something like isc::datasrc::detail (with an explicit
   note that it's only for tests) and test the function directly.
 - test the behavior indirectly, e.g., by passing a non existent path
   to DataSourceClientContainer and check the resulting what() string
   on exception.

 (If the second option isn't feasible) It would be case-by-case whether
 we want to test a particular function/method even if it would
 technically publish internal details.  I personally think the
 testability is more important in this case because we can often
 overlook some corner case errors in this type of string manipulation.

 '''datasrc/tests/Makefile.am'''

 > yes, sorry, forgot about this in my hurry to get you to take a look at
 > it :p Moved the if check and removed the old part.

 Okay, then I have some additional comments:

 - is this about "the libraries" or "modules"?
 {{{
 # For the factory unit tests, we need to specify that we want
 # the libraries from the build tree, ...
 }}}
 - also, maybe we should copy the reason why we limit this to
   !USE_STATIC_LINK from the now-removed comment:
 {{{
 # This test uses dynamically loadable module.  It will cause various
 # troubles with static link such as "missing" symbols in the static object
 # for the module.  As a workaround we disable this particualr test
 # in this case.
 }}}

 Changes I've not explicitly commented on above are okay to me.

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