From d1f1914d6c100d3ab42d352dfdf26c11bb7b22ce Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Fri, 19 Jun 2026 13:47:05 -0400 Subject: [PATCH 1/2] libnvme: pass global context to registry public APIs Make struct libnvme_global_ctx the first argument of every public libnvmf_registry_* function. Most do not use it yet, but threading it in now lets a future version log via libnvme_msg() or reach per-context state without breaking the ABI again. Each public function rejects a NULL context with -EINVAL; the internal *_instance helpers only forward it and do not re-validate. Also forward-declare the struct in registry.h so the public header is self-contained. While here, fix disconnect_all_match() in fabrics.c to take the caller's context instead of creating one per controller. It also returned -ENOMEM from a function declared bool, which coerces to true and would have disconnected controllers on an allocation failure. Signed-off-by: Martin Belanger --- fabrics.c | 12 ++- libnvme/libnvme/nvme.i | 51 +++++---- libnvme/libnvme/tests/test-registry.py | 80 ++++++++------ libnvme/src/nvme/fabrics.c | 5 +- libnvme/src/nvme/private-fabrics.h | 6 +- libnvme/src/nvme/registry.c | 39 +++++-- libnvme/src/nvme/registry.h | 25 +++-- libnvme/src/nvme/tree-fabrics.c | 3 +- libnvme/test/registry.c | 138 +++++++++++++++---------- plugins/registry/registry-nvme.c | 19 +++- 10 files changed, 245 insertions(+), 133 deletions(-) diff --git a/fabrics.c b/fabrics.c index cca4beb65a..ddf7cd4a58 100644 --- a/fabrics.c +++ b/fabrics.c @@ -902,7 +902,8 @@ int fabrics_disconnect(const char *desc, int argc, char **argv) } /* disconnect-all policy: should controller @c be torn down? */ -static bool disconnect_all_match(libnvme_ctrl_t c, const char *transport, +static bool disconnect_all_match(struct libnvme_global_ctx *ctx, + libnvme_ctrl_t c, const char *transport, const char *owner, bool force) { if (transport && strcmp(transport, libnvme_ctrl_get_transport(c))) @@ -911,7 +912,12 @@ static bool disconnect_all_match(libnvme_ctrl_t c, const char *transport, return false; if (force) return true; - return libnvmf_registry_attr_equal(libnvme_ctrl_get_name(c), + + /* + * attr_equal() returns 0 only on an exact match; a read error (<0) + * compares as "not a match", so we never disconnect on error. + */ + return libnvmf_registry_attr_equal(ctx, libnvme_ctrl_get_name(c), "owner", owner) == 0; } @@ -997,7 +1003,7 @@ int fabrics_disconnect_all(const char *desc, int argc, char **argv) libnvme_for_each_host(ctx, h) { libnvme_for_each_subsystem(h, s) { libnvme_subsystem_for_each_ctrl(s, c) { - if (!disconnect_all_match(c, cfg.transport, + if (!disconnect_all_match(ctx, c, cfg.transport, cfg.owner, cfg.force)) continue; if (libnvmf_disconnect_ctrl(c)) diff --git a/libnvme/libnvme/nvme.i b/libnvme/libnvme/nvme.i index 57182fccb6..38b78a17c2 100644 --- a/libnvme/libnvme/nvme.i +++ b/libnvme/libnvme/nvme.i @@ -504,11 +504,12 @@ PyObject *nbft_get(struct libnvme_global_ctx *ctx, const char *filename) libnvmf_free_nbft(ctx, nbft); return output; } -void registry_update(const char *device, const char *attr, const char *value) +void registry_update(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, const char *value) { int ret; - ret = libnvmf_registry_update(device, attr, value); + ret = libnvmf_registry_update(ctx, device, attr, value); if (ret == -EINVAL) PyErr_SetString(PyExc_ValueError, "invalid device or attribute name"); @@ -517,11 +518,11 @@ void registry_update(const char *device, const char *attr, const char *value) "registry_update failed: %s", strerror(-ret)); } -void registry_delete(const char *device) +void registry_delete(struct libnvme_global_ctx *ctx, const char *device) { int ret; - ret = libnvmf_registry_delete(device); + ret = libnvmf_registry_delete(ctx, device); if (ret == -EINVAL) PyErr_SetString(PyExc_ValueError, "invalid device name"); else if (ret == -ENOENT) @@ -533,13 +534,14 @@ void registry_delete(const char *device) device, strerror(-ret)); } -PyObject *registry_retrieve(const char *device, const char *attr) +PyObject *registry_retrieve(struct libnvme_global_ctx *ctx, + const char *device, const char *attr) { char *value = NULL; PyObject *str; int ret; - ret = libnvmf_registry_retrieve(device, attr, &value); + ret = libnvmf_registry_retrieve(ctx, device, attr, &value); if (ret == -ENOENT) Py_RETURN_NONE; if (ret == -EINVAL) { @@ -567,14 +569,14 @@ static void _registry_collect_device(const char *device, void *user_data) } } -PyObject *registry_devices(void) +PyObject *registry_devices(struct libnvme_global_ctx *ctx) { PyObject *list = PyList_New(0); int ret; if (!list) return NULL; - ret = libnvmf_registry_device_for_each(_registry_collect_device, list); + ret = libnvmf_registry_device_for_each(ctx, _registry_collect_device, list); if (ret < 0) { Py_DECREF(list); PyErr_Format(PyExc_OSError, @@ -599,15 +601,16 @@ static void _registry_collect_attr(const char *attr, const char *value, } } -PyObject *registry_device_attrs(const char *device) +PyObject *registry_device_attrs(struct libnvme_global_ctx *ctx, + const char *device) { PyObject *dict = PyDict_New(); int ret; if (!dict) return NULL; - ret = libnvmf_registry_attr_for_each(device, _registry_collect_attr, - dict); + ret = libnvmf_registry_attr_for_each(ctx, device, + _registry_collect_attr, dict); if (ret == -EINVAL) { Py_DECREF(dict); PyErr_SetString(PyExc_ValueError, "invalid device name"); @@ -675,19 +678,21 @@ from libnvme._exceptions import ( NotConnectedError, ) -def registry_entries(): +def registry_entries(ctx): """Yield (device, attrs) for each live registry entry. Wraps the C registry iterators: registry_devices() applies the /dev stale-entry check and registry_device_attrs() reads each entry's attributes -- all file access happens in libnvme. + ctx: nvme.GlobalCtx instance + Yields: tuple: (device, attrs) where device is the kernel device name (e.g. 'nvme3') and attrs is a dict of attribute name to value. """ - for device in registry_devices(): - yield device, registry_device_attrs(device) + for device in registry_devices(ctx): + yield device, registry_device_attrs(ctx, device) %} /*############################################################################*/ @@ -964,27 +969,35 @@ PyObject *nbft_get(struct libnvme_global_ctx *ctx, const char *filename); $action if (PyErr_Occurred()) SWIG_fail; } -void registry_update(const char *device, const char *attr, const char *value); +void registry_update(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, const char *value); + %exception registry_delete { $action if (PyErr_Occurred()) SWIG_fail; } -void registry_delete(const char *device); +void registry_delete(struct libnvme_global_ctx *ctx, + const char *device); + %exception registry_retrieve { $action if (PyErr_Occurred()) SWIG_fail; } -PyObject *registry_retrieve(const char *device, const char *attr); +PyObject *registry_retrieve(struct libnvme_global_ctx *ctx, + const char *device, const char *attr); + %exception registry_devices { $action if (PyErr_Occurred()) SWIG_fail; } -PyObject *registry_devices(void); +PyObject *registry_devices(struct libnvme_global_ctx *ctx); + %exception registry_device_attrs { $action if (PyErr_Occurred()) SWIG_fail; } -PyObject *registry_device_attrs(const char *device); +PyObject *registry_device_attrs(struct libnvme_global_ctx *ctx, + const char *device); %rename(_libnvme_first_host) libnvme_first_host; %rename(_libnvme_next_host) libnvme_next_host; diff --git a/libnvme/libnvme/tests/test-registry.py b/libnvme/libnvme/tests/test-registry.py index f2f140af78..ce7f902812 100644 --- a/libnvme/libnvme/tests/test-registry.py +++ b/libnvme/libnvme/tests/test-registry.py @@ -35,51 +35,64 @@ def _teardown_tmpdir(): class TestRegistryUpdate(unittest.TestCase): + def setUp(self): + self.ctx = nvme.GlobalCtx() def tearDown(self): - nvme.registry_delete('nvme5') + nvme.registry_delete(self.ctx, 'nvme5') + self.ctx = None def test_update_creates_entry(self): - nvme.registry_update('nvme5', 'owner', 'stas') - value = nvme.registry_retrieve('nvme5', 'owner') + nvme.registry_update(self.ctx, 'nvme5', 'owner', 'stas') + value = nvme.registry_retrieve(self.ctx, 'nvme5', 'owner') self.assertEqual(value, 'stas') def test_update_steals_ownership(self): - nvme.registry_update('nvme5', 'owner', 'nbft') - nvme.registry_update('nvme5', 'owner', 'stas') - value = nvme.registry_retrieve('nvme5', 'owner') + nvme.registry_update(self.ctx, 'nvme5', 'owner', 'nbft') + nvme.registry_update(self.ctx, 'nvme5', 'owner', 'stas') + value = nvme.registry_retrieve(self.ctx, 'nvme5', 'owner') self.assertEqual(value, 'stas') def test_update_multiple_attrs(self): - nvme.registry_update('nvme5', 'owner', 'stas') - nvme.registry_update('nvme5', 'extra', 'hello') - self.assertEqual(nvme.registry_retrieve('nvme5', 'owner'), 'stas') - self.assertEqual(nvme.registry_retrieve('nvme5', 'extra'), 'hello') + nvme.registry_update(self.ctx, 'nvme5', 'owner', 'stas') + nvme.registry_update(self.ctx, 'nvme5', 'extra', 'hello') + self.assertEqual(nvme.registry_retrieve(self.ctx, 'nvme5', 'owner'), 'stas') + self.assertEqual(nvme.registry_retrieve(self.ctx, 'nvme5', 'extra'), 'hello') class TestRegistryRetrieve(unittest.TestCase): + def setUp(self): + self.ctx = nvme.GlobalCtx() + + def tearDown(self): + self.ctx = None def test_retrieve_unregistered_returns_none(self): - value = nvme.registry_retrieve('nvme99', 'owner') + value = nvme.registry_retrieve(self.ctx, 'nvme99', 'owner') self.assertIsNone(value) def test_retrieve_missing_attr_returns_none(self): - nvme.registry_update('nvme6', 'owner', 'stas') - value = nvme.registry_retrieve('nvme6', 'nosuchattr') - nvme.registry_delete('nvme6') + nvme.registry_update(self.ctx, 'nvme6', 'owner', 'stas') + value = nvme.registry_retrieve(self.ctx, 'nvme6', 'nosuchattr') + nvme.registry_delete(self.ctx, 'nvme6') self.assertIsNone(value) class TestRegistryDelete(unittest.TestCase): + def setUp(self): + self.ctx = nvme.GlobalCtx() + + def tearDown(self): + self.ctx = None def test_delete_removes_entry(self): - nvme.registry_update('nvme7', 'owner', 'stas') - nvme.registry_delete('nvme7') - self.assertIsNone(nvme.registry_retrieve('nvme7', 'owner')) + nvme.registry_update(self.ctx, 'nvme7', 'owner', 'stas') + nvme.registry_delete(self.ctx, 'nvme7') + self.assertIsNone(nvme.registry_retrieve(self.ctx, 'nvme7', 'owner')) def test_delete_nonexistent_raises(self): with self.assertRaises(FileNotFoundError): - nvme.registry_delete('nvme99') + nvme.registry_delete(self.ctx, 'nvme99') class TestRegistryEntries(unittest.TestCase): @@ -88,50 +101,57 @@ class TestRegistryEntries(unittest.TestCase): an iterable.""" def setUp(self): - nvme.registry_update('nvme1', 'owner', 'stas') - nvme.registry_update('nvme2', 'owner', 'nbft') + self.ctx = nvme.GlobalCtx() + nvme.registry_update(self.ctx, 'nvme1', 'owner', 'stas') + nvme.registry_update(self.ctx, 'nvme2', 'owner', 'nbft') def tearDown(self): - nvme.registry_delete('nvme1') - nvme.registry_delete('nvme2') + nvme.registry_delete(self.ctx, 'nvme1') + nvme.registry_delete(self.ctx, 'nvme2') + self.ctx = None def test_entries_returns_iterable(self): - entries = list(nvme.registry_entries()) + entries = list(nvme.registry_entries(self.ctx)) # All entries are stale (no /dev/nvme* in test environment) — list is empty. self.assertIsInstance(entries, list) def test_entries_skips_stale(self): - for device, attrs in nvme.registry_entries(): + for device, attrs in nvme.registry_entries(self.ctx): self.assertTrue(os.path.exists('/dev/' + device)) -def _writer(device, owner, iterations): +def _writer(ctx, device, owner, iterations): """Child process: repeatedly update the owner attribute.""" for _ in range(iterations): - nvme.registry_update(device, 'owner', owner) + nvme.registry_update(ctx, device, 'owner', owner) class TestRegistryParallelWrites(unittest.TestCase): """Verify that concurrent writes from multiple processes do not corrupt the registry. The atomic tmp->rename write protocol must ensure the final value is always one of the two written strings.""" + def setUp(self): + self.ctx = nvme.GlobalCtx() + + def tearDown(self): + self.ctx = None @unittest.skipIf(_under_valgrind, "skipped under valgrind — covered by C test") def test_parallel_writes_no_corruption(self): nprocs = 10 owners = [f'proc{i}' for i in range(nprocs)] - nvme.registry_update('nvme10', 'owner', 'parent') + nvme.registry_update(self.ctx, 'nvme10', 'owner', 'parent') - procs = [multiprocessing.Process(target=_writer, args=('nvme10', owner, 200)) + procs = [multiprocessing.Process(target=_writer, args=(self.ctx, 'nvme10', owner, 200)) for owner in owners] for p in procs: p.start() for p in procs: p.join() - value = nvme.registry_retrieve('nvme10', 'owner') - nvme.registry_delete('nvme10') + value = nvme.registry_retrieve(self.ctx, 'nvme10', 'owner') + nvme.registry_delete(self.ctx, 'nvme10') self.assertIn(value, owners, f'corrupted value: {value!r}') diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index 277da30d00..145e4a9814 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -985,9 +985,10 @@ static void registry_update_on_connect(struct libnvme_global_ctx *ctx, int ret; if (ctx->owner) - ret = libnvmf_registry_create_instance(instance, ctx->owner); + ret = libnvmf_registry_create_instance(ctx, instance, + ctx->owner); else - ret = libnvmf_registry_delete_instance(instance); + ret = libnvmf_registry_delete_instance(ctx, instance); if (ret) libnvme_msg(ctx, LIBNVME_LOG_WARN, "nvme%d: registry update failed: %s\n", diff --git a/libnvme/src/nvme/private-fabrics.h b/libnvme/src/nvme/private-fabrics.h index 36ba950a81..a3fdb83f21 100644 --- a/libnvme/src/nvme/private-fabrics.h +++ b/libnvme/src/nvme/private-fabrics.h @@ -220,7 +220,8 @@ size_t libnvmf_get_entity_version(char *buffer, size_t bufsz); * connected controller. Called from the connect path once the kernel returns * instance=N. */ -int libnvmf_registry_create_instance(int instance, const char *owner); +int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx, + int instance, const char *owner); /** * libnvmf_registry_delete_instance - Remove the registry entry for a @@ -229,4 +230,5 @@ int libnvmf_registry_create_instance(int instance, const char *owner); * held the same instance number before it was recycled. ENOENT is silently * ignored. */ -int libnvmf_registry_delete_instance(int instance); +int libnvmf_registry_delete_instance(struct libnvme_global_ctx *ctx, + int instance); diff --git a/libnvme/src/nvme/registry.c b/libnvme/src/nvme/registry.c index 223647220e..ff65a95d25 100644 --- a/libnvme/src/nvme/registry.c +++ b/libnvme/src/nvme/registry.c @@ -349,7 +349,8 @@ static int write_device_attr(const char *device, const char *attr, * entry: instance recycling means an old nvmeN/ directory is stale by * definition. */ -int libnvmf_registry_create_instance(int instance, const char *owner) +int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx, + int instance, const char *owner) { char device[32]; @@ -362,27 +363,31 @@ int libnvmf_registry_create_instance(int instance, const char *owner) * a private attribute written via libnvmf_registry_update()) must not * leak into the new entry. Ignore errors: ENOENT is the common case. */ - libnvmf_registry_delete(device); + libnvmf_registry_delete(ctx, device); return write_device_attr(device, "owner", owner); } -int libnvmf_registry_delete_instance(int instance) +int libnvmf_registry_delete_instance(struct libnvme_global_ctx *ctx, + int instance) { char device[32]; int ret; snprintf(device, sizeof(device), "nvme%d", instance); - ret = libnvmf_registry_delete(device); + ret = libnvmf_registry_delete(ctx, device); return (ret == -ENOENT) ? 0 : ret; } -__libnvme_public int libnvmf_registry_retrieve(const char *device, +__libnvme_public int libnvmf_registry_retrieve(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, char **value) { __cleanup_free char *path = NULL; int fd, ret; + if (!ctx) + return -EINVAL; if (!device || !attr || !value) return -EINVAL; if (!valid_device(device)) @@ -401,14 +406,18 @@ __libnvme_public int libnvmf_registry_retrieve(const char *device, return ret; } -__libnvme_public int libnvmf_registry_attr_equal(const char *device, +__libnvme_public int libnvmf_registry_attr_equal(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, const char *value) { char *stored = NULL; int rc; - rc = libnvmf_registry_retrieve(device, attr, &stored); + if (!ctx) + return -EINVAL; + + rc = libnvmf_registry_retrieve(ctx, device, attr, &stored); if (rc < 0 && rc != -ENOENT) return rc; if (!stored) @@ -419,22 +428,28 @@ __libnvme_public int libnvmf_registry_attr_equal(const char *device, return rc; } -__libnvme_public int libnvmf_registry_update(const char *device, +__libnvme_public int libnvmf_registry_update(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, const char *value) { + if (!ctx) + return -EINVAL; if (!device || !valid_device(device)) return -EINVAL; return write_device_attr(device, attr, value); } -__libnvme_public int libnvmf_registry_delete(const char *device) +__libnvme_public int libnvmf_registry_delete(struct libnvme_global_ctx *ctx, + const char *device) { __cleanup_free char *path = NULL; struct dirent *de; int dir_fd, ret = 0; DIR *d; + if (!ctx) + return -EINVAL; if (!device || !valid_device(device)) return -EINVAL; @@ -464,6 +479,7 @@ __libnvme_public int libnvmf_registry_delete(const char *device) } __libnvme_public int libnvmf_registry_device_for_each( + struct libnvme_global_ctx *ctx, void (*callback)(const char *device, void *user_data), void *user_data) { @@ -471,6 +487,8 @@ __libnvme_public int libnvmf_registry_device_for_each( struct dirent *de; DIR *d; + if (!ctx) + return -EINVAL; if (!callback) return -EINVAL; @@ -516,6 +534,7 @@ __libnvme_public int libnvmf_registry_device_for_each( } __libnvme_public int libnvmf_registry_attr_for_each( + struct libnvme_global_ctx *ctx, const char *device, void (*callback)(const char *attr, const char *value, void *user_data), @@ -526,6 +545,8 @@ __libnvme_public int libnvmf_registry_attr_for_each( int dir_fd; DIR *d; + if (!ctx) + return -EINVAL; if (!device || !callback) return -EINVAL; if (!valid_device(device)) diff --git a/libnvme/src/nvme/registry.h b/libnvme/src/nvme/registry.h index 48129b468a..49ea5c0972 100644 --- a/libnvme/src/nvme/registry.h +++ b/libnvme/src/nvme/registry.h @@ -24,8 +24,11 @@ * orchestrator by another. */ +struct libnvme_global_ctx; + /** * libnvmf_registry_retrieve() - Read an attribute from a controller's registry entry + * @ctx: libnvme global context; must not be NULL * @device: Kernel device name (e.g. "nvme3") * @attr: Attribute name to retrieve (e.g. "owner") * @value: On success, set to a newly allocated string with the attribute @@ -35,11 +38,13 @@ * attribute is not present, -ENOMEM on allocation failure, negative errno * from underlying system calls otherwise. */ -int libnvmf_registry_retrieve(const char *device, const char *attr, - char **value); +int libnvmf_registry_retrieve(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, + char **value); /** * libnvmf_registry_attr_equal() - Compare a registry attribute to a value + * @ctx: libnvme global context; must not be NULL * @device: Kernel device name (e.g. "nvme3") * @attr: Attribute name to compare (e.g. "owner") * @value: Value to compare against; NULL means "attribute absent" @@ -47,11 +52,13 @@ int libnvmf_registry_retrieve(const char *device, const char *attr, * Return: 0 if @attr equals @value (a missing attribute equals a NULL * @value), >0 if it differs, negative errno on a read error. */ -int libnvmf_registry_attr_equal(const char *device, const char *attr, +int libnvmf_registry_attr_equal(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, const char *value); /** * libnvmf_registry_update() - Update an attribute in a controller's registry entry + * @ctx: libnvme global context; must not be NULL * @device: Kernel device name (e.g. "nvme3") * @attr: Attribute name to update (e.g. "owner") * @value: New attribute value, or NULL to remove the attribute file @@ -62,11 +69,13 @@ int libnvmf_registry_attr_equal(const char *device, const char *attr, * * Return: 0 on success, negative errno from underlying system calls otherwise. */ -int libnvmf_registry_update(const char *device, const char *attr, - const char *value); +int libnvmf_registry_update(struct libnvme_global_ctx *ctx, + const char *device, const char *attr, + const char *value); /** * libnvmf_registry_delete() - Remove a controller's registry entry + * @ctx: libnvme global context; must not be NULL * @device: Kernel device name (e.g. "nvme3") * * Removes the registry directory and all attribute files for @device. Called @@ -77,10 +86,11 @@ int libnvmf_registry_update(const char *device, const char *attr, * Return: 0 on success, -ENOENT if no entry exists, negative errno from * underlying system calls otherwise. */ -int libnvmf_registry_delete(const char *device); +int libnvmf_registry_delete(struct libnvme_global_ctx *ctx, const char *device); /** * libnvmf_registry_device_for_each() - Iterate over live controller registry entries + * @ctx: libnvme global context; must not be NULL * @callback: Callback invoked for each live entry * @user_data: User data passed to @callback * @@ -94,11 +104,13 @@ int libnvmf_registry_delete(const char *device); * opened. Returns 0 when the directory does not exist (nothing registered). */ int libnvmf_registry_device_for_each( + struct libnvme_global_ctx *ctx, void (*callback)(const char *device, void *user_data), void *user_data); /** * libnvmf_registry_attr_for_each() - Iterate over attributes in a controller's registry entry + * @ctx: libnvme global context; must not be NULL * @device: Kernel device name (e.g. "nvme3") * @callback: Callback invoked for each attribute * @user_data: User data passed to @callback @@ -111,6 +123,7 @@ int libnvmf_registry_device_for_each( * time of the initial open, negative errno otherwise. */ int libnvmf_registry_attr_for_each( + struct libnvme_global_ctx *ctx, const char *device, void (*callback)(const char *attr, const char *value, void *user_data), diff --git a/libnvme/src/nvme/tree-fabrics.c b/libnvme/src/nvme/tree-fabrics.c index 79cd17ef5c..0d0ccffdda 100644 --- a/libnvme/src/nvme/tree-fabrics.c +++ b/libnvme/src/nvme/tree-fabrics.c @@ -443,7 +443,8 @@ __libnvme_public char *libnvme_ctrl_owner(libnvme_ctrl_t c) { char *owner = NULL; - libnvmf_registry_retrieve(libnvme_ctrl_get_name(c), "owner", &owner); + libnvmf_registry_retrieve(c->ctx, + libnvme_ctrl_get_name(c), "owner", &owner); return owner; } diff --git a/libnvme/test/registry.c b/libnvme/test/registry.c index cd9e20e86e..4d47a0ad44 100644 --- a/libnvme/test/registry.c +++ b/libnvme/test/registry.c @@ -20,10 +20,12 @@ #include #include +#include #include /* Internal — not in registry.h; called from the connect path in production. */ -int libnvmf_registry_create_instance(int instance, const char *owner); +int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx, + int instance, const char *owner); static char tmpdir[256]; @@ -38,7 +40,7 @@ static void setup_tmpdir(void) setenv("NVME_REGISTRY_DIR", tmpdir, 1); } -static void cleanup_tmpdir(void) +static void cleanup_tmpdir(struct libnvme_global_ctx *ctx) { /* * Remove any remaining device directories, then the tmpdir itself. @@ -52,13 +54,13 @@ static void cleanup_tmpdir(void) return; while ((de = readdir(d)) != NULL) { if (de->d_name[0] != '.') - libnvmf_registry_delete(de->d_name); + libnvmf_registry_delete(ctx, de->d_name); } closedir(d); rmdir(tmpdir); } -static bool test_create(void) +static bool test_create(struct libnvme_global_ctx *ctx) { char *value = NULL; bool pass = true; @@ -66,13 +68,13 @@ static bool test_create(void) printf("test_create:\n"); - ret = libnvmf_registry_create_instance(3, "stas"); + ret = libnvmf_registry_create_instance(ctx, 3, "stas"); if (ret) { printf(" - create returned %d [FAIL]\n", ret); return false; } - ret = libnvmf_registry_retrieve("nvme3", "owner", &value); + ret = libnvmf_registry_retrieve(ctx, "nvme3", "owner", &value); if (ret || !value) { printf(" - retrieve after create returned %d [FAIL]\n", ret); pass = false; @@ -87,11 +89,12 @@ static bool test_create(void) out: free(value); - libnvmf_registry_delete("nvme3"); + libnvmf_registry_delete(ctx, "nvme3"); + return pass; } -static bool test_update_and_retrieve(void) +static bool test_update_and_retrieve(struct libnvme_global_ctx *ctx) { char *value = NULL; bool pass = true; @@ -100,13 +103,13 @@ static bool test_update_and_retrieve(void) printf("test_update_and_retrieve:\n"); /* Create entry via update (no prior entry). */ - ret = libnvmf_registry_update("nvme5", "owner", "nbft"); + ret = libnvmf_registry_update(ctx, "nvme5", "owner", "nbft"); if (ret) { printf(" - initial update returned %d [FAIL]\n", ret); return false; } - ret = libnvmf_registry_retrieve("nvme5", "owner", &value); + ret = libnvmf_registry_retrieve(ctx, "nvme5", "owner", &value); if (ret || !value || strcmp(value, "nbft") != 0) { printf(" - expected 'nbft', got '%s' ret=%d [FAIL]\n", value ? value : "(null)", ret); @@ -118,14 +121,14 @@ static bool test_update_and_retrieve(void) value = NULL; /* Steal ownership. */ - ret = libnvmf_registry_update("nvme5", "owner", "stas"); + ret = libnvmf_registry_update(ctx, "nvme5", "owner", "stas"); if (ret) { printf(" - steal update returned %d [FAIL]\n", ret); pass = false; goto out; } - ret = libnvmf_registry_retrieve("nvme5", "owner", &value); + ret = libnvmf_registry_retrieve(ctx, "nvme5", "owner", &value); if (ret || !value || strcmp(value, "stas") != 0) { printf(" - expected 'stas' after steal, got '%s' ret=%d [FAIL]\n", value ? value : "(null)", ret); @@ -136,11 +139,11 @@ static bool test_update_and_retrieve(void) out: free(value); - libnvmf_registry_delete("nvme5"); + libnvmf_registry_delete(ctx, "nvme5"); return pass; } -static bool test_delete(void) +static bool test_delete(struct libnvme_global_ctx *ctx) { char *value = NULL; bool pass = true; @@ -148,19 +151,19 @@ static bool test_delete(void) printf("test_delete:\n"); - ret = libnvmf_registry_update("nvme7", "owner", "stas"); + ret = libnvmf_registry_update(ctx, "nvme7", "owner", "stas"); if (ret) { printf(" - setup update failed: %d [FAIL]\n", ret); return false; } - ret = libnvmf_registry_delete("nvme7"); + ret = libnvmf_registry_delete(ctx, "nvme7"); if (ret) { printf(" - delete returned %d [FAIL]\n", ret); return false; } - ret = libnvmf_registry_retrieve("nvme7", "owner", &value); + ret = libnvmf_registry_retrieve(ctx, "nvme7", "owner", &value); if (ret != -ENOENT) { printf(" - expected -ENOENT after delete, got %d [FAIL]\n", ret); pass = false; @@ -171,7 +174,7 @@ static bool test_delete(void) free(value); /* Deleting a non-existent entry must return -ENOENT. */ - ret = libnvmf_registry_delete("nvme7"); + ret = libnvmf_registry_delete(ctx, "nvme7"); if (ret != -ENOENT) { printf(" - double-delete expected -ENOENT, got %d [FAIL]\n", ret); pass = false; @@ -182,14 +185,14 @@ static bool test_delete(void) return pass; } -static bool test_retrieve_missing(void) +static bool test_retrieve_missing(struct libnvme_global_ctx *ctx) { char *value = NULL; int ret; printf("test_retrieve_missing:\n"); - ret = libnvmf_registry_retrieve("nvme99", "owner", &value); + ret = libnvmf_registry_retrieve(ctx, "nvme99", "owner", &value); free(value); if (ret != -ENOENT) { @@ -214,7 +217,7 @@ static void collect_device(const char *device, void *user_data) "%s", device); } -static bool test_device_for_each(void) +static bool test_device_for_each(struct libnvme_global_ctx *ctx) { struct for_each_result result = { .count = 0 }; bool pass = true; @@ -227,13 +230,13 @@ static bool test_device_for_each(void) * node is absent, so both will be skipped here — we are only testing * that the function runs without error and skips gracefully. */ - if (libnvmf_registry_update("nvme1", "owner", "stas") || - libnvmf_registry_update("nvme2", "owner", "nbft")) { + if (libnvmf_registry_update(ctx, "nvme1", "owner", "stas") || + libnvmf_registry_update(ctx, "nvme2", "owner", "nbft")) { printf(" - setup update failed [FAIL]\n"); return false; } - ret = libnvmf_registry_device_for_each(collect_device, &result); + ret = libnvmf_registry_device_for_each(ctx, collect_device, &result); if (ret) { printf(" - for_each returned %d [FAIL]\n", ret); pass = false; @@ -242,8 +245,8 @@ static bool test_device_for_each(void) result.count); } - libnvmf_registry_delete("nvme1"); - libnvmf_registry_delete("nvme2"); + libnvmf_registry_delete(ctx, "nvme1"); + libnvmf_registry_delete(ctx, "nvme2"); return pass; } @@ -264,7 +267,7 @@ static void collect_attr(const char *attr, const char *value, void *user_data) } } -static bool test_attr_for_each(void) +static bool test_attr_for_each(struct libnvme_global_ctx *ctx) { struct attr_result result = { .count = 0 }; bool pass = true; @@ -272,14 +275,15 @@ static bool test_attr_for_each(void) printf("test_attr_for_each:\n"); - if (libnvmf_registry_update("nvme4", "owner", "stas") || - libnvmf_registry_update("nvme4", "extra", "hello")) { + if (libnvmf_registry_update(ctx, "nvme4", "owner", "stas") || + libnvmf_registry_update(ctx, "nvme4", "extra", "hello")) { printf(" - setup update failed [FAIL]\n"); pass = false; goto out; } - ret = libnvmf_registry_attr_for_each("nvme4", collect_attr, &result); + ret = libnvmf_registry_attr_for_each(ctx, "nvme4", collect_attr, + &result); if (ret) { printf(" - attr_for_each returned %d [FAIL]\n", ret); pass = false; @@ -293,7 +297,8 @@ static bool test_attr_for_each(void) } /* attr_for_each on non-existent device must return -ENOENT. */ - ret = libnvmf_registry_attr_for_each("nvme99", collect_attr, &result); + ret = libnvmf_registry_attr_for_each(ctx, "nvme99", collect_attr, + &result); if (ret != -ENOENT) { printf(" - missing device expected -ENOENT, got %d [FAIL]\n", ret); pass = false; @@ -302,11 +307,12 @@ static bool test_attr_for_each(void) } out: - libnvmf_registry_delete("nvme4"); + libnvmf_registry_delete(ctx, "nvme4"); + return pass; } -static bool test_null_args(void) +static bool test_null_args(struct libnvme_global_ctx *ctx) { char *value = NULL; bool pass = true; @@ -325,32 +331,46 @@ static bool test_null_args(void) } \ } while (0) + /* NULL ctx is rejected by every public API */ + CHECK(libnvmf_registry_retrieve(NULL, "nvme3", "owner", &value), + -EINVAL, "retrieve(NULL ctx)"); + CHECK(libnvmf_registry_attr_equal(NULL, "nvme3", "owner", "stas"), + -EINVAL, "attr_equal(NULL ctx)"); + CHECK(libnvmf_registry_update(NULL, "nvme3", "owner", "stas"), + -EINVAL, "update(NULL ctx)"); + CHECK(libnvmf_registry_delete(NULL, "nvme3"), + -EINVAL, "delete(NULL ctx)"); + CHECK(libnvmf_registry_device_for_each(NULL, collect_device, NULL), + -EINVAL, "device_for_each(NULL ctx)"); + CHECK(libnvmf_registry_attr_for_each(NULL, "nvme3", collect_attr, NULL), + -EINVAL, "attr_for_each(NULL ctx)"); + /* retrieve: any NULL parameter */ - CHECK(libnvmf_registry_retrieve(NULL, "owner", &value), + CHECK(libnvmf_registry_retrieve(ctx, NULL, "owner", &value), -EINVAL, "retrieve(NULL, attr, &value)"); - CHECK(libnvmf_registry_retrieve("nvme3", NULL, &value), + CHECK(libnvmf_registry_retrieve(ctx, "nvme3", NULL, &value), -EINVAL, "retrieve(device, NULL, &value)"); - CHECK(libnvmf_registry_retrieve("nvme3", "owner", NULL), + CHECK(libnvmf_registry_retrieve(ctx, "nvme3", "owner", NULL), -EINVAL, "retrieve(device, attr, NULL)"); /* update: NULL device or NULL attr; NULL value is remove-attr, not an error */ - CHECK(libnvmf_registry_update(NULL, "owner", "stas"), + CHECK(libnvmf_registry_update(ctx, NULL, "owner", "stas"), -EINVAL, "update(NULL, attr, value)"); - CHECK(libnvmf_registry_update("nvme3", NULL, "stas"), + CHECK(libnvmf_registry_update(ctx, "nvme3", NULL, "stas"), -EINVAL, "update(device, NULL, value)"); /* delete: NULL device */ - CHECK(libnvmf_registry_delete(NULL), + CHECK(libnvmf_registry_delete(ctx, NULL), -EINVAL, "delete(NULL)"); /* device_for_each: NULL callback */ - CHECK(libnvmf_registry_device_for_each(NULL, NULL), + CHECK(libnvmf_registry_device_for_each(ctx, NULL, NULL), -EINVAL, "device_for_each(NULL, user_data)"); /* attr_for_each: NULL device or NULL callback */ - CHECK(libnvmf_registry_attr_for_each(NULL, collect_attr, NULL), + CHECK(libnvmf_registry_attr_for_each(ctx, NULL, collect_attr, NULL), -EINVAL, "attr_for_each(NULL, cback, user_data)"); - CHECK(libnvmf_registry_attr_for_each("nvme3", NULL, NULL), + CHECK(libnvmf_registry_attr_for_each(ctx, "nvme3", NULL, NULL), -EINVAL, "attr_for_each(device, NULL, user_data)"); #undef CHECK @@ -365,7 +385,7 @@ static bool test_null_args(void) * concurrently. The value read after all exit must be one of the written * values — never a partial or garbled string. */ -static bool test_parallel_writes(void) +static bool test_parallel_writes(struct libnvme_global_ctx *ctx) { char *value = NULL; int status; @@ -374,7 +394,7 @@ static bool test_parallel_writes(void) printf("test_parallel_writes:\n"); - libnvmf_registry_update("nvme10", "owner", "parent"); + libnvmf_registry_update(ctx, "nvme10", "owner", "parent"); #define NPROCS 10 pid_t pids[NPROCS]; @@ -386,7 +406,8 @@ static bool test_parallel_writes(void) if (pids[i] == 0) { snprintf(owner, sizeof(owner), "child%d", i); for (int j = 0; j < 200; j++) - libnvmf_registry_update("nvme10", "owner", owner); + libnvmf_registry_update(ctx, "nvme10", "owner", + owner); exit(0); } } @@ -394,7 +415,7 @@ static bool test_parallel_writes(void) for (i = 0; i < NPROCS; i++) waitpid(pids[i], &status, 0); - libnvmf_registry_retrieve("nvme10", "owner", &value); + libnvmf_registry_retrieve(ctx, "nvme10", "owner", &value); pass = false; for (i = 0; i < NPROCS; i++) { @@ -412,26 +433,31 @@ static bool test_parallel_writes(void) value ? value : "(null)"); free(value); - libnvmf_registry_delete("nvme10"); + libnvmf_registry_delete(ctx, "nvme10"); return pass; } int main(int argc, char *argv[]) { + struct libnvme_global_ctx *ctx; bool pass = true; + ctx = libnvme_create_global_ctx(stdout, LIBNVME_LOG_DEBUG_VERBOSE); + setup_tmpdir(); - pass &= test_create(); - pass &= test_update_and_retrieve(); - pass &= test_delete(); - pass &= test_retrieve_missing(); - pass &= test_device_for_each(); - pass &= test_attr_for_each(); - pass &= test_null_args(); - pass &= test_parallel_writes(); + pass &= test_create(ctx); + pass &= test_update_and_retrieve(ctx); + pass &= test_delete(ctx); + pass &= test_retrieve_missing(ctx); + pass &= test_device_for_each(ctx); + pass &= test_attr_for_each(ctx); + pass &= test_null_args(ctx); + pass &= test_parallel_writes(ctx); + + cleanup_tmpdir(ctx); - cleanup_tmpdir(); + libnvme_free_global_ctx(ctx); fflush(stdout); exit(pass ? EXIT_SUCCESS : EXIT_FAILURE); diff --git a/plugins/registry/registry-nvme.c b/plugins/registry/registry-nvme.c index 9898a46815..8603e65e2e 100644 --- a/plugins/registry/registry-nvme.c +++ b/plugins/registry/registry-nvme.c @@ -33,13 +33,15 @@ static void print_attr(const char *attr, const char *value, void *user_data) static void print_device(const char *device, void *user_data) { + struct libnvme_global_ctx *ctx = user_data; printf("%s\n", device); - libnvmf_registry_attr_for_each(device, print_attr, NULL); + libnvmf_registry_attr_for_each(ctx, device, print_attr, NULL); } static int registry_list(int argc, char **argv, struct command *acmd, struct plugin *plugin) { + __cleanup_nvme_global_ctx struct libnvme_global_ctx *ctx = NULL; const char *desc = "List all live NVMeoF controller ownership registry entries."; NVME_ARGS(opts); @@ -47,12 +49,14 @@ static int registry_list(int argc, char **argv, struct command *acmd, if (argconfig_parse(argc, argv, desc, opts)) return -EINVAL; - return libnvmf_registry_device_for_each(print_device, NULL); + ctx = libnvme_create_global_ctx(stdout, LIBNVME_DEFAULT_LOGLEVEL); + return libnvmf_registry_device_for_each(ctx, print_device, ctx); } static int registry_retrieve(int argc, char **argv, struct command *acmd, struct plugin *plugin) { + __cleanup_nvme_global_ctx struct libnvme_global_ctx *ctx = NULL; const char *desc = "Read an attribute from a controller's registry entry."; const char *device_help = "NVMe device name (e.g. nvme3)"; const char *attr_help = "attribute name (default: owner)"; @@ -80,7 +84,8 @@ static int registry_retrieve(int argc, char **argv, struct command *acmd, } strip_dev_prefix(&cfg.device); - ret = libnvmf_registry_retrieve(cfg.device, cfg.attr, &value); + ctx = libnvme_create_global_ctx(stdout, LIBNVME_DEFAULT_LOGLEVEL); + ret = libnvmf_registry_retrieve(ctx, cfg.device, cfg.attr, &value); if (ret == -ENOENT) { fprintf(stderr, "%s: not registered or '%s' not found\n", cfg.device, cfg.attr); @@ -97,6 +102,7 @@ static int registry_retrieve(int argc, char **argv, struct command *acmd, static int registry_update(int argc, char **argv, struct command *acmd, struct plugin *plugin) { + __cleanup_nvme_global_ctx struct libnvme_global_ctx *ctx = NULL; const char *desc = "Write an attribute to a controller's registry entry. The 'owner' attribute is immutable and cannot be changed."; const char *device_help = "NVMe device name (e.g. nvme3)"; const char *attr_help = "attribute name (e.g. note); 'owner' is not allowed"; @@ -132,7 +138,8 @@ static int registry_update(int argc, char **argv, struct command *acmd, strip_dev_prefix(&cfg.device); - ret = libnvmf_registry_update(cfg.device, cfg.attr, cfg.value); + ctx = libnvme_create_global_ctx(stdout, LIBNVME_DEFAULT_LOGLEVEL); + ret = libnvmf_registry_update(ctx, cfg.device, cfg.attr, cfg.value); if (ret) fprintf(stderr, "update failed: %s\n", libnvme_strerror(-ret)); return ret; @@ -141,6 +148,7 @@ static int registry_update(int argc, char **argv, struct command *acmd, static int registry_delete(int argc, char **argv, struct command *acmd, struct plugin *plugin) { + __cleanup_nvme_global_ctx struct libnvme_global_ctx *ctx = NULL; const char *desc = "Remove a controller's registry entry."; const char *device_help = "NVMe device name (e.g. nvme3)"; int ret; @@ -164,7 +172,8 @@ static int registry_delete(int argc, char **argv, struct command *acmd, } strip_dev_prefix(&cfg.device); - ret = libnvmf_registry_delete(cfg.device); + ctx = libnvme_create_global_ctx(stdout, LIBNVME_DEFAULT_LOGLEVEL); + ret = libnvmf_registry_delete(ctx, cfg.device); if (ret) fprintf(stderr, "%s: %s\n", cfg.device, libnvme_strerror(-ret)); return ret; From fb26991b1e07bfca270f56bd020de9d1437b0f05 Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Fri, 19 Jun 2026 14:17:33 -0400 Subject: [PATCH 2/2] libnvme-python: make registry python test robust to spawn/forkserver The python-registry parallel-writes test only passed under the "fork" multiprocessing start method. Python 3.14 makes "forkserver" the default on Linux, where the test failed two ways: - Each child re-imports the test module, which unconditionally created a fresh NVME_REGISTRY_DIR. The children then wrote to a directory the parent never reads, so the final value came back as the parent's initial write. - The libnvme context was passed as a Process argument. A SwigPyObject cannot be pickled for spawn/forkserver, and a context is not meant to be shared across processes anyway. Reuse an inherited /tmp registry directory instead of always creating a new one, and have each writer process create its own GlobalCtx. Verified under fork, forkserver and spawn. Signed-off-by: Martin Belanger --- libnvme/libnvme/tests/test-registry.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/libnvme/libnvme/tests/test-registry.py b/libnvme/libnvme/tests/test-registry.py index ce7f902812..9c57e8ee8a 100644 --- a/libnvme/libnvme/tests/test-registry.py +++ b/libnvme/libnvme/tests/test-registry.py @@ -18,8 +18,16 @@ # dir='/tmp' is required: libnvme confines NVME_REGISTRY_DIR to /tmp, so the # test directory must live there (not under an arbitrary $TMPDIR). -_tmpdir = tempfile.mkdtemp(prefix='nvme-registry-test-', dir='/tmp') -os.environ['NVME_REGISTRY_DIR'] = _tmpdir +# +# Reuse an inherited /tmp directory instead of always creating a new one: with +# the spawn/forkserver start methods (the default on Linux as of Python 3.14) +# each child re-imports this module. The child inherits NVME_REGISTRY_DIR from +# the parent, so it must reuse that path rather than create a second, unrelated +# registry directory it would then write into alone. +_tmpdir = os.environ.get('NVME_REGISTRY_DIR', '') +if not _tmpdir.startswith('/tmp/'): + _tmpdir = tempfile.mkdtemp(prefix='nvme-registry-test-', dir='/tmp') + os.environ['NVME_REGISTRY_DIR'] = _tmpdir from libnvme import nvme # noqa: E402 (import after env var set intentionally) @@ -120,8 +128,15 @@ def test_entries_skips_stale(self): self.assertTrue(os.path.exists('/dev/' + device)) -def _writer(ctx, device, owner, iterations): - """Child process: repeatedly update the owner attribute.""" +def _writer(device, owner, iterations): + """Child process: repeatedly update the owner attribute. + + Each process creates its own GlobalCtx. A libnvme context must not be + shared across a process boundary: passing it as a Process argument is not + picklable under the spawn/forkserver start methods, and even under fork a + context is not designed to be used concurrently from two processes. + """ + ctx = nvme.GlobalCtx() for _ in range(iterations): nvme.registry_update(ctx, device, 'owner', owner) @@ -143,7 +158,7 @@ def test_parallel_writes_no_corruption(self): nvme.registry_update(self.ctx, 'nvme10', 'owner', 'parent') - procs = [multiprocessing.Process(target=_writer, args=(self.ctx, 'nvme10', owner, 200)) + procs = [multiprocessing.Process(target=_writer, args=('nvme10', owner, 200)) for owner in owners] for p in procs: p.start()