From 1fb6316d81e6fd4e40c797ec325b433595f371d6 Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Mar 2025 17:15:32 -0400 Subject: [PATCH] python, swig: add missing controller attributes Found that some controller attributes (e.g. tls_key) were missing from the SWIG wrapper code. Also found that for some of the attributes, SWIG was generating code that accessed to attributes directly instead of using the getter/setter methods. For example, SWIG would generate code like this: name = c->name; Instead of that: name = nvme_ctrl_get_name(c); This may have been OK in most cases, but the Python code should really use the getter/setter methods instead of accessing the controller object directly. SWIG is a weird beast to tame. For example, all the setter/getter methods must exactly match the syntax: [class]_[member]_[set|get] (e.g. nvme_ctrl_traddr_get) However, most of the libnvme functions follow this syntax: [class]_[set|get]_[member] (e.g. nvme_ctrl_get_traddr) The trick to force SWIG to use the setter/getter methods is to define the attributes in a %extend block and provide explicit translation methods for all the setter/getter methods. Signed-off-by: Martin Belanger --- libnvme/meson.build | 6 +- libnvme/nvme.i | 224 +++++++++++++++++++++++++++++++++++++------- 2 files changed, 191 insertions(+), 39 deletions(-) diff --git a/libnvme/meson.build b/libnvme/meson.build index b5b99fc59..c06c07f1d 100644 --- a/libnvme/meson.build +++ b/libnvme/meson.build @@ -62,9 +62,9 @@ if build_python_bindings # Set the PYTHONPATH so that we can run the # tests directly from the build directory. test_env = environment() - test_env.append('MALLOC_PERTURB_', '0') - test_env.append('PYTHONPATH', join_paths(meson.current_build_dir(), '..')) - test_env.append('PYTHONMALLOC', 'malloc') + test_env.set('MALLOC_PERTURB_', '90') + test_env.prepend('PYTHONPATH', join_paths(meson.current_build_dir(), '..')) + test_env.set('PYTHONMALLOC', 'malloc') # Test section test('python-import-libnvme', python3, args: ['-c', 'from libnvme import nvme'], env: test_env, depends: pynvme_clib) diff --git a/libnvme/nvme.i b/libnvme/nvme.i index eb94cac8d..4ede50e74 100644 --- a/libnvme/nvme.i +++ b/libnvme/nvme.i @@ -357,6 +357,9 @@ struct nvme_subsystem { }; struct nvme_ctrl { + %immutable name; + %immutable subsystem; + %immutable state; %immutable sysfs_dir; %immutable address; %immutable firmware; @@ -369,34 +372,67 @@ struct nvme_ctrl { %immutable subsysnqn; %immutable traddr; %immutable trsvcid; - %immutable dhchap_host_key; - %immutable dhchap_key; %immutable cntrltype; + %immutable cntlid; %immutable dctype; - %immutable discovery_ctrl; + %immutable phy_slot; %immutable discovered; - %immutable persistent; - char *sysfs_dir; - char *address; - char *firmware; - char *model; - char *numa_node; - char *queue_count; - char *serial; - char *sqsize; - char *transport; - char *subsysnqn; - char *traddr; - char *trsvcid; + + const char *cntrltype; // Do not put in %extend because there's no getter method in libnvme.map + const char *dctype; // Do not put in %extend because there's no getter method in libnvme.map + const bool discovered; // Do not put in %extend because there's no getter method in libnvme.map + %extend { - char *dhchap_host_key: - char *dhchap_key; + /** + * By putting these attributes in an %extend block, we're + * forcing SWIG to invoke getter/setter methods instead of + * accessing the members directly. + * + * For example, SWIG will generate code like this: + * name = nvme_ctrl_name_get(ctrl) + * + * instead of that: + * name = ctrl->name + */ + const char *name; + const char *state; + const char *sysfs_dir; + const char *address; + const char *firmware; + const char *model; + const char *numa_node; + const char *queue_count; + const char *serial; + const char *sqsize; + const char *transport; + const char *subsysnqn; + const char *traddr; + const char *trsvcid; + const char *cntlid; + const char *phy_slot; + + bool unique_discovery_ctrl; + bool discovery_ctrl; + bool persistent; + + char *keyring; + char *tls_key_identity; + char *tls_key; + + /** + * We are remapping the following members of the C code's + * nvme_ctrl_t to different names in Python. Here's the mapping: + * + * C code Python (SWIG) + * ===================== ===================== + * ctrl->s ctrl->subsystem + * ctrl->dhchap_key ctrl->dhchap_host_key + * ctrl->dhchap_ctrl_key ctrl->dhchap_key + */ + struct nvme_subsystem *subsystem; // Maps to "s" in the C code + char *dhchap_host_key; // Maps to "dhchap_key" in the C code + char *dhchap_key; // Maps to "dhchap_ctrl_key" in the C code } - char *cntrltype; - char *dctype; - bool discovery_ctrl; - bool discovered; - bool persistent; }; struct nvme_ns { @@ -634,9 +670,13 @@ struct nvme_ns { nvme_free_ctrl($self); } - void discovery_ctrl_set(bool discovery) { - nvme_ctrl_set_discovery_ctrl($self, discovery); - } + %pythoncode %{ + def discovery_ctrl_set(self, discovery: bool): + r"""DEPRECATED METHOD: Use property setter instead (e.g. ctrl.discovery_ctrl = True)""" + import warnings + warnings.warn("Use property setter instead (e.g. ctrl_obj.discovery_ctrl = True)", DeprecationWarning, stacklevel=2) + return _nvme.ctrl_discovery_ctrl_set(self, discovery) + %} bool init(struct nvme_host *h, int instance) { return nvme_init_ctrl(h, $self, instance) == 0; @@ -665,9 +705,13 @@ struct nvme_ns { bool connected() { return nvme_ctrl_get_name($self) != NULL; } - void persistent_set(bool persistent) { - nvme_ctrl_set_persistent($self, persistent); - } + %pythoncode %{ + def persistent_set(self, persistent: bool): + r"""DEPRECATED METHOD: Use property setter instead (e.g. ctrl.persistent = True)""" + import warnings + warnings.warn("Use property setter instead (e.g. ctrl_obj.persistent = True)", DeprecationWarning, stacklevel=2) + return _nvme.ctrl_persistent_set(self, persistent) + %} void rescan() { nvme_rescan_ctrl($self); } @@ -760,15 +804,22 @@ struct nvme_ns { struct nvme_ns* namespaces() { return nvme_ctrl_first_ns($self); } - %immutable name; - const char *name; - %immutable subsystem; - struct nvme_subsystem *subsystem; - %immutable state; - const char *state; } %{ + /********************************************************************** + * SWIG automatically generates getter and setter methods using + * the syntax: [class]_[member]_[get|set]. These need to be mapped + * to the matching methods in libnvme (i.e. those that are defined + * publicly in libnvme.map). Typically, we get the following mapping: + * + * SWIG libnvme.map + * ====================== ======================= + * nvme_ctrl_[member]_get -> nvme_ctrl_get_[member] + * nvme_ctrl_[member]_set -> nvme_ctrl_set_[member] + * + */ + const char *nvme_ctrl_name_get(struct nvme_ctrl *c) { return nvme_ctrl_get_name(c); } @@ -787,7 +838,108 @@ struct nvme_ns { const char *nvme_ctrl_dhchap_host_key_get(struct nvme_ctrl *c) { return nvme_ctrl_get_dhchap_host_key(c); } -%}; + void nvme_ctrl_dhchap_host_key_set(struct nvme_ctrl *c, const char *key) { + nvme_ctrl_set_dhchap_host_key(c, key); + } + + const char *nvme_ctrl_cntlid_get(nvme_ctrl_t c) { + return nvme_ctrl_get_cntlid(c); + } + + bool nvme_ctrl_persistent_get(struct nvme_ctrl *c) { + return nvme_ctrl_is_persistent(c); + } + void nvme_ctrl_persistent_set(struct nvme_ctrl *c, bool persistent) { + nvme_ctrl_set_persistent(c, persistent); + } + + const char *nvme_ctrl_phy_slot_get(nvme_ctrl_t c) { + return nvme_ctrl_get_phy_slot(c); + } + + const char *nvme_ctrl_trsvcid_get(nvme_ctrl_t c) { + return nvme_ctrl_get_trsvcid(c); + } + + const char *nvme_ctrl_traddr_get(nvme_ctrl_t c) { + return nvme_ctrl_get_traddr(c); + } + + const char *nvme_ctrl_subsysnqn_get(nvme_ctrl_t c) { + return nvme_ctrl_get_subsysnqn(c); + } + + const char *nvme_ctrl_transport_get(nvme_ctrl_t c) { + return nvme_ctrl_get_transport(c); + } + + const char *nvme_ctrl_sqsize_get(nvme_ctrl_t c) { + return nvme_ctrl_get_sqsize(c); + } + + const char *nvme_ctrl_serial_get(nvme_ctrl_t c) { + return nvme_ctrl_get_serial(c); + } + + const char *nvme_ctrl_queue_count_get(nvme_ctrl_t c) { + return nvme_ctrl_get_queue_count(c); + } + + const char *nvme_ctrl_numa_node_get(nvme_ctrl_t c) { + return nvme_ctrl_get_numa_node(c); + } + + const char *nvme_ctrl_model_get(nvme_ctrl_t c) { + return nvme_ctrl_get_model(c); + } + + const char *nvme_ctrl_firmware_get(nvme_ctrl_t c) { + return nvme_ctrl_get_firmware(c); + } + + const char *nvme_ctrl_address_get(nvme_ctrl_t c) { + return nvme_ctrl_get_address(c); + } + + const char *nvme_ctrl_sysfs_dir_get(nvme_ctrl_t c) { + return nvme_ctrl_get_sysfs_dir(c); + } + + bool nvme_ctrl_discovery_ctrl_get(struct nvme_ctrl *c) { + return nvme_ctrl_is_discovery_ctrl(c); + } + void nvme_ctrl_discovery_ctrl_set(struct nvme_ctrl *c, bool discovery) { + nvme_ctrl_set_discovery_ctrl(c, discovery); + } + + bool nvme_ctrl_unique_discovery_ctrl_get(nvme_ctrl_t c) { + return nvme_ctrl_is_unique_discovery_ctrl(c); + } + void nvme_ctrl_unique_discovery_ctrl_set(nvme_ctrl_t c, bool unique) { + nvme_ctrl_set_unique_discovery_ctrl(c, unique); + } + + const char *nvme_ctrl_keyring_get(nvme_ctrl_t c) { + return nvme_ctrl_get_keyring(c); + } + void nvme_ctrl_keyring_set(nvme_ctrl_t c, const char *keyring) { + nvme_ctrl_set_keyring(c, keyring); + } + + const char *nvme_ctrl_tls_key_identity_get(nvme_ctrl_t c) { + return nvme_ctrl_get_tls_key_identity(c); + } + void nvme_ctrl_tls_key_identity_set(nvme_ctrl_t c, const char *identity) { + nvme_ctrl_set_tls_key_identity(c, identity); + } + + const char *nvme_ctrl_tls_key_get(nvme_ctrl_t c) { + return nvme_ctrl_get_tls_key(c); + } + void nvme_ctrl_tls_key_set(nvme_ctrl_t c, const char *key) { + nvme_ctrl_set_tls_key(c, key); + } +%} %pythonappend nvme_ns::nvme_ns(struct nvme_subsystem *s, unsigned int nsid) {