Repository navigation
libnvmf: various fixes - #4157
Conversation
igaw
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The public API documentation incorrectly guarantees a seqnum stamp that the implementation explicitly treats as best-effort.
1 open finding
What changed in this PR
Fixes libnvmf registry ownership, duplicate loop-controller discovery, and JSON output handling.
Changes:
- Preserves ownership when reusing existing connections.
- Atomically creates registry entries with tests.
- Correctly represents duplicate loop controllers and missing addresses.
| File | Description |
|---|---|
tests/cli/nvme_fabrics_mock_test.py |
Tests reused-controller ownership. |
src/nvme-print-json.c |
Handles missing controller addresses. |
libnvme/tests/test-fabrics.c |
Updates ownership action tests. |
libnvme/tests/sysfs/meson.build |
Registers duplicate-loop fixture. |
libnvme/tests/sysfs/data/tree-loop-duplicate.tar.xz |
Provides duplicate-loop sysfs data. |
libnvme/tests/sysfs/data/tree-loop-duplicate.out |
Defines expected topology. |
libnvme/tests/sysfs/data/tree-loop-duplicate-hostnqn.out |
Defines expected host resolution. |
libnvme/tests/registry.c |
Tests atomic registry creation. |
libnvme/src/nvme/tree.c |
Prevents incorrect controller reuse. |
libnvme/src/nvme/registry.h |
Documents update semantics. |
libnvme/src/nvme/registry.c |
Implements atomic entry creation. |
libnvme/src/nvme/fabrics.c |
Preserves ownership on reconnect. |
libnvme/design/REGISTRY.md |
Updates registry design guidance. |
Documentation/nvme-connect.txt |
Updates connect ownership documentation. |
Documentation/nvme-connect-all.txt |
Updates connect-all ownership documentation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Commits 1 and 3 look good to me. Two points on the others. Commit 2 and #4156 #4156 is merged. obj_add_str(o, "Address", address ? address : "");Commit 4 breaks what nvme-discoverd relies on nvme-discoverd has no other way to claim a controller. Its units run
libblockdev #1213 does not need this commit. It does not call nvme-discoverd and nvme-stas both claim an existing controller only when it has no owner. Neither takes a controller from another owner. Only an explicit Could you drop commit 4? If the rule for an explicit |
libnvme_ctrl_alloc() looks for an existing controller object matching the transport parameters of the controller being scanned. Controllers which match but carry a different name are skipped and the search continues. When the search ends without a result, the last skipped controller was used anyway: if (!c) c = p; This was harmless as long as the lookup created a new controller when no match was found, so that c was never NULL. Since the lookup only finds existing controllers, the fallback reuses and renames the object of a different controller and the create path below is never reached. As a result, multiple connections with identical parameters, e.g. loop controllers or controllers connected with --duplicate-connect, collapse into a single controller object. Only one of them is visible in the tree and 'nvme disconnect -n <nqn>' leaves the others connected. Drop the fallback so that a new controller object is created instead. Add a sysfs fixture with two identical loop controllers. Signed-off-by: Daniel Wagner <wagi@monom.org>
Controllers which do not report an address, e.g. loop controllers, return NULL from libnvme_ctrl_get_address() and the JSON printers emit "Address":null, while other missing sysfs attributes are printed as an empty string. Add obj_add_ctrl_address() which prints an empty string in this case and use it for all controller addresses. Signed-off-by: Daniel Wagner <wagi@monom.org>
Entries written on connect are built in a temporary directory together with the seqnum stamp the udev REMOVE rule relies on and renamed into place. libnvmf_registry_update() created a missing entry with a plain mkdir() instead, so the entry had no stamp and was briefly visible without any attribute. An orchestrator registering a controller it connected through the public API therefore lost the protection against a recycled instance number. Factor out the creation of an entry from libnvmf_registry_create_instance() and use it in libnvmf_registry_update() when the entry does not exist yet. Removing an attribute of a missing entry no longer creates an empty entry. Signed-off-by: Daniel Wagner <wagi@monom.org>
fe8d2b0 to
bad2691
Compare
While porting libblockdev to libnvme3 a bunch of bugs were discovered. Fix them.
See storaged-project/libblockdev#1213