Skip to content

python bindings: redesign Phase 1 + Phase 2#3332

Merged
igaw merged 2 commits intolinux-nvme:masterfrom
martin-belanger:libnvme-python-bindings-redesign-plan-3
Apr 30, 2026
Merged

python bindings: redesign Phase 1 + Phase 2#3332
igaw merged 2 commits intolinux-nvme:masterfrom
martin-belanger:libnvme-python-bindings-redesign-plan-3

Conversation

@martin-belanger
Copy link
Copy Markdown

@martin-belanger martin-belanger commented Apr 29, 2026

Summary

This PR delivers Phase 1 and Phase 2 of a 3-phase refactoring of the libnvme Python bindings. Phase 3 (callback/hook support) is planned but not yet scheduled.

Phase 1 — Generator-emitted SWIG fragments

  • Extends generate-accessors.py with a --swig-out option that emits accessors.i and accessors-fabrics.i directly from the !generate-accessors / !generate-python annotations in private.h and private-fabrics.h.
  • Each struct annotated !generate-python gets a SWIG fragment with per-axis access routing: generated axes use direct field access (p->member), custom axes route through %rename + #define bridge to the hand-written accessor.
  • Removes the hand-maintained nvme-swig-accessors.i and generate-swig-accessors.py (replaced by the generator).
  • Removes check-nvme-i-consistency.py and its meson test (no longer needed — nvme.i no longer duplicates struct declarations).
  • Consolidates the __setattr__ guard into a single generated definition installed per class at module import.

Phase 2 — API surface polish

  • connected() and is_registration_supported() converted to read-only Python properties (ctrl.connected, ctrl.registration_supported).
  • host.set_symname(x) deleted; host.hostsymname = x works directly via the generated SWIG field descriptor.
  • registration_ctrl() renamed to registration_control() — the method is a multi-action control interface (register, deregister, update via NVMF_DIM_TAS_*), not a single register operation.

Test plan

  • meson test -C .build passes (all 66 tests including python-object-properties-and-errors)
  • meson compile -C .build update-accessors regenerates identical accessors.i / accessors-fabrics.i

Extend generate-accessors.py to emit three new SWIG fragment files from
the existing !generate-accessors annotations in private.h and
private-fabrics.h:

  accessors.i          — kernel-object structs (Ctrl, Host, Subsystem,
                         Namespace, GlobalCtx)
  accessors-fabrics.i  — fabrics-specific structs
  nvme-manual-bridges.i — residual hand-written %rename bridges

A new struct-level annotation !generate-python[:alias=NAME] gates
emission for Python and carries the PascalCase class alias. Remove the
hand-written %rename(ctrl) / %rename(host) / ... lines from nvme.i;
they are now generator-emitted.

Restructure nvme.i so all %typemap directives precede the generated
%include "accessors.i". SWIG freezes type-to-typemap associations when
it first processes a struct body; struct bodies now arrive via the
generated include so typemaps must come first.

A generator-emitted _nvme_guarded_setattr is installed on each class at
import to catch typos and writes to read-only (%immutable) properties.
C-internal iteration helpers are renamed with a leading underscore via
%rename to keep them out of the public API surface.

Update nvme.i docstrings to Pythonic style (Args:/Returns: conventions,
PascalCase class names, module-level usage examples). Update tests and
examples to use the new API.

Verified: meson test -C .build passes (64 OK, 2 expected failures).

Signed-off-by: Martin Belanger <[email protected]>
Assisted-by: Claude Sonnet 4.6 <[email protected]>
Comment thread libnvme/libnvme/nvme.i
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 30, 2026

Overall I think this goes into the right direction. I don't mind that the patches are bit messy with all the code reshuffling. That is kind of expected.

I was wondering if we can also git rid of the code snippet below which is still hand written and makes the 'add a new member to struct libnvme_ctrl and generate the bindings' not fully automated:

		if (!PyUnicode_CompareWithASCIIString(key, "nr_io_queues")) {
			cfg->nr_io_queues = PyLong_AsLong(value);
			continue;
		}
		if (!PyUnicode_CompareWithASCIIString(key, "reconnect_delay")) {
			cfg->reconnect_delay = PyLong_AsLong(value);
			continue;
		}

@martin-belanger
Copy link
Copy Markdown
Author

martin-belanger commented Apr 30, 2026

I agree that the %typemap for converting a Python dict to struct libnvmf_context is one of the remaining pieces that isn’t fully automated.

I did look into generating it, but it’s not entirely straightforward. There are a few cases where fields can’t be handled as simple assignments. For example:

  • hostnqn / hostid must go through libnvmf_context_set_hostnqn()
  • crypto-related fields require libnvmf_context_set_crypto()
  • connection parameters are grouped and handled via libnvmf_context_set_connection()

So we can’t treat all fields uniformly.

That said, I think there’s still room to improve things incrementally. We could generate handling for the “simple” scalar fields (like the ones you pointed out), and keep the more complex/grouped cases implemented manually.

To make that work cleanly, we’d likely need some annotations to distinguish:

  • fields that can be auto-generated (plain assignments)
  • fields that require special handling

That way we can reduce the amount of handwritten code without losing clarity or breaking the more complex semantics.

I’d prefer to keep the current structure for readability and layer generation on top of it, rather than fully replacing it.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 30, 2026

Sure, I agree, let's do step by step. BTW, I think we should have the setters/getters for libnvmf_context created anyway, so this libnvmf_context_set_connection() would be just an optimization. For the PoC I am working on, I did this.

- Convert connected() and is_registration_supported() to read-only
  Python properties (connected, registration_supported)
- Delete set_symname(); host.hostsymname is already writable via the
  generated SWIG field descriptor
- Rename registration_ctrl() to registration_control() to reflect that
  the method is a multi-action control interface (register, deregister,
  update via NVMF_DIM_TAS_*), not a single register operation
- Update test-objects.py to match the new API

Signed-off-by: Martin Belanger <[email protected]>
Assisted-by: Claude Sonnet <[email protected]>
@martin-belanger martin-belanger force-pushed the libnvme-python-bindings-redesign-plan-3 branch from d65df4d to bd5d69c Compare April 30, 2026 13:46
@igaw igaw merged commit ccd5d5a into linux-nvme:master Apr 30, 2026
28 of 29 checks passed
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 30, 2026

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants