[1253] make createInstance set an error instead of raising an exception

src/lib/datasrc/factory.cc

@@ -70,15 +70,10 @@ DataSourceClientContainer::DataSourceClientContainer(const std::string& type,
     ds_creator* ds_create = (ds_creator*)ds_lib_.getSym("createInstance");
     destructor_ = (ds_destructor*)ds_lib_.getSym("destroyInstance");
 
-    // We catch and reraise copies of known-to-be-thrown exceptions
-    // Since otherwise, if the constructor fails, the stack unroll loop may
-    // want access to the then-destroyed library for info
-    try {
-        instance_ = ds_create(config);
-    } catch (const DataSourceConfigError& dsce) {
-        throw DataSourceConfigError(dsce);
-    } catch (const DataSourceError& dse) {
-        throw DataSourceError(dse);
+    std::string error;
+    instance_ = ds_create(config, error);
+    if (instance_ == NULL) {
+        isc_throw(DataSourceError, error);
     }
 }
 

src/lib/datasrc/factory.h

@@ -44,17 +44,8 @@ public:
         DataSourceError(file, line, what) {}
 };
 
-/// \brief Raised if the given config contains bad data
-///
-/// Depending on the datasource type, the configuration may differ (for
-/// instance, the sqlite3 datasource needs a database file).
-class DataSourceConfigError : public DataSourceError {
-public:
-    DataSourceConfigError(const char* file, size_t line, const char* what) :
-        DataSourceError(file, line, what) {}
-};
-
-typedef DataSourceClient* ds_creator(isc::data::ConstElementPtr config);
+typedef DataSourceClient* ds_creator(isc::data::ConstElementPtr config,
+                                     std::string& error);
 typedef void ds_destructor(DataSourceClient* instance);
 
 /// \brief Container class for dynamically loaded libraries

src/lib/datasrc/memory_datasrc.cc

@@ -931,10 +931,11 @@ checkConfig(ConstElementPtr config, ElementPtr errors) {
 } // end anonymous namespace
 
 DataSourceClient *
-createInstance(isc::data::ConstElementPtr config) {
+createInstance(isc::data::ConstElementPtr config, std::string& error) {
     ElementPtr errors(Element::createList());
     if (!checkConfig(config, errors)) {
-        isc_throw(DataSourceConfigError, errors->str());
+        error = "Configuration error: " + errors->str();
+        return (NULL);
     }
     return (new InMemoryClient());
 }

src/lib/datasrc/memory_datasrc.h

@@ -310,7 +310,8 @@ private:
 ///
 /// This configuration setup is currently under discussion and will change in
 /// the near future.
-extern "C" DataSourceClient* createInstance(isc::data::ConstElementPtr config);
+extern "C" DataSourceClient* createInstance(isc::data::ConstElementPtr config,
+                                            std::string& error);
 
 /// \brief Destroy the instance created by createInstance()
 extern "C" void destroyInstance(DataSourceClient* instance);

src/lib/datasrc/sqlite3_accessor.cc

@@ -760,15 +760,21 @@ checkConfig(ConstElementPtr config, ElementPtr errors) {
 } // end anonymous namespace
 
 DataSourceClient *
-createInstance(isc::data::ConstElementPtr config) {
+createInstance(isc::data::ConstElementPtr config, std::string& error) {
     ElementPtr errors(Element::createList());
     if (!checkConfig(config, errors)) {
-        isc_throw(DataSourceConfigError, errors->str());
+        error = std::string("Configuration error: " + errors->str());
+        return (NULL);
     }
     std::string dbfile = config->get(CONFIG_ITEM_DATABASE_FILE)->stringValue();
-    boost::shared_ptr<DatabaseAccessor> sqlite3_accessor(
-        new SQLite3Accessor(dbfile, isc::dns::RRClass::IN()));
-    return (new DatabaseClient(isc::dns::RRClass::IN(), sqlite3_accessor));
+    try {
+        boost::shared_ptr<DatabaseAccessor> sqlite3_accessor(
+            new SQLite3Accessor(dbfile, isc::dns::RRClass::IN()));
+        return (new DatabaseClient(isc::dns::RRClass::IN(), sqlite3_accessor));
+    } catch (const std::exception& exc) {
+        error = exc.what();
+        return (NULL);
+    }
 }
 
 void destroyInstance(DataSourceClient* instance) {

src/lib/datasrc/sqlite3_accessor.h

@@ -200,7 +200,8 @@ private:
 ///
 /// This configuration setup is currently under discussion and will change in
 /// the near future.
-extern "C" DataSourceClient* createInstance(isc::data::ConstElementPtr config);
+extern "C" DataSourceClient* createInstance(isc::data::ConstElementPtr config,
+                                            std::string& error);
 
 /// \brief Destroy the instance created by createInstance()
 extern "C" void destroyInstance(DataSourceClient* instance);

src/lib/datasrc/tests/factory_unittest.cc

@@ -37,43 +37,43 @@ TEST(FactoryTest, sqlite3ClientBadConfig) {
     // tests are left to the implementation-specific backends)
     ElementPtr config;
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config = Element::create("asdf");
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config = Element::createMap();
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", ElementPtr());
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", Element::create(1));
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", Element::create("FOO"));
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", Element::create("IN"));
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("database_file", ElementPtr());
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("database_file", Element::create(1));
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("database_file", Element::create("/foo/bar/doesnotexist"));
     ASSERT_THROW(DataSourceClientContainer("sqlite3", config),
-                 SQLite3Error);
+                 DataSourceError);
 
     config->set("database_file", Element::create(SQLITE_DBFILE_EXAMPLE_ORG));
     DataSourceClientContainer dsc("sqlite3", config);
@@ -100,55 +100,55 @@ TEST(FactoryTest, memoryClient) {
     // tests are left to the implementation-specific backends)
     ElementPtr config;
     ASSERT_THROW(DataSourceClientContainer client("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config = Element::create("asdf");
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config = Element::createMap();
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("type", ElementPtr());
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("type", Element::create(1));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("type", Element::create("FOO"));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("type", Element::create("memory"));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", ElementPtr());
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", Element::create(1));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", Element::create("FOO"));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("class", Element::create("IN"));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("zones", ElementPtr());
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("zones", Element::create(1));
     ASSERT_THROW(DataSourceClientContainer("memory", config),
-                 DataSourceConfigError);
+                 DataSourceError);
 
     config->set("zones", Element::createList());
     DataSourceClientContainer dsc("memory", config);

src/lib/python/isc/datasrc/client_python.cc

@@ -184,12 +184,7 @@ DataSourceClient_init(s_DataSourceClient* self, PyObject* args) {
         const string ex_what = "JSON parse error in data source configuration "
                                "data for type " +
                                string(ds_type_str) + ":" + je.what();
-        PyErr_SetString(getDataSourceException("ConfigError"), ex_what.c_str());
-        return (-1);
-    } catch (const DataSourceConfigError& dsce) {
-        const string ex_what = "Bad data source configuration data for type " +
-                               string(ds_type_str) + ":" + string(dsce.what());
-        PyErr_SetString(getDataSourceException("ConfigError"), ex_what.c_str());
+        PyErr_SetString(getDataSourceException("Error"), ex_what.c_str());
         return (-1);
     } catch (const DataSourceError& dse) {
         const string ex_what = "Failed to create DataSourceClient of type " +

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

@@ -10,9 +10,12 @@ CLEANFILES = $(abs_builddir)/rwtest.sqlite3.copied
 
 # If necessary (rare cases), explicitly specify paths to dynamic libraries
 # required by loadable python modules.
-LIBRARY_PATH_PLACEHOLDER =
+# We always add one, the location of the data source modules
+# We may want to add an API method for this to the ds factory, but that is out
+# of scope for this ticket
+LIBRARY_PATH_PLACEHOLDER = $(ENV_LIBRARY_PATH)=$(abs_top_builddir)/src/lib/datasrc/.libs
 if SET_ENV_LIBRARY_PATH
-LIBRARY_PATH_PLACEHOLDER += $(ENV_LIBRARY_PATH)=$(abs_top_builddir)/src/lib/cryptolink/.libs:$(abs_top_builddir)/src/lib/dns/.libs:$(abs_top_builddir)/src/lib/dns/python/.libs:$(abs_top_builddir)/src/lib/cc/.libs:$(abs_top_builddir)/src/lib/config/.libs:$(abs_top_builddir)/src/lib/log/.libs:$(abs_top_builddir)/src/lib/util/.libs:$(abs_top_builddir)/src/lib/exceptions/.libs:$(abs_top_builddir)/src/lib/datasrc/.libs:$$$(ENV_LIBRARY_PATH)
+LIBRARY_PATH_PLACEHOLDER += :$(abs_top_builddir)/src/lib/cryptolink/.libs:$(abs_top_builddir)/src/lib/dns/.libs:$(abs_top_builddir)/src/lib/dns/python/.libs:$(abs_top_builddir)/src/lib/cc/.libs:$(abs_top_builddir)/src/lib/config/.libs:$(abs_top_builddir)/src/lib/log/.libs:$(abs_top_builddir)/src/lib/util/.libs:$(abs_top_builddir)/src/lib/exceptions/.libs:$$$(ENV_LIBRARY_PATH)
 endif
 
 # test using command-line arguments, so use check-local target instead of TESTS

src/lib/python/isc/datasrc/tests/datasrc_test.py

@@ -68,14 +68,14 @@ class DataSrcClient(unittest.TestCase):
         self.assertRaises(TypeError, isc.datasrc.DataSourceClient, "sqlite3", 1)
         self.assertRaises(isc.datasrc.Error,
                           isc.datasrc.DataSourceClient, "foo", "{}")
-        self.assertRaises(isc.datasrc.ConfigError,
+        self.assertRaises(isc.datasrc.Error,
                           isc.datasrc.DataSourceClient, "sqlite3", "")
-        self.assertRaises(isc.datasrc.ConfigError,
+        self.assertRaises(isc.datasrc.Error,
                           isc.datasrc.DataSourceClient, "sqlite3", "{}")
-        self.assertRaises(isc.datasrc.ConfigError,
+        self.assertRaises(isc.datasrc.Error,
                           isc.datasrc.DataSourceClient, "sqlite3",
                           "{ \"foo\": 1 }")
-        self.assertRaises(isc.datasrc.ConfigError,
+        self.assertRaises(isc.datasrc.Error,
                           isc.datasrc.DataSourceClient, "memory",
                           "{ \"foo\": 1 }")