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:27 jelte]:

 > Also not fully thought out yet, but possibly a nice approach could be
 > to just modify them upon installation (or recreate them with
 > install-values at that point), so there's no confusion in-tree.

 Yes, that's one concrete form of realization of the general idea.

 > So I propose to keep the approach in the branch for now, then
 > - identify the environment variables we use
 > - discuss a final approach to remove or consolidate them
 > - implement that approach
 > And the same for the generated files and run_scripts (perhaps even in
 > one go, though that might be a bit much).

 This is fine to me.

 '''factory.cc'''

 > > > > - Don't we need a test for getDataSourceLibFile?

 > But checking the error message is a good idea, added a test. Do you
 > know if there are dlopen() implementations that form different errors?
 > (if so i'll have to modify the test a bit to first discover that error,
 > or do some form of string.startswith check, but the error a direct
 > EXPECT_EQ() gives is much nicer.

 Yeah actually that failed for my environment:
 {{{
 factory_unittest.cc:41: Failure
 Value of: error
   Actual: "dlopen(/no_such_file.so, 10): image not found"
 Expected: expected_error
 Which is: "/no_such_file.so: cannot open shared object file: No such file
 or directory"
 }}}

 I think it makes more sense to make the what() message independent
 from dlopen() implementation details, e.g.:
 {{{#!c++
         isc_throw(DataSourceLibrarySymbolError, "dlopen failed for "
         << name << ": " << dlsym_error);
 }}}

 and search for "dlopen failed for /no_such_file.so", etc.  That may
 result in some redundant message like repeating the path name, but
 assuming it's basically something "exceptional" I think it's
 acceptable.

 Also, pathtest_helper would better be named pathtestHelper per naming
 guideline.

 > Finally, here's the changelog proposal (it's a pretty long one for a
 > relatively small change :p)
 >
 > [build?] jelte
 >
 > The DataSourceClient class that dynamically loads datasource backend
 > libraries no longer provides just a .so file name to its call to
 > dlopen(), but passes it an absolute path. This means that it is no
 > longer an system implementation detail that depends on
 > [DY]LD_LIBRARY_PATH which file is chosen, should there be multiple
 > options (for instance, when test-running a new build while a different
 > version is installed).
 > These loadable modules are also no longer installed in the default
 > library path, but in a subdirectory of the libexec directory of the
 > target ($prefix/libexec/[version]/backends).
 > This also removes the need to handle b10-xfin and b10-xfrout as
 > 'special' hardcoded components, and they are now started as regular
 > components as dictated by the configuration of the boss process.

 Overall it looks okay.  I have some minor comments.

 - I think this is categorized as "bug" (for which we currently provide
   workaround, and this change is a more complete fix)
 - This is a backward incompatible change (someone who tweaks bob.spec
   will need to update it), so we'll need '*'.  We may also explicitly
   note the incompatibility.
 - I would consistently call the .so's "modules" (this changelog entry
   first calls it "library" then "module".  See also below for library
   vs module)
 - We may want to note that previously installed .so's can now be
   removed.

 BTW I think it's okay to write a long entry.  Our general consensus is
 to rather provide longer ones than helplessly concise ones like just
 "Fixed a b10-auth crash bug".

 '''factory.h'''

 I don't see the reason for changing "module" to "library" here:
 {{{#!c++
 -/// The value of type can be a specific loadable module; if it already
 ends
 +/// The value of type can be a specific loadable library; if it already
 ends
  /// with '.so', the loader will not add '_ds.so'.
  /// It may also be an absolute path; if it starts with '/', nothing is
 -/// prepended. If it does not, the loadable module will be taken from the
 -/// installation library directory.
 +/// prepended. If it does not, the loadable library will be taken from
 the
 +/// libexec/backends/ installation directory.
 }}}

 I see the motivation of not using "module" for the installation
 directory, but in this context we don't see any confusion in using
 "module".  I also believe the term (dynamic) "loadable module" is
 pretty well known (just like indicated by the "-module" LD flag).
 Besides, even if we want to avoid "module" for them they are not
 really "libraries" in their usual sense either, so I don't think the
 latter is necessarily a better choice anyway.  Same comment applies to
 tests/Makefile.am.

 Also, I'm not sure if we really want to hardcode "libexec/backends"
 here because we might change the decision of the naming/path, in which
 case we could easily just update Makefile.am and forget update this
 one.

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