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

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:28 jinmei]:
 >
 > '''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 was afraid of that :) (and it's even worse than I feared).

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

 Right. Done.

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

 ack, done

 > > Finally, here's the changelog proposal (it's a pretty long one for a
 > > relatively small change :p)
 > >
 >

 <snip>

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

 ack. I'll take your suggestions for the final entry

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

 (see also discussion in jabber, I myself have no strong preference for
 any specific name either but we should indeed be consistent)

 Oh yeah I missed a number of them, updated, also updated MODULE_PATH
 to BACKEND_LIBRARY_PATH.

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

 Replaced it with a reference to the const value in the
 datasrc_config.h file.

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