Make struct libnvme_fabrics_config private by igaw · Pull Request #3273 · linux-nvme/nvme-cli

igaw · 2026-04-15T13:39:25Z

Move struct libnvme_fabrics_config into the private section, so it's not part of the stable API.

Extend the !generate-accessors annotation so callers can set a struct-level default for accessor generation: struct foo { //!generate-accessors - both get+set (unchanged) struct foo { //!generate-accessors:none - no accessors by default struct foo { //!generate-accessors:readonly - getter only by default struct foo { //!generate-accessors:writeonly - setter only by default Add two new per-member annotations to complement the new struct-level defaults when individual members need to override them: //!accessors:writeonly - setter only for this member //!accessors:readwrite - both getter and setter for this member Internally, replace Member.is_const with explicit gen_getter/gen_setter flags and add parse_struct_annotation() to extract the mode qualifier. Update generate-accessors.md with the full annotation table. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Copilot

Pull request overview

This PR aims to remove struct libnvme_fabrics_config from the public/stable API surface by moving its definition into private headers, while introducing new fabrics-context-based flows and accessor generation updates to keep configuration usable without exposing struct layout.

Changes:

Move struct libnvme_fabrics_config out of the public fabrics header and update consumers to use fabrics-context/controller config getters.
Extend the accessor generator to support struct-level default modes (both/none/readonly/writeonly) plus per-member overrides.
Update libnvme/libnvmf APIs, examples, tests, and Python bindings to create controllers via libnvmf_context and new helper APIs.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
libnvme/tools/generator/update-accessors.sh	Adjusts symbol drift output formatting for version scripts.
libnvme/tools/generator/generate-accessors.py	Adds struct-level accessor modes and per-member mode overrides.
libnvme/tools/generator/generate-accessors.md	Documents new accessor-generation modes and annotations.
libnvme/test/tree.c	Includes fabrics private header for tests.
libnvme/src/nvme/tree.h	Removes some previously public controller/config APIs from the public header.
libnvme/src/nvme/tree.c	Refactors controller creation/config initialization and moves fabrics helpers out.
libnvme/src/nvme/private.h	Moves `struct libnvme_fabrics_config` into private header and updates internal prototypes.
libnvme/src/nvme/private-fabrics.h	Introduces/moves `struct libnvmf_context` definition and fabrics-private helpers.
libnvme/src/nvme/no-fabrics.c	Adds non-fabrics stub for hostname detection.
libnvme/src/nvme/linux.c	Updates renamed config fields (e.g., `keyring_id`).
libnvme/src/nvme/json.c	Switches to fabrics-config getter API for JSON config handling.
libnvme/src/nvme/fabrics.h	Removes public config struct; adds new public getter APIs and controller lifecycle APIs.
libnvme/src/nvme/fabrics.c	Implements new config getter APIs, disconnect helper, and refactors config handling.
libnvme/src/nvme/accessors.h / accessors.c	Adds generated public accessors for `struct libnvme_fabrics_config` (opaque in headers).
libnvme/src/nvme/accessors-fabrics.h / accessors-fabrics.c	Formatting/wrapping changes in generated accessors.
libnvme/src/meson.build	Builds `no-fabrics.c` when fabrics are disabled.
libnvme/src/libnvmf.ld / libnvme/src/libnvme.ld / libnvme/src/accessors.ld	Updates exported symbol lists for the new/removed APIs.
libnvme/libnvme/tests/test-objects.py	Updates Python tests to create controllers via `fabrics_context`.
libnvme/libnvme/tests/gc.py / create-ctrl-obj.py	Updates Python scripts to use `fabrics_context`.
libnvme/libnvme/nvme.i	Adds Python `fabrics_context` wrapper and updates controller creation/connect/disconnect.
libnvme/examples/discover-loop.c	Updates example to use `libnvmf_context` + new create/add/disconnect APIs.
fabrics.c	Updates nvme-cli fabrics tooling to set fabrics options via accessors/context config.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T14:56:00Z

@@ -25,7 +26,7 @@
 static void json_update_attributes(libnvme_ctrl_t c,
 				   struct json_object *ctrl_obj)
 {
-	struct libnvme_fabrics_config *cfg = libnvme_ctrl_get_config(c);
+	struct libnvme_fabrics_config *cfg = libnvmf_ctrl_get_fabrics_config(c);



json.c now depends on fabrics-only symbols (libnvmf_ctrl_get_fabrics_config) via private-fabrics.h. Since nvme/json.c is built regardless of want_fabrics (when json-c is found) but nvme/fabrics.c is not, non-fabrics builds will fail to link. Consider either (a) building json.c only when fabrics are enabled, or (b) avoiding fabrics-only APIs here by accessing c->cfg directly (json.c already includes private.h), or (c) providing non-fabrics stubs for the required helpers.

I suppose we only need the json stuff for the config.json and this is a fabrics feature. I am going to json dependent on fabrics.

Copilot · 2026-04-15T14:56:01Z

 1. **Dynamic strings** (`char *`) — setters store a `strdup()` copy; passing `NULL` clears the field.
 2. **Fixed char arrays** (`char foo[N]`) — setters use `snprintf`, always NUL-terminated.
-3. **`const` members** — only a getter is generated, no setter.
+3. **`const` members** — only a getter is generated, no setter (applies regardless of any annotation).
 4. **`//!accessors:readonly`** — same effect as `const`: getter only.
-5. **`//!accessors:none`** — member is completely ignored by the generator.
-6. **`--prefix`** — prepended to every function name (e.g. `--prefix nvme_` turns `ctrl_set_name` into `nvme_ctrl_set_name`).
-7. **Line length** — generated code is automatically wrapped to stay within the 80-column limit required by `checkpatch.pl`.
+5. **`//!accessors:writeonly`** — setter only; getter is suppressed.
+6. **`//!accessors:readwrite`** — both getter and setter; overrides a restrictive struct-level default.


The documentation states that const members "only" generate a getter regardless of any annotation, but the generator now allows struct/member modes (none, writeonly) that can suppress getters. Either update the docs to match the new behavior, or update the generator so const forces gen_getter=true (and gen_setter=false) even when the selected mode would otherwise suppress accessors.

martin-belanger · 2026-04-15T16:36:08Z

+	if (!strcmp(traddr, "none"))
 		return false;
-	if (inet_pton_with_scope(ctx, AF_UNSPEC, c->traddr, c->trsvcid, &addr) == 0)
+


traddr_is_hostname() drops scoped-IPv6 support.

The old implementation called inet_pton_with_scope() which handles IPv6 addresses with scope IDs (e.g. fe80::1%eth0). The new one calls inet_pton(AF_INET6, ...) directly, which will fail on scope IDs and misclassify them as hostnames.

Unlikely to matter in practice (NVMe over scoped-link-local is exotic), but worth noting.

NVMe over scoped-link-local is exotic

Actually that's what we've attempted for our NBFT testbed since we currently lack native ipv6/dualstack in our testbed. Worth noting there were more issues such exotic identified at various places and it never truly worked :-)

is this below not good enough?

+ if (inet_pton(AF_INET, traddr, addrstr) > 0 || + inet_pton(AF_INET6, traddr, addrstr) > 0)

or do we need this and the dropped inet_pton_with_scope test?

two implementations and they don't agree. nice one...

or let me formulate my question differently, the version which was already in fabrics.c is the better one?

In theory, inet_pton(AF_INET6, traddr, addrstr) should support scoped IPv6 addresses but let me write a quick test program to confirm. BTW, I'm the one that initially introduced inet_pton_with_scope() (which I copied from the kernel) because the original code would consider something like fe80::1%eth0 to be hostname.

I wrote a test program to compare the old implementation that uses inet_pton_with_scope() with the new implementation. The results are in the below table.

The new implementation gets 4 scoped IPv6 cases wrong — all misclassified as hostnames. Notice it also gets the eth99 case wrong even though that interface doesn't exist; the address format is still valid.

There's also an unexpected finding: the old implementation has a bug for the literal string "none" — it falls through to the IP parsing, fails, and returns true (hostname). The new implementation correctly short-circuits it with an explicit !strcmp(traddr, "none") check. So, the new code actually fixes one edge case while introducing the scoped address regression.

transport traddr expected old new description ---------- ----------------------------------- -------- -------- -------- ----------- tcp 192.168.1.10 IP IP IP IPv4 address rdma 192.168.1.10 IP IP IP IPv4 address (rdma) tcp fe80::1 IP IP IP IPv6 link-local (no scope) tcp 2001:db8::1 IP IP IP IPv6 global unicast tcp ::1 IP IP IP IPv6 loopback tcp fe80::1%lo IP IP hostname IPv6 scoped (lo — exists locally) <-- NEW_WRONG MISMATCH tcp fe80::1%enp0s31f6 IP IP hostname IPv6 scoped (enp0s31f6 — exists locally) <-- NEW_WRONG MISMATCH [note] interface 'eth99' not found locally (if_nametoindex failed) — treating as valid IP tcp fe80::1%eth99 IP IP hostname IPv6 scoped (eth99 — does not exist) <-- NEW_WRONG MISMATCH rdma fe80::1%lo IP IP hostname IPv6 scoped rdma (lo) <-- NEW_WRONG MISMATCH tcp storage.example.com hostname hostname hostname hostname (FQDN) tcp mynvme-target hostname hostname hostname hostname (short) rdma storage.example.com hostname hostname hostname hostname rdma pcie 192.168.1.10 IP IP IP IPv4 — pcie transport (check skipped) fc fe80::1%lo IP IP IP scoped IPv6 — fc transport (check skipped) loop storage.example.com IP IP IP hostname — loop transport (check skipped) tcp none IP hostname IP literal "none" <-- OLD_WRONG MISMATCH tcp (null) IP IP IP NULL traddr (null) 192.168.1.1 IP IP IP NULL transport Results: 13/18 passed | old wrong: 1 | new wrong: 4 | mismatches: 5

The ideal fix for this PR is simple: keep the "none" guard from the new implementation, but switch back to the scope-aware parsing for the IP detection:

bool traddr_is_hostname(const char *transport, const char *traddr) { struct sockaddr_storage addr; if (!traddr || !transport) return false; if (!strcmp(traddr, "none")) return false; if (strcmp(transport, "tcp") && strcmp(transport, "rdma")) return false; if (inet_pton_with_scope(traddr, &addr) == 0) /* scope-aware */ return false; return true; }

Just FYI: I added a new Wiki page describing --traddr, --host-traddr, and --host-iface. I describe how to use each parameter for each transport type: TCP, RDMA, FC. I also added a note regarding scoped IPv6 addresses saying to stay away from them due to lack of testing.

https://github.com/linux-nvme/nvme-cli/wiki/Using-%E2%80%90%E2%80%90host%E2%80%90traddr-and-%E2%80%90%E2%80%90host%E2%80%90iface

This function exists in two files with slightly different implementations. Use a single shared implementation instead. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Prepare the stage for generating getter/setters for libnvmf_context. For this, the struct needs to be in the private-fabrics header so the getter/setters are placed in the correct header file. Signed-off-by: Daniel Wagner <wagi@kernel.org>

merge_config merges libnvme_fabrics_config into the controller object from the first argument. There is no need to return the config, and there is no user for it. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Teach the Python binding about libnvmf_context so that libnvme_fabric_options can eventually be retired. Signed-off-by: Daniel Wagner <wagi@kernel.org>

libnvme_fabrics_config is going to be removed from the API. The first step is to let libnvmf_context own the configuration, coupling the fabrics config lifetime to the context object. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Retire the libnvme_fabrics_options type from the public API as any modification is breaking ABI. The libnvmf_context has been introduced as replacement, with an extendable getter/setter API. Signed-off-by: Daniel Wagner <wagi@kernel.org>

The connect and disconnect functions belong to the fabrics API. Move them there. While at it, also move libnvme_ctrl_get_config to the fabrics.h header, since it returns the fabrics configuration. Signed-off-by: Daniel Wagner <wagi@kernel.org>

There are two types of keys used: one is the string representation and the other is the actual key ID used by the kernel. Add a postfix to the kernel types so it is clear which is being used. Signed-off-by: Daniel Wagner <wagi@kernel.org>

libnvme_fabrics_config is going to be removed from the library API. Add the missing common command-line options to nvmf_args and introduce a central function that sets fabrics options in nvmf_context. This reduces the usage of libnvme_fabrics_options to a single place. Signed-off-by: Daniel Wagner <wagi@kernel.org>

The controller is created from the provided fabrics context, which contains all the information required to fully construct it. The libnvmf_add_ctrl function should only perform the connect call. Signed-off-by: Daniel Wagner <wagi@kernel.org>

The typedefs are defined in the public fabrics.h. Minimize the dependency on fabrics in the private.h header by using the concrete types for the argument types. This prepares the stage to drop the include. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Simplify maintainers' workflow by printing the symbols to add in the correct format so they can be copied and pasted directly. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Retire struct libnvme_fabrics_config from the public API and replace it with an anonymous struct accessed via getter/setters. This makes it possible to extend the API without breaking the ABI. Signed-off-by: Daniel Wagner <wagi@kernel.org>

The tls_configured_key_id should also be handled by the merge/update logic. Signed-off-by: Daniel Wagner <wagi@kernel.org>

Copolit insist on correct spelling. Signed-off-by: Daniel Wagner <wagi@kernel.org>

igaw force-pushed the fabrics-options branch from efbc610 to 10bce3a Compare April 15, 2026 14:45

igaw marked this pull request as ready for review April 15, 2026 14:45

igaw requested a review from Copilot April 15, 2026 14:46

Copilot started reviewing on behalf of igaw April 15, 2026 14:47 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

martin-belanger reviewed Apr 15, 2026

View reviewed changes

igaw force-pushed the fabrics-options branch from 10bce3a to 297b419 Compare April 15, 2026 17:40

igaw added 15 commits April 15, 2026 19:52

tree: use traddr_is_hostname from fabrics

7fd9acf

This function exists in two files with slightly different implementations. Use a single shared implementation instead. Signed-off-by: Daniel Wagner <wagi@kernel.org>

fabrics: don't return config from merge_config

dd07cb6

merge_config merges libnvme_fabrics_config into the controller object from the first argument. There is no need to return the config, and there is no user for it. Signed-off-by: Daniel Wagner <wagi@kernel.org>

libnvme: update Python binding to use libnvmf_context

5e92f0a

Teach the Python binding about libnvmf_context so that libnvme_fabric_options can eventually be retired. Signed-off-by: Daniel Wagner <wagi@kernel.org>

generate-accessors: output added list with correct formatting

db9564d

Simplify maintainers' workflow by printing the symbols to add in the correct format so they can be copied and pasted directly. Signed-off-by: Daniel Wagner <wagi@kernel.org>

fabrics: add tls_configured_key to merge/update logic

4756776

The tls_configured_key_id should also be handled by the merge/update logic. Signed-off-by: Daniel Wagner <wagi@kernel.org>

private-fabrics: fix spelling in comment

f741ff8

Copolit insist on correct spelling. Signed-off-by: Daniel Wagner <wagi@kernel.org>

igaw force-pushed the fabrics-options branch from 297b419 to f741ff8 Compare April 15, 2026 18:19

Conversation

igaw commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-belanger Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martin-belanger Apr 15, 2026 •

edited

Loading