python, swig: fix bugs and restore Python ergonomics after libnvmf_context refactor#3293
Merged
igaw merged 3 commits intolinux-nvme:masterfrom Apr 21, 2026
Merged
Conversation
added 3 commits
April 20, 2026 18:46
Three bugs were introduced when libnvmf_context replaced the old libnvme_fabrics_config parameter in the connect/create flow: 1. The %pythonappend GC guard for connect() had a stale signature that still included the removed fctx parameter. SWIG silently ignores a mismatched %pythonappend, so self.__host = h never ran, leaving a latent use-after-free where Python could collect the host object while the ctrl was still alive. 2. The duplicate_connect check in connect() had an inverted condition (missing !) and belonged in the library, not the binding. Move it to libnvmf_add_ctrl() in fabrics.c where it applies uniformly to all callers, C and Python alike. 3. fctx_set_fabrics_config was missing its %rename directive, exposing it to Python as fctx.fctx_set_fabrics_config() instead of fctx.set_fabrics_config() like all other libnvmf_context methods. Signed-off-by: Martin Belanger <[email protected]> Assisted-by: Claude Sonnet 4.6 <[email protected]>
The libnvmf_context object was a leaky abstraction: Python callers had to create it, configure it, pass it to ctrl(), then discard it — despite never needing it after construction. This commit restores the ergonomics that were lost when the C API switched from libnvme_fabrics_config to libnvmf_context. The public header declares libnvmf_context as an opaque forward declaration, which caused SWIG to treat it as an opaque pointer type and override any user-defined %typemap. Fix this by providing an empty struct body in the SWIG parsed section, which gives SWIG a complete type and restores normal typemap precedence without modifying any C header. The %typemap(in) / %typemap(freearg) pair for struct libnvmf_context * now handles the full fctx lifecycle transparently: the typemap creates the fctx from the Python dict (using arg1 as the global context), the constructor body calls libnvmf_create_ctrl(), and freearg frees the fctx. The typemap is reusable for any future function that takes struct libnvmf_context *. The dict supports all configurable fields: connection identity (subsysnqn, transport, traddr, trsvcid, host_traddr, host_iface), the full libnvme_fabrics_config (via libnvme_fabrics_config_set_* accessors and libnvmf_context_get_fabrics_config), host identity (hostnqn, hostid), crypto and TLS (hostkey, ctrlkey, keyring, tls_key, tls_key_identity), and persistence. All context-level fields are set via their public API setters. An autodoc string on the constructor documents every supported key so that help(nvme.ctrl) is the complete reference. Tests are updated to use the new dict-based API. Signed-off-by: Martin Belanger <[email protected]> Assisted-by: Claude Sonnet 4.6 <[email protected]>
SWIG >= 4.1 generates calls to Py_NewRef() in its runtime boilerplate. Py_NewRef was introduced in Python 3.10, causing an undefined symbol error on older distributions (e.g. SLES 15.6/15.7 with Python 3.6). Add a static inline shim inside the %begin block, guarded by PY_VERSION_HEX, so it precedes SWIG's own runtime in the generated C file. Python.h is included explicitly at that point to make PyObject available. A meson warning is emitted at configure time when the older Python is detected. Signed-off-by: Martin Belanger <[email protected]> Assisted-by: Claude Sonnet 4.6 <[email protected]>
Collaborator
|
Looks great. Thanks As indicated on the chat I'd to see the callbacks also be added to the Python binding: This would make the Python binding offering the same features as the C binding and allows to write any discovery/connect logic in Python. The C library should just do the low level parsing/iterating over the log pages and the rest can be somewhere else. This would allow to have way simpler custom logic supported on default. |
Author
|
I agree with the callbacks and I do plan to revisit this soon. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits fixing regressions and compatibility issues introduced when the C API moved from
libnvme_fabrics_configto the opaquelibnvmf_context.Commit 1 — Bug fixes from the libnvmf_context refactor
%pythonappendforconnect()had a stalefctxparameter — SWIG silently ignored it, so the GC guard (self.__host = h) never ran, leaving a latent use-after-free.duplicate_connectcheck was inverted in the SWIG binding. Moved to the C library (libnvmf_add_ctrl()) where it belongs, with the correct polarity.fctx_set_fabrics_configwas missing its%rename, exposing it to Python with the uglyfctx_prefix intact.Commit 2 — Restore Python ergonomics: dict-based ctrl constructor
The refactor forced Python callers to manually create, configure, and discard a
libnvmf_context— a C implementation detail with no value at the Python level. This commit hides it entirely behind a%typemap(in/freearg)pair that converts a Python dict to an fctx transparently. The typemap is reusable for any future function that takesstruct libnvmf_context *.Commit 3 — Py_NewRef compatibility (Python < 3.10)
SWIG ≥ 4.1 generates calls to
Py_NewRef(), which was added in Python 3.10. Distributions with SWIG ≥ 4.1 and Python < 3.10 (e.g. SLES 15.x) got anundefined symbolat import time. Fixed with a%begincompat shim and a meson warning when the condition is detected.The dict covers all configurable fields: connection identity, the full
libnvme_fabrics_config(set via the generated accessors), host identity, DH-HMAC-CHAP keys, TLS parameters, and persistence.help(nvme.ctrl)documents every supported key via an autodoc string.