tree: remove fabrics-only code from tree.c#3369
Open
martin-belanger wants to merge 6 commits into
Open
Conversation
added 6 commits
May 16, 2026 08:04
The function pointers in libnvmf_context are synchronous interceptors called at specific points in the operation lifecycle — some of them (the parser hooks) actually modify the context for subsequent steps. The term "callback" implies passive, fire-and-forget behavior, which does not accurately describe their role. Rename to "hooks" throughout. Changes: - Comments in private-fabrics.h: "callbacks" -> "hooks" - Kdoc in fabrics.h: "Callback" -> "Hook" in parameter descriptions - libnvmf_context_set_discovery_cbs() -> libnvmf_context_set_discovery_hooks() (updated definition, declaration, libnvmf.ld export, and man page) - nvme-cli/fabrics.c: struct cb_fabrics_data -> hook_fabrics_data, cfd -> hfd, cb_* -> hook_* for all seven hook implementations Signed-off-by: Martin Belanger <martin.belanger@dell.com> Assisted-by: Claude:claude-sonnet-4-6 [Claude Code]
Group the seven hook function pointers and the associated user_data
field into a dedicated sub-struct embedded in libnvmf_context:
struct libnvmf_hooks { decide_retry, connected, already_connected,
discovery_log, parser_init, parser_cleanup, parser_next_line,
user_data };
All invocation and assignment sites change from fctx->hook_name to
fctx->hooks.hook_name. The shallow-copy pattern (nfctx = *fctx) used
in discovery loops is unaffected. No change to public API signatures.
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Assisted-by: Claude:claude-sonnet-4-6 [Claude Code]
Introduce struct libnvme_ctrl_params to hold the NVMe controller parameters that identify and configure a connection: transport, traddr, host_traddr, host_iface, trsvcid, subsysnqn, and the fabrics config. Update libnvme_create_ctrl() to take a const struct libnvme_ctrl_params * instead of struct libnvmf_context *, removing the dependency on the fabrics context from the controller allocation path. Embed struct libnvme_ctrl_params as ctrl_params in struct libnvmf_context, replacing the individual fields that were previously scattered directly in the context. Call sites that bridge fctx into libnvme_create_ctrl() now pass &fctx->ctrl_params directly. The goal is to make controller parameters self-contained so that code which only needs to identify or allocate a controller does not need to depend on libnvmf_context. This is a stepping stone toward separating fabrics-only code from code that is common to all transports. Signed-off-by: Martin Belanger <martin.belanger@dell.com> Assisted-by: Claude:claude-sonnet-4-6 [Claude Code]
This is a pre-existing bug uncovered during refactoring. libnvme_create_ctrl() copies fctx->ctrl_params.cfg to c->cfg before nvme_parse_tls_args() runs. When the keyring or TLS key is specified as a numeric ID string, nvme_parse_tls_args() writes the resolved ID back into the cfg it receives. Because libnvme_create_ctrl() already ran, the resolved ID ends up only in fctx->ctrl_params.cfg and never reaches c->cfg, so build_options() reads 0 for tls_key_id and the key is silently ignored. Fix by calling update_config() after nvme_parse_tls_args(), mirroring what libnvmf_config_modify() already does correctly. Signed-off-by: Martin Belanger <martin.belanger@dell.com> Assisted-by: Claude:claude-sonnet-4-6 [Claude Code]
The ctrl-find/alloc cluster (libnvme_candidate_init, libnvme_ctrl_find, libnvme_lookup_ctrl, libnvme_ctrl_alloc) only ever accessed fctx->ctrl_params.*, so passing the full libnvmf_context was unnecessary coupling. Change all four functions to accept const struct libnvme_ctrl_params * instead: - libnvme_candidate_init(ctx, candidate, params) - libnvme_ctrl_find(s, params, p) - libnvme_lookup_ctrl(s, params, p) - libnvme_ctrl_alloc() — now static (no external callers remain) Fabrics-side wrappers libnvmf_candidate_init, libnvmf_ctrl_find, libnvmf_ctrl_match_config and libnvmf_read_sysfs_fabrics_attrs retain their libnvmf_context / libnvme_ctrl_t signatures; they pass &fctx->ctrl_params to the inner functions as appropriate. json.c no longer needs a full libnvmf_context; simplify to use struct libnvme_ctrl_params directly and drop the private-fabrics.h include. Move libnvmf_read_sysfs_fabrics_attrs() declaration from private-fabrics.h to private.h since it is called from tree.c which must not include private-fabrics.h after the upcoming Commit 5. Remove a redundant cfg copy in libnvmf_connect_disc_entry(): the line nfctx.ctrl_params.cfg = fctx->ctrl_params.cfg; was made dead by the shallow copy nfctx = *fctx two lines above it. Signed-off-by: Martin Belanger <martin.belanger@dell.com> Assisted-by: Claude:claude-sonnet-4-6 [Claude Code]
Move the ctrl-find/match cluster out of tree.c and into tree-fabrics.c
so that tree.c no longer depends on private-fabrics.h:
- _libnvmf_tree_ctrl_match → tree-fabrics.c (static)
- _libnvmf_candidate_init → tree-fabrics.c (static)
- libnvme_ctrl_find → tree-fabrics.c (non-static; called from
tree.c::libnvme_lookup_ctrl, so a
simplified no-fabrics stub is added to
no-fabrics.c)
- libnvme_lookup_ctrl → stays in tree.c (calls libnvme_ctrl_find
and libnvmf_default_config, both now
declared in private.h)
Promote traddr_is_hostname, libnvmf_default_config, and
libnvme_ctrl_find declarations from private-fabrics.h to private.h
since they are called from tree.c which no longer includes
private-fabrics.h.
Remove #include "private-fabrics.h" from tree.c.
Add a comment to each stub in no-fabrics.c identifying the file that
holds the real implementation.
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Assisted-by: Claude:claude-sonnet-4-6 [Claude Code]
martin-belanger
force-pushed
the
decouple-ctrl-alloc
branch
from
1fa23ed to
8648654
Compare
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
1 participant
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.
Summary
The ultimate goal of this series is to completely remove fabrics-only code from
tree.cso that PCIe-only builds carry zero NVMe-oF overhead. The previous series (already merged) createdtree-fabrics.cand moved the TCP matching functions and sysfs readers there. This series goes further by decoupling the controller allocation and lookup machinery fromlibnvmf_context, then moving the ctrl-find cluster itself intotree-fabrics.c, leavingtree.cwith no#include "private-fabrics.h"dependency.fabrics: rename callbacks to hooks (327e17c)
Renames
libnvmf_callbacks→libnvmf_hooksand the correspondinglibnvmf_context_set_*_cbs()accessors tolibnvmf_context_set_*_hooks(). The term "hooks" is more accurate — these are injection points for application behaviour, not observer callbacks. Preparatory rename before the structural changes that follow.fabrics: group hooks and user_data into struct libnvmf_hooks (7efbed4)
Embeds all hook function pointers and
user_datainto a nestedstruct libnvmf_hooks hooksinsidelibnvmf_context. This makes the relationship between the hooks and their shareduser_dataexplicit in the type system. Call sites change fromfctx->hook_name(...)tofctx->hooks.hook_name(...).tree/fabrics: introduce libnvme_ctrl_params and embed in libnvmf_context (01d61a1)
Introduces
struct libnvme_ctrl_paramsto group the connection-target fields (transport,traddr,trsvcid,subsysnqn,host_traddr,host_iface, and the embeddedstruct libnvme_fabrics_config). The struct is embedded asctrl_paramsinlibnvmf_context, replacing the individual fields that were scattered across the context. Also introduceslibnvme_fabrics_config_copy()as a safe alternative to raw struct assignment forstruct libnvme_fabrics_config, guarding against future additions of owned pointer members.fabrics: fix libnvmf_connect() not propagating resolved TLS key IDs (3a1667e)
Pre-existing bug uncovered during refactoring:
nvme_parse_tls_args()resolves symbolic TLS key identifiers into numeric IDs and writes them intofctx->ctrl_params.cfg, but this happens afterlibnvme_create_ctrl()has already copiedcfgtoc->cfg, so the resolved IDs never reached the controller. The fix mirrors the pattern already present inlibnvmf_config_modify(): callupdate_config(c, &fctx->ctrl_params.cfg)afternvme_parse_tls_args().tree: decouple ctrl-find/alloc from libnvmf_context (d3ed1ed)
The ctrl-find/alloc cluster (
libnvme_candidate_init,libnvme_ctrl_find,libnvme_lookup_ctrl,libnvme_ctrl_alloc) only ever accessedfctx->ctrl_params.*, so passing the fulllibnvmf_contextwas unnecessary coupling. All four functions are changed to acceptconst struct libnvme_ctrl_params *instead.libnvme_ctrl_allocbecomes static.json.cis simplified to usestruct libnvme_ctrl_paramsdirectly and drops itsprivate-fabrics.hinclude.tree: move ctrl-find cluster to tree-fabrics.c (8648654)
Moves the ctrl-find/match functions from
tree.cintotree-fabrics.c:_libnvmf_tree_ctrl_matchand_libnvmf_candidate_initbecome static;libnvme_ctrl_findis non-static becausetree.c::libnvme_lookup_ctrlstill calls it, so a simplified no-fabrics stub (transport + traddr matching) is added tono-fabrics.c. The declarations oftraddr_is_hostname,libnvmf_default_config, andlibnvme_ctrl_findare promoted fromprivate-fabrics.htoprivate.h. The#include "private-fabrics.h"is removed fromtree.c.