From 89da0c604705285cb9d081a0f33e6816b5e08545 Mon Sep 17 00:00:00 2001 From: Jesper Friis Date: Sun, 20 Aug 2023 18:02:26 +0200 Subject: [PATCH] Fixed segfault in iteration over storage plugin names Also changed plugin help to be a class method. --- bindings/python/dlite-storage-python.i | 34 +++++++++++++++++-- bindings/python/dlite-storage.i | 22 +++++++++--- bindings/python/tests/CMakeLists.txt | 1 + bindings/python/tests/test_storage_plugins.py | 25 ++++++++++++++ src/dlite-storage-plugins.h | 2 +- src/dlite-storage.c | 2 +- src/utils/plugin.c | 10 ------ storages/python/dlite-plugins-python.c | 6 ++-- 8 files changed, 80 insertions(+), 22 deletions(-) create mode 100644 bindings/python/tests/test_storage_plugins.py diff --git a/bindings/python/dlite-storage-python.i b/bindings/python/dlite-storage-python.i index 1f6ce4c91..13dd1633c 100644 --- a/bindings/python/dlite-storage-python.i +++ b/bindings/python/dlite-storage-python.i @@ -8,7 +8,7 @@ # Override default __init__() def __init__(self, driver_or_url, location=None, options=None): loc = str(location) if location else None - _dlite.Instance_swiginit(self, _dlite.new_Storage( + _dlite.Storage_swiginit(self, _dlite.new_Storage( driver_or_url=driver_or_url, location=loc, options=options)) def __enter__(self): @@ -41,10 +41,25 @@ ) @classmethod - def create_from_url(cls, url): + def from_url(cls, url): """Create a new storage from `url`.""" return cls(url) + @classmethod + def load_plugins(cls): + """Load all storage plugins.""" + _dlite._load_all_storage_plugins() + + @classmethod + def unload_plugin(cls, name): + """Unload storage plugin with this name.""" + _dlite._unload_storage_plugin(str(name)) + + @classmethod + def plugin_help(cls, name): + """Return documentation of storage plogin with this name.""" + return _dlite._storage_plugin_help(name) + def instances(self, pattern=None): """Returns an iterator over all instances in storage whos metadata URI matches `pattern`.""" @@ -64,6 +79,7 @@ driver = property(get_driver, doc='Name of driver associated with this storage') + %} } @@ -77,12 +93,24 @@ %} } - %extend StoragePluginIter { %pythoncode %{ + # Override default __init__() + def __init__(self): + """Iterates over loaded storage plugins.""" + _dlite.StoragePluginIter_swiginit( + self, _dlite.new_StoragePluginIter()) + # Keep a reference to self, such that it is not garbage-collected + # before end of iterations + if not hasattr(_dlite, "_storage_plugin_iters"): + _dlite._storage_plugin_iters = {} + _dlite._storage_plugin_iters[id(self.iter)] = self + def __next__(self): name = self.next() if not name: + # Delete reference to iterator object stored away in __init__() + del _dlite._storage_plugin_iters[id(self.this)] raise StopIteration() return name %} diff --git a/bindings/python/dlite-storage.i b/bindings/python/dlite-storage.i index 3a6c31c49..f3e78d056 100644 --- a/bindings/python/dlite-storage.i +++ b/bindings/python/dlite-storage.i @@ -2,8 +2,17 @@ %{ #include "dlite.h" +#include "dlite-errors.h" #include "dlite-storage.h" #include "dlite-storage-plugins.h" + +char *_storage_plugin_help(const char *name) { + const DLiteStoragePlugin *api = dlite_storage_plugin_get(name); + if (api->help) return api->help(api); + return dlite_err(dliteUnsupportedError, + "\"%s\" storage does not support help", name), NULL; +} + %} @@ -68,8 +77,8 @@ enum _DLiteIDFlag { %feature("docstring", "\ Represents a data storage. -Parameters ----------- +Arguments +--------- driver_or_url : string Name of driver used to connect to the storage or, if `location` is not given, the URL to the storage: @@ -195,9 +204,9 @@ Iterates over loaded storage plugins. DLiteStoragePluginIter *iter; }; %} -%feature("docstring", "") new_StoragePluginIter; %extend StoragePluginIter { StoragePluginIter(void) { + //dlite_storage_plugin_load_all(); DLiteStoragePluginIter *iter = dlite_storage_plugin_iter_create(); return (struct StoragePluginIter *)iter; } @@ -218,9 +227,14 @@ Iterates over loaded storage plugins. } -%rename(storage_unload) dlite_storage_plugin_unload; +%rename(_load_all_storage_plugins) dlite_storage_plugin_load_all; +int dlite_storage_plugin_load_all(); + +%rename(_unload_storage_plugin) dlite_storage_plugin_unload; int dlite_storage_plugin_unload(const char *name); +char *_storage_plugin_help(const char *name); + /* ----------------------------------- * Target language-spesific extensions diff --git a/bindings/python/tests/CMakeLists.txt b/bindings/python/tests/CMakeLists.txt index 1d7678413..ec0a357f9 100644 --- a/bindings/python/tests/CMakeLists.txt +++ b/bindings/python/tests/CMakeLists.txt @@ -9,6 +9,7 @@ set(tests test_misc test_python_storage test_storage + test_storage_plugins test_paths test_utils test_global_dlite_state diff --git a/bindings/python/tests/test_storage_plugins.py b/bindings/python/tests/test_storage_plugins.py new file mode 100644 index 000000000..b502b5fe4 --- /dev/null +++ b/bindings/python/tests/test_storage_plugins.py @@ -0,0 +1,25 @@ +import dlite + + +# Initially no storage plugins are loaded +assert list(dlite.StoragePluginIter()) == [] + +# Storage plugins can be loaded +dlite.Storage.load_plugins() + +# Now plugins are loaded +plugins = set(dlite.StoragePluginIter()) +assert plugins +assert "json" in plugins + +# Plugins can be iterated over +print("Storage plugins") +plugins2 = set() +for name in dlite.StoragePluginIter(): + print(" -", name) + plugins2.add(name) +assert plugins2 == plugins + +# Unload json plugin +dlite.Storage.unload_plugin("json") +assert "json" not in set(dlite.StoragePluginIter()) diff --git a/src/dlite-storage-plugins.h b/src/dlite-storage-plugins.h index 2ea6f8f56..4523ea8aa 100644 --- a/src/dlite-storage-plugins.h +++ b/src/dlite-storage-plugins.h @@ -272,7 +272,7 @@ typedef int (*Flush)(DLiteStorage *s); Returns a malloc'ed string with plugin documentation or NULL on error. Optional. */ -typedef char *(*Help)(DLiteStorage *s); +typedef char *(*Help)(const DLiteStoragePlugin *api); /** @} */ diff --git a/src/dlite-storage.c b/src/dlite-storage.c index 777f44793..5d974a4c5 100644 --- a/src/dlite-storage.c +++ b/src/dlite-storage.c @@ -261,7 +261,7 @@ int dlite_storage_delete(DLiteStorage *s, const char *id) */ char *dlite_storage_help(DLiteStorage *s) { - if (s->api->help) return s->api->help(s); + if (s->api->help) return s->api->help(s->api); return err(dliteUnsupportedError, "storage does not support help: %s", s->api->name), NULL; } diff --git a/src/utils/plugin.c b/src/utils/plugin.c index 979da7030..901d96235 100644 --- a/src/utils/plugin.c +++ b/src/utils/plugin.c @@ -80,14 +80,6 @@ PluginInfo *plugin_info_create(const char *kind, const char *symbol, map_init(&info->pluginpaths); map_init(&info->apis); - printf("\n"); - const char *key; - map_iter_t iter = map_iter(&info->apis); - printf("*** iter:\n"); - while ((key = map_next(&info->apis, &iter))) - printf("*** key='%s'\n", key); - printf("*** iterdone:\n"); - return info; } @@ -422,7 +414,6 @@ char **plugin_names(const PluginInfo *info) */ void plugin_api_iter_init(PluginIter *iter, const PluginInfo *info) { - printf("*** init iter...\n"); memset(iter, 0, sizeof(PluginIter)); iter->info = info; iter->miter = map_iter(&info->apis); @@ -440,7 +431,6 @@ const PluginAPI *plugin_api_iter_next(PluginIter *iter) PluginAPI **p, *api=NULL; PluginInfo *info = (PluginInfo *)iter->info; const char *name = map_next(&info->apis, &iter->miter); - printf(" -- '%s'\n", name); if (!name) return NULL; if (!(p = map_get(&info->apis, name)) || !(api = *p)) fatal(1, "failed to get api: %s", name); diff --git a/storages/python/dlite-plugins-python.c b/storages/python/dlite-plugins-python.c index 5487f54c3..af733036b 100644 --- a/storages/python/dlite-plugins-python.c +++ b/storages/python/dlite-plugins-python.c @@ -164,17 +164,17 @@ int flusher(DLiteStorage *s) It combines the class documentation with the documentation of the open() method. */ -char *helper(DLiteStorage *s) +char *helper(const DLiteStoragePlugin *api) { PyObject *v=NULL, *pyclassdoc=NULL, *open=NULL, *pyopendoc=NULL; - PyObject *class = (PyObject *)s->api->data; + PyObject *class = (PyObject *)api->data; const char *classname, *classdoc=NULL, *opendoc=NULL; char *doc=NULL; Py_ssize_t n=0, clen=0, olen=0, i, newlines=0; dlite_errclr(); if (!(classname = dlite_pyembed_classname(class))) - dlite_warnx("cannot get class name for storage plugin '%s'", s->api->name); + dlite_warnx("cannot get class name for storage plugin '%s'", api->name); if (PyObject_HasAttrString(class, "__doc__")) { if (!(pyclassdoc = PyObject_GetAttrString(class, "__doc__")))