[trac1290] medium-term solution to the ld_path hack

DataSourceClient intializer now calls a helper function that modifies the datasource type, so that dlopen() always gets an absolute path
Also updated the lettuce test so that when bindctl fails when sending a command, its output is printed

configure.ac

@@ -988,6 +988,7 @@ AC_OUTPUT([doc/version.ent
            src/lib/python/bind10_config.py
            src/lib/cc/session_config.h.pre
            src/lib/cc/tests/session_unittests_config.h
+           src/lib/datasrc/datasrc_config.h
            src/lib/log/tests/console_test.sh
            src/lib/log/tests/destination_test.sh
            src/lib/log/tests/init_logger_test.sh

src/bin/bind10/bind10_src.py.in

@@ -628,32 +628,17 @@ class BoB:
         # ... and start
         return self.start_process("b10-resolver", resargs, self.c_channel_env)
 
-    def __ld_path_hack(self):
-        # XXX: a quick-hack workaround.  xfrin/out will implicitly use
-        # dynamically loadable data source modules, which will be installed in
-        # $(libdir).
-        # On some OSes (including MacOS X and *BSDs) the main process (python)
-        # cannot find the modules unless they are located in a common shared
-        # object path or a path in the (DY)LD_LIBRARY_PATH.  We should seek
-        # a cleaner solution, but for a short term workaround we specify the
-        # path here, unconditionally, and without even bothering which
-        # environment variable should be used.
-        #
-        # We reuse the ADD_LIBEXEC_PATH variable to see whether we need to
-        # do this, as the conditions that make this workaround needed are
-        # the same as for the libexec path addition
-        # TODO: Once #1292 is finished, remove this method and the special
-        # component, use it as normal component.
-        env = dict(self.c_channel_env)
-        if ADD_LIBEXEC_PATH:
-            cur_path = os.getenv('DYLD_LIBRARY_PATH')
-            cur_path = '' if cur_path is None else ':' + cur_path
-            env['DYLD_LIBRARY_PATH'] = "@@LIBDIR@@" + cur_path
+    def start_xfrout(self, c_channel_env):
+        self.start_simple("b10-xfrout", c_channel_env)
+
+    def start_xfrin(self, c_channel_env):
+        self.start_simple("b10-xfrin", c_channel_env)
+
+    def start_zonemgr(self, c_channel_env):
+        self.start_simple("b10-zonemgr", c_channel_env)
 
-            cur_path = os.getenv('LD_LIBRARY_PATH')
-            cur_path = '' if cur_path is None else ':' + cur_path
-            env['LD_LIBRARY_PATH'] = "@@LIBDIR@@" + cur_path
-        return env
+    def start_stats(self, c_channel_env):
+        self.start_simple("b10-stats", c_channel_env)
 
     def start_cmdctl(self):
         """
@@ -673,7 +658,7 @@ class BoB:
         if self.verbose:
             args += ['-v']
 
-        return self.start_process("b10-xfrin", args, self.__ld_path_hack())
+        return self.start_process("b10-xfrin", args, self.c_channel_env)
 
     def start_xfrout(self):
         # Set up the command arguments.
@@ -681,7 +666,7 @@ class BoB:
         if self.verbose:
             args += ['-v']
 
-        return self.start_process("b10-xfrout", args, self.__ld_path_hack())
+        return self.start_process("b10-xfrout", args, self.c_channel_env)
 
     def start_all_components(self):
         """

src/bin/xfrin/tests/Makefile.am

@@ -27,5 +27,6 @@ endif
 	PYTHONPATH=$(abs_top_builddir)/src/lib/dns/python/.libs:$(abs_top_builddir)/src/bin/xfrin:$(COMMON_PYTHON_PATH) \
 	TESTDATASRCDIR=$(abs_top_srcdir)/src/bin/xfrin/tests/testdata/ \
 	TESTDATAOBJDIR=$(abs_top_builddir)/src/bin/xfrin/tests/testdata/ \
+	B10_FROM_BUILD=$(abs_top_builddir) \
 	$(PYCOVERAGE_RUN) $(abs_srcdir)/$$pytest || exit ; \
 	done

src/lib/datasrc/datasrc_config.h.in

@@ -0,0 +1,15 @@
+// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#define MODULE_PATH "@prefix@/lib/"

src/lib/datasrc/factory.cc

@@ -19,6 +19,8 @@
 #include "sqlite3_accessor.h"
 #include "memory_datasrc.h"
 
+#include "datasrc_config.h"
+
 #include <datasrc/logger.h>
 
 #include <dlfcn.h>
@@ -26,6 +28,44 @@
 using namespace isc::data;
 using namespace isc::datasrc;
 
+namespace {
+// This helper function takes the 'type' string as passed to
+// the DataSourceClient container below, and, unless it
+// already specifies a specific loadable .so file, will
+// convert the short-name to the full file.
+// I.e. it will add '_ds.so' (if necessary), and prepend
+// it with an absolute path (if necessary).
+// Returns the resulting string to use with LibraryContainer.
+const std::string getDataSourceLibFile(std::string type) {
+    std::string lib_file = type;
+    if (type.length() == 0) {
+        isc_throw(DataSourceError,
+                  "DataSourceClient container called with empty type value");
+    }
+
+    // Type can be either a short name, in which case we need to
+    // append "_ds.so", or it can be a direct .so module.
+    const int ext_pos = lib_file.rfind(".so");
+    if (ext_pos == std::string::npos ||
+        ext_pos + 3 != lib_file.length()) {
+        lib_file.append("_ds.so");
+    }
+    // And if it is not an absolute path, prepend it with our
+    // module path
+    if (type[0] != '/') {
+        // When running from the build tree, we do NOT want
+        // to load the installed module
+        if (getenv("B10_FROM_BUILD")) {
+            lib_file = std::string(getenv("B10_FROM_BUILD")) +
+                       "/src/lib/datasrc/.libs/" + lib_file;
+        } else {
+            lib_file = MODULE_PATH + lib_file;
+        }
+    }
+    return lib_file;
+}
+} // end anonymous namespace
+
 namespace isc {
 namespace datasrc {
 
@@ -61,7 +101,7 @@ LibraryContainer::getSym(const char* name) {
 
 DataSourceClientContainer::DataSourceClientContainer(const std::string& type,
                                                      ConstElementPtr config)
-: ds_lib_(type + "_ds.so")
+: ds_lib_(getDataSourceLibFile(type))
 {
     // We are casting from a data to a function pointer here
     // Some compilers (rightfully) complain about that, but

src/lib/datasrc/factory.h

@@ -115,6 +115,13 @@ private:
 /// easy recognition and to reduce potential mistakes.
 /// For example, the sqlite3 implementation has the type 'sqlite3', and the
 /// derived filename 'sqlite3_ds.so'
+/// The value of type can be a specific loadable module; 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.
+/// \note When 'B10_FROM_BUILD' is set in the environment, the build
+///       directory is used instead of the install directory.
 ///
 /// There are of course some demands to an implementation, not all of which
 /// can be verified compile-time. It must provide a creator and destructor

src/lib/datasrc/tests/Makefile.am

@@ -60,7 +60,7 @@ if !USE_STATIC_LINK
 # 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.
-run_unittests_SOURCES += factory_unittest.cc
+#run_unittests_SOURCES += factory_unittest.cc
 endif
 
 run_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
@@ -110,3 +110,18 @@ EXTRA_DIST += testdata/test.sqlite3
 EXTRA_DIST += testdata/test.sqlite3.nodiffs
 EXTRA_DIST += testdata/rwtest.sqlite3
 EXTRA_DIST += testdata/diffs.sqlite3
+
+# For the factory unit tests, we need to specify that we want
+# the libraries from the build tree, and not from the installation
+# directory. Therefore we build it into a separate binary,
+# and call that from check-local with B10_FROM_BUILD set.
+# Also, we only want to do this when static building is not used.
+noinst_PROGRAMS+=run_unittests_factory
+run_unittests_factory_SOURCES = $(common_sources)
+run_unittests_factory_SOURCES += factory_unittest.cc
+run_unittests_factory_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
+run_unittests_factory_LDFLAGS  = $(AM_LDFLAGS)  $(GTEST_LDFLAGS)
+run_unittests_factory_LDADD = $(common_ldadd)
+check-local:
+	B10_FROM_BUILD=${abs_top_builddir} ./run_unittests_factory
+

src/lib/python/isc/datasrc/tests/Makefile.am

@@ -33,5 +33,6 @@ endif
 	PYTHONPATH=:$(COMMON_PYTHON_PATH):$(abs_top_builddir)/src/lib/python/isc/log:$(abs_top_builddir)/src/lib/python/isc/datasrc/.libs:$(abs_top_builddir)/src/lib/dns/python/.libs \
 	TESTDATA_PATH=$(abs_srcdir)/testdata \
 	TESTDATA_WRITE_PATH=$(abs_builddir) \
+	B10_FROM_BUILD=$(abs_top_builddir) \
 	$(PYCOVERAGE_RUN) $(abs_srcdir)/$$pytest || exit ; \
 	done

src/lib/python/isc/notify/tests/Makefile.am

@@ -29,5 +29,6 @@ endif
 	PYTHONPATH=$(COMMON_PYTHON_PATH):$(abs_top_builddir)/src/lib/dns/python/.libs \
 	$(LIBRARY_PATH_PLACEHOLDER) \
 	TESTDATASRCDIR=$(abs_top_srcdir)/src/lib/python/isc/notify/tests/testdata/ \
+	B10_FROM_BUILD=$(abs_top_builddir) \
 	$(PYCOVERAGE_RUN) $(abs_srcdir)/$$pytest || exit ; \
 	done

tests/lettuce/features/terrain/bind10_control.py

@@ -138,4 +138,7 @@ def send_command(step, command, cmdctl_port):
     bindctl.stdin.write(command + "\n")
     bindctl.stdin.write("quit\n")
     result = bindctl.wait()
-    assert result == 0, "bindctl exit code: " + str(result)
+    (stdout, stderr) = bindctl.communicate()
+    assert result == 0, "bindctl exit code: " + str(result) +\
+                        "\nstdout:\n" + str(stdout) +\
+                        "stderr:\n" + str(stderr)

tests/lettuce/features/xfrin_bind10.feature

@@ -5,6 +5,7 @@ Feature: Xfrin
     Given I have bind10 running with configuration xfrin/retransfer_master.conf with cmdctl port 47804 as master
     And I have bind10 running with configuration xfrin/retransfer_slave.conf
     A query for www.example.org should have rcode REFUSED
+    Wait for bind10 stderr message CMDCTL_STARTED
     When I send bind10 the command Xfrin retransfer example.org IN 127.0.0.1 47807
     Then wait for new bind10 stderr message XFRIN_XFR_TRANSFER_SUCCESS not XFRIN_XFR_PROCESS_FAILURE
     A query for www.example.org should have rcode NOERROR