From b2b0630878aa4d5323df0533deeca98038652f9a Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Aug 2026 12:30:27 -0400 Subject: [PATCH 1/6] libnvme: guard three unchecked fabrics hook calls against NULL Every fabrics hook is optional and every other call site in fabrics.c already guards accordingly. These are public APIs. A caller must be allowed to pass NULL for a hook it doesn't need. Signed-off-by: Martin Belanger --- libnvme/src/nvme/fabrics.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index 047e01ca86..83d5ec9393 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -2943,7 +2943,8 @@ static int libnvme_add_ctrl(struct libnvmf_context *fctx, err = libnvmf_add_ctrl(h, c); if (!err) return 0; - if (fctx->hooks.decide_retry(fctx, err, fctx->hooks.user_data)) + if (fctx->hooks.decide_retry && + fctx->hooks.decide_retry(fctx, err, fctx->hooks.user_data)) goto retry; return err; @@ -3816,11 +3817,13 @@ __shr_public int libnvmf_connect( write_devid_file(fctx, devid_fd, c); if (instance >= 0) registry_update_on_connect(ctx, instance); - fctx->hooks.already_connected(fctx, h, - libnvme_ctrl_get_subsysnqn(c), - libnvme_ctrl_get_transport(c), - libnvme_ctrl_get_traddr(c), - libnvme_ctrl_get_trsvcid(c), fctx->hooks.user_data); + if (fctx->hooks.already_connected) + fctx->hooks.already_connected(fctx, h, + libnvme_ctrl_get_subsysnqn(c), + libnvme_ctrl_get_transport(c), + libnvme_ctrl_get_traddr(c), + libnvme_ctrl_get_trsvcid(c), + fctx->hooks.user_data); return -EALREADY; } @@ -3872,7 +3875,8 @@ __shr_public int libnvmf_connect( } write_devid_file(fctx, devid_fd, c); - fctx->hooks.connected(fctx, c, fctx->hooks.user_data); + if (fctx->hooks.connected) + fctx->hooks.connected(fctx, c, fctx->hooks.user_data); return 0; } From b7a6aa3540cd565d1ab2162ca2ea5f1209efac95 Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Aug 2026 12:42:17 -0400 Subject: [PATCH 2/6] nvme, libnvme: rename discover/connect-all --force to --no-reuse --force used to do double duty: force persistence, and skip reusing an existing connection. Now that --persistent=force exists as its own explicit option, --force only ever means the latter -- so the name no longer matches what it does. Add --no-reuse as the real name; keep --force as a deprecated alias so existing scripts and discovery.conf files keep working. Rename the backing fctx->force field and its accessors to match, and check_ctrl_owner()'s force parameter, which shares the same concept. Document --no-reuse on nvme-connect-all(1) too, which never had a --force entry at all. Signed-off-by: Martin Belanger --- Documentation/nvme-connect-all.txt | 4 ++ Documentation/nvme-discover.txt | 8 ++-- libnvme/src/accessors-fabrics.ld | 4 +- libnvme/src/nvme/fabrics.c | 11 ++--- .../src/nvme/generated/accessors-fabrics.c | 10 ++--- .../src/nvme/generated/accessors-fabrics.h | 12 +++--- libnvme/src/nvme/private-fabrics.h | 2 +- src/fabrics.c | 42 ++++++++++++------- 8 files changed, 55 insertions(+), 38 deletions(-) diff --git a/Documentation/nvme-connect-all.txt b/Documentation/nvme-connect-all.txt index 743da99c06..ffe6772cbf 100644 --- a/Documentation/nvme-connect-all.txt +++ b/Documentation/nvme-connect-all.txt @@ -14,6 +14,7 @@ SYNOPSIS [--config= | -J ] [--persistent[=] | -p] [--quiet] + [--no-reuse] [--nbft] [--no-nbft] [--nbft-path=] @@ -86,6 +87,9 @@ OPTIONS --quiet:: Suppress error messages. +--no-reuse:: + Always create a new connection, never reuse an existing one. + --nbft:: Only look at NBFT tables diff --git a/Documentation/nvme-discover.txt b/Documentation/nvme-discover.txt index 5068de7a41..c401a1d48b 100644 --- a/Documentation/nvme-discover.txt +++ b/Documentation/nvme-discover.txt @@ -14,6 +14,7 @@ SYNOPSIS [--config= | -J ] [--persistent[=] | -p] [--quiet] + [--no-reuse] [--force] [--nbft] [--no-nbft] @@ -107,10 +108,11 @@ OPTIONS --quiet:: Suppress already connected errors. +--no-reuse:: + Always create a new connection, never reuse an existing one. + --force:: - Disable the built-in persistent discovery controller connection - rules. Combined with --persistent flag, always create new - persistent discovery controller connection. + (deprecated, see --no-reuse) --nbft:: Only look at NBFT tables diff --git a/libnvme/src/accessors-fabrics.ld b/libnvme/src/accessors-fabrics.ld index 72d96838cf..777d49e6a6 100644 --- a/libnvme/src/accessors-fabrics.ld +++ b/libnvme/src/accessors-fabrics.ld @@ -29,7 +29,6 @@ LIBNVMF_ACCESSORS_3 { libnvmf_context_get_disable_sqflow; libnvmf_context_get_duplicate_connect; libnvmf_context_get_fast_io_fail_tmo; - libnvmf_context_get_force; libnvmf_context_get_hdr_digest; libnvmf_context_get_host_iface; libnvmf_context_get_host_traddr; @@ -40,6 +39,7 @@ LIBNVMF_ACCESSORS_3 { libnvmf_context_get_keyring; libnvmf_context_get_keyring_id; libnvmf_context_get_nbft_path; + libnvmf_context_get_no_reuse; libnvmf_context_get_nr_io_queues; libnvmf_context_get_nr_poll_queues; libnvmf_context_get_nr_write_queues; @@ -64,11 +64,11 @@ LIBNVMF_ACCESSORS_3 { libnvmf_context_set_disable_sqflow; libnvmf_context_set_duplicate_connect; libnvmf_context_set_fast_io_fail_tmo; - libnvmf_context_set_force; libnvmf_context_set_hdr_digest; libnvmf_context_set_keep_alive_tmo; libnvmf_context_set_keyring_id; libnvmf_context_set_nbft_path; + libnvmf_context_set_no_reuse; libnvmf_context_set_nr_io_queues; libnvmf_context_set_nr_poll_queues; libnvmf_context_set_nr_write_queues; diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index 83d5ec9393..c9df6c64d3 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -3749,10 +3749,11 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, if (err) return err; - if (!fctx->force) { + if (!fctx->no_reuse) { /* - * When --force is used, always create a controller, otherwise - * try to lookup an already existing controller first. + * When --no-reuse is used, always create a controller, + * otherwise try to lookup an already existing controller + * first. */ err = discover_lookup_ctrl(ctx, fctx, h, &c, &already_connected); @@ -3766,8 +3767,8 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, if (!c) { /* - * No existing controller or --force has been used, thus create - * a new controller. + * No existing controller or --no-reuse has been used, thus + * create a new controller. */ err = nvmf_create_discovery_ctrl(ctx, fctx, &fctx->ctrl_params, h, &c); if (err) { diff --git a/libnvme/src/nvme/generated/accessors-fabrics.c b/libnvme/src/nvme/generated/accessors-fabrics.c index 437570f0ad..038470a9d0 100644 --- a/libnvme/src/nvme/generated/accessors-fabrics.c +++ b/libnvme/src/nvme/generated/accessors-fabrics.c @@ -343,16 +343,16 @@ __shr_public bool libnvmf_context_get_connect(const struct libnvmf_context *p) return p->connect; } -__shr_public void libnvmf_context_set_force( +__shr_public void libnvmf_context_set_no_reuse( struct libnvmf_context *p, - bool force) + bool no_reuse) { - p->force = force; + p->no_reuse = no_reuse; } -__shr_public bool libnvmf_context_get_force(const struct libnvmf_context *p) +__shr_public bool libnvmf_context_get_no_reuse(const struct libnvmf_context *p) { - return p->force; + return p->no_reuse; } __shr_public void libnvmf_context_set_nbft_path( diff --git a/libnvme/src/nvme/generated/accessors-fabrics.h b/libnvme/src/nvme/generated/accessors-fabrics.h index adf5833f46..92305f9fa1 100644 --- a/libnvme/src/nvme/generated/accessors-fabrics.h +++ b/libnvme/src/nvme/generated/accessors-fabrics.h @@ -446,19 +446,19 @@ void libnvmf_context_set_connect(struct libnvmf_context *p, bool connect); bool libnvmf_context_get_connect(const struct libnvmf_context *p); /** - * libnvmf_context_set_force() - Set force. + * libnvmf_context_set_no_reuse() - Set no_reuse. * @p: The &struct libnvmf_context instance to update. - * @force: Value to assign to the force field. + * @no_reuse: Value to assign to the no_reuse field. */ -void libnvmf_context_set_force(struct libnvmf_context *p, bool force); +void libnvmf_context_set_no_reuse(struct libnvmf_context *p, bool no_reuse); /** - * libnvmf_context_get_force() - Get force. + * libnvmf_context_get_no_reuse() - Get no_reuse. * @p: The &struct libnvmf_context instance to query. * - * Return: The value of the force field. + * Return: The value of the no_reuse field. */ -bool libnvmf_context_get_force(const struct libnvmf_context *p); +bool libnvmf_context_get_no_reuse(const struct libnvmf_context *p); /** * libnvmf_context_set_nbft_path() - Set nbft_path. diff --git a/libnvme/src/nvme/private-fabrics.h b/libnvme/src/nvme/private-fabrics.h index 7876c9e5ab..530ff5c14b 100644 --- a/libnvme/src/nvme/private-fabrics.h +++ b/libnvme/src/nvme/private-fabrics.h @@ -75,7 +75,7 @@ struct libnvmf_context { // !generate-accessors:read=generated,write=generated /* discovery invocation options */ bool connect; // !access - bool force; // !access + bool no_reuse; // !access char *nbft_path; // !access /* host configuration */ diff --git a/src/fabrics.c b/src/fabrics.c index 279175ebf7..e8c5e1a08f 100644 --- a/src/fabrics.c +++ b/src/fabrics.c @@ -382,7 +382,7 @@ struct consume_state { struct libnvme_global_ctx *ctx; enum consume_mode mode; bool connect; - bool force; + bool no_reuse; nvme_print_flags_t flags; char *raw; const char *hostnqn; @@ -478,7 +478,7 @@ static void consume_conn(const struct libnvmf_config_conn *conn, libnvmf_context_set_default_keep_alive_timeout(fctx, NVMF_DEF_DISC_TMO); libnvmf_context_set_connect(fctx, st->connect); - libnvmf_context_set_force(fctx, st->force); + libnvmf_context_set_no_reuse(fctx, st->no_reuse); err = libnvmf_discover(st->ctx, fctx); } else { err = libnvmf_connect(st->ctx, fctx); @@ -521,7 +521,7 @@ int nvmf_convert_discovery_line(struct libnvmf_config_emitter *emitter, char *argv[MAX_DISC_ARGS] = { "discovery.conf" }; char *ptr, *p = line; int argc = 1; - bool force = false; + bool no_reuse = false; char *persistent_arg = NULL; NVMF_ARGS(opts, fa, @@ -529,8 +529,11 @@ int nvmf_convert_discovery_line(struct libnvmf_config_emitter *emitter, &persistent_arg, "persistent discovery connection mode " "(default: no; auto if given bare)"), - OPT_FLAG("force", 0, &force, - "Force persistent discovery controller creation")); + OPT_FLAG("no-reuse", 0, &no_reuse, + "always create a new connection, never reuse " + "an existing one"), + OPT_FLAG("force", 0, &no_reuse, + "deprecated, see --no-reuse")); if (line[0] == '#' || line[0] == '\n' || line[0] == '\0') return 0; @@ -692,7 +695,7 @@ static void load_nvme_fabrics_module(void) */ static int fabrics_discovery_config(struct libnvme_global_ctx *ctx, char *config_file, const char *hostnqn_arg, - const char *hostid_arg, bool connect, bool force, + const char *hostid_arg, bool connect, bool no_reuse, nvme_print_flags_t flags) { __cleanup_free char *ini_path = NULL; @@ -736,7 +739,7 @@ static int fabrics_discovery_config(struct libnvme_global_ctx *ctx, .ctx = ctx, .mode = CONSUME_ROLE_BASED, .connect = connect, - .force = force, + .no_reuse = no_reuse, .flags = flags, .raw = raw, .hostnqn = hostnqn, @@ -757,20 +760,21 @@ static int fabrics_discovery_config(struct libnvme_global_ctx *ctx, * exemption -- a mismatched or missing --owner is skipped the same way; * the escape hatch is passing the owner's own identity. * - * --force skips the check entirely: it means the caller will never reuse - * an existing controller, so there is nothing to check ownership against. + * --no-reuse skips the check entirely: it means the caller will never + * reuse an existing controller, so there is nothing to check ownership + * against. * * Returns 0 to proceed, 1 to skip, or a negative errno on a registry * read failure. */ static int check_ctrl_owner(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, - const char *owner, bool force) + const char *owner, bool no_reuse) { __cleanup_free char *reg_owner = NULL; int ret; - if (force) + if (no_reuse) return 0; ret = libnvmf_get_owner_from_fctx(ctx, fctx, ®_owner); @@ -798,7 +802,7 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) int ret; struct nvmf_args fa = { .subsysnqn = NVME_DISC_SUBSYS_NAME }; char *device = NULL; - bool force = false; + bool no_reuse = false; char *persistent_arg = NULL; const char *persistent; bool nbft = false, nonbft = false; @@ -813,7 +817,11 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) "persistent discovery connection mode " "(default: no; auto if given bare)"), OPT_STRING("config", 'J', "FILE", &config_file, nvmf_config_file), - OPT_FLAG("force", 0, &force, "Force persistent discovery controller creation"), + OPT_FLAG("no-reuse", 0, &no_reuse, + "always create a new connection, never reuse " + "an existing one"), + OPT_FLAG("force", 0, &no_reuse, + "deprecated, see --no-reuse"), OPT_FLAG("nbft", 0, &nbft, "Only look at NBFT tables"), OPT_FLAG("no-nbft", 0, &nonbft, "Do not look at NBFT tables"), OPT_STRING("owner", 0, "NAME", &owner, "record this owner in the registry"), @@ -889,7 +897,7 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) return ret; libnvmf_context_set_connect(fctx, connect); - libnvmf_context_set_force(fctx, force); + libnvmf_context_set_no_reuse(fctx, no_reuse); if (persistent && libnvmf_context_set_persistent(fctx, persistent)) { nvme_show_error( @@ -909,10 +917,12 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) } if (!nbft && config_file) ret = fabrics_discovery_config(ctx, config_file, - fa.hostnqn, fa.hostid, connect, force, flags); + fa.hostnqn, fa.hostid, connect, no_reuse, + flags); } else { ret = check_ctrl_owner(ctx, fctx, - owner ? owner : (nbft ? "nbft" : NULL), force); + owner ? owner : (nbft ? "nbft" : NULL), + no_reuse); if (ret < 0) { nvme_show_error("failed to check owner: %s", libnvme_strerror(-ret)); From 17dc3a378ec357eac068362ce1510f382454a076 Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Aug 2026 12:51:34 -0400 Subject: [PATCH 3/6] libnvme: use a Discovery Descriptor's own NQN when specified A DCNQNHOR cleared to 0h is spec-legal (NVMe Boot Specification rev 1.4): it means "no unique NQN, use the well-known Discovery NQN". get_heap_obj() reports that as -ENOENT, but read_discovery() treated any nonzero return as a hard parse failure and silently dropped the whole Discovery Descriptor -- including the common case, since most descriptors have no unique NQN at all. Tolerate -ENOENT for the NQN lookup specifically; a genuinely malformed reference (-EINVAL) still drops the descriptor. Once the descriptor's own nqn survives parsing, libnvmf_discover_nbft() can use it when present instead of always hardcoding the well-known Discovery NQN. Signed-off-by: Martin Belanger --- libnvme/src/nvme/fabrics.c | 3 ++- libnvme/src/nvme/nbft.c | 12 ++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index c9df6c64d3..cc348d6f4d 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -3569,7 +3569,8 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, strdup(libnvmf_get_default_trsvcid( uri->protocol, true)); - params.subsysnqn = NVME_DISC_SUBSYS_NAME; + params.subsysnqn = (*dd)->nqn ? + (*dd)->nqn : NVME_DISC_SUBSYS_NAME; params.transport = uri->protocol; params.traddr = uri->host; params.trsvcid = trsvcid; diff --git a/libnvme/src/nvme/nbft.c b/libnvme/src/nvme/nbft.c index f419d9512f..c5853ff93b 100644 --- a/libnvme/src/nvme/nbft.c +++ b/libnvme/src/nvme/nbft.c @@ -567,8 +567,16 @@ static int read_discovery(struct libnvme_global_ctx *ctx, 1, &discovery->uri)) goto error; - if (get_heap_obj(ctx, raw_discovery, discovery_ctrl_nqn_obj, - 1, &discovery->nqn)) + /* + * A DCNQNHOR cleared to 0h is spec-legal: it means "no unique NQN, + * use the well-known Discovery NQN" (Boot Specification rev 1.4). + * get_heap_obj() reports that as -ENOENT, not a parse failure -- + * only a genuinely malformed reference (-EINVAL) should drop the + * whole descriptor. + */ + r = get_heap_obj(ctx, raw_discovery, discovery_ctrl_nqn_obj, + 1, &discovery->nqn); + if (r && r != -ENOENT) goto error; discovery->hfi = hfi_from_index(nbft, raw_discovery->hfi_index); From 3293595021474aec08553767d096d04d520c686d Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Aug 2026 12:59:48 -0400 Subject: [PATCH 4/6] libnvme: unify discovery ctrl lookup-or-create into dc_open() discover_lookup_ctrl()/discover_lookup_ctrl_by_device() only ever resolved a controller; libnvmf_discover() then had its own separate create-fallback and its own already_connected bool to track what it found. Fold both into dc_open(), returning enum dc_ownership {DC_OWNED, DC_BORROWED} instead of the bool. Also stop overwriting fctx->persistent to NO when --device doesn't resolve to a real controller (not found, or found but not a discovery controller). fctx->persistent must keep reflecting what the user actually asked for. Signed-off-by: Martin Belanger --- libnvme/src/nvme/fabrics.c | 129 ++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 73 deletions(-) diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index cc348d6f4d..14dd0a5b6d 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -3621,27 +3621,22 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, return ret; } -static struct libnvme_ctrl *discover_lookup_ctrl_by_device( - struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, - bool *already_connected) +enum dc_ownership { + DC_OWNED, + DC_BORROWED, +}; + +static struct libnvme_ctrl *dc_open_by_device(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, enum dc_ownership *own) { struct libnvme_ctrl *c; int err; err = libnvme_scan_ctrl(ctx, fctx->device, &c); if (err) { - /* - * No controller found, fall back to create one. - * But that controller cannot be persistent. - */ + /* No controller found, fall back to creating one. */ libnvme_msg(ctx, LIBNVME_LOG_ERR, - "ctrl device %s not found%s\n", fctx->device, - fctx->persistent == LIBNVMF_PERSISTENT_AUTO || - fctx->persistent == LIBNVMF_PERSISTENT_FORCE ? - ", ignoring --persistent" : ""); - - fctx->persistent = LIBNVMF_PERSISTENT_NO; - + "ctrl device %s not found\n", fctx->device); return NULL; } @@ -3657,8 +3652,6 @@ static struct libnvme_ctrl *discover_lookup_ctrl_by_device( "ctrl device %s found, ignoring non discovery controller\n", fctx->device); - fctx->persistent = LIBNVMF_PERSISTENT_NO; - libnvme_free_ctrl(c); return NULL; } @@ -3669,7 +3662,7 @@ static struct libnvme_ctrl *discover_lookup_ctrl_by_device( * locally instead of overriding fctx->persistent, which must keep * reflecting what the user actually asked for. */ - *already_connected = true; + *own = DC_BORROWED; /* * When --host-traddr/--host-iface are not specified on the @@ -3691,41 +3684,58 @@ static struct libnvme_ctrl *discover_lookup_ctrl_by_device( return c; } -static int discover_lookup_ctrl(struct libnvme_global_ctx *ctx, - struct libnvmf_context *fctx, struct libnvme_host *h, - struct libnvme_ctrl **ctrl, bool *already_connected) +/* + * Resolve the primary discovery controller connection: reuse one via + * --device if given, reuse one found by matching connection parameters + * otherwise, or create a fresh one if --no-reuse was given or neither + * lookup found anything. Never touches fctx->persistent; ownership is + * reported separately so the caller knows what it may disconnect later. + */ +static int dc_open(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, + struct libnvme_host *h, enum dc_ownership *own, + struct libnvme_ctrl **ctrl) { struct libnvme_ctrl *c = NULL; int err; - if (fctx->device) - c = discover_lookup_ctrl_by_device(ctx, fctx, - already_connected); + *own = DC_OWNED; - if (!c) { - c = lookup_ctrl(h, &fctx->ctrl_params); - if (c) { - /* - * It was not created by us: record that fact - * locally, do not touch fctx->persistent. - */ - *already_connected = true; + if (!fctx->no_reuse) { + if (fctx->device) + c = dc_open_by_device(ctx, fctx, own); + + if (!c) { + c = lookup_ctrl(h, &fctx->ctrl_params); + if (c) + *own = DC_BORROWED; } } - if (!c) - return 0; - - if (!libnvme_ctrl_get_transport_handle(c)) { + if (c) { + if (!libnvme_ctrl_get_transport_handle(c)) { + /* + * When we found an existing controller it might not + * have a device handle yet + */ + err = libnvme_open(ctx, c->name, O_RDONLY, &c->hdl); + if (err) { + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "failed to open %s\n", c->name); + return err; + } + } + } else { /* - * When we found an existing controller it might not have a - * device handle yet + * No existing controller, or --no-reuse was given: create a + * new one. */ - err = libnvme_open(ctx, c->name, O_RDONLY, &c->hdl); + err = nvmf_create_discovery_ctrl(ctx, fctx, &fctx->ctrl_params, + h, &c); if (err) { - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "failed to open %s\n", c->name); - + if (err != -ENVME_CONNECT_IGNORED) + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "failed to add controller, error %s\n", + libnvme_strerror(-err)); return err; } } @@ -3739,7 +3749,7 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, { struct libnvme_ctrl *c = NULL; struct libnvme_host *h; - bool already_connected = false; + enum dc_ownership own; int err; err = libnvme_get_host(ctx, fctx->hostnqn, fctx->hostid, &h); @@ -3750,39 +3760,12 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, if (err) return err; - if (!fctx->no_reuse) { - /* - * When --no-reuse is used, always create a controller, - * otherwise try to lookup an already existing controller - * first. - */ - err = discover_lookup_ctrl(ctx, fctx, h, &c, - &already_connected); - if (err) { - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "failed to lookup controller, error %s\n", - libnvme_strerror(-err)); - return err; - } - } - - if (!c) { - /* - * No existing controller or --no-reuse has been used, thus - * create a new controller. - */ - err = nvmf_create_discovery_ctrl(ctx, fctx, &fctx->ctrl_params, h, &c); - if (err) { - if (err != -ENVME_CONNECT_IGNORED) - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "failed to add controller, error %s\n", - libnvme_strerror(-err)); - return err; - } - } + err = dc_open(ctx, fctx, h, &own, &c); + if (err) + return err; err = _nvmf_discover(ctx, fctx, &fctx->ctrl_params, c, true, - already_connected); + own == DC_BORROWED); libnvme_free_ctrl(c); return err; From 1bfd9a12f8b2fa879b1d81ff0a40256793c607ad Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Aug 2026 13:19:08 -0400 Subject: [PATCH 5/6] libnvme: rewrite _nvmf_discover() as a depth-capped, deduped walk _nvmf_discover() had four real bugs, all living in code this rewrite already has to touch. An already-connected referral was skipped entirely, without ever walking its own referrals. A DC's self entry was identified by pointer identity (cl == c), which is wrong per the spec's SUBTYPE 03h semantics. A referral's KATO was requested after the connect already happened, too late to take effect. And hooks.connected never fired for anything discovered and connected during the walk itself. Rewrite it as a two-pass, depth-capped (NVMF_MAX_REFERRAL_DEPTH, 8 per spec), TID-deduped walk. Pass 1 finds the self entry and decides this DC's own disconnect fate via the new dc_decide() -- a pure function, unit-tested directly, that gives every DC (primary or referral) the same self-entry-driven decision, falling back to the parent's reported EFLAGS only when a referral has no self entry of its own. Pass 2 walks NVM subsystem entries (connect, never recurse) and referrals (depth-cap check, then visited-set check, then reuse-or-connect, then recurse), fixing all four bugs above along the way. New: dc_decide(), dc_already_connected() (dedups a hook-invocation pattern), the dc_visited TID set, dc_walk() (owns the visited-set lifetime so libnvmf_discover() doesn't need to know it exists). Deleted: dc_should_connect(), superseded by dc_decide() plus the explicit subtype branches in pass 2. Also drops nvmf_connect_disc_entry()'s dead NVME_NQN_CURR handling and its unused discover out-parameter -- self entries never reach it now that pass 1 filters them first. Signed-off-by: Martin Belanger --- libnvme/src/nvme/fabrics.c | 444 ++++++++++++++++++++++++----------- libnvme/tests/test-fabrics.c | 74 ++++++ 2 files changed, 379 insertions(+), 139 deletions(-) diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index 14dd0a5b6d..8b592ff913 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -35,6 +35,7 @@ #include #include +#include #include #include @@ -1813,7 +1814,7 @@ static bool nvmf_excluded(struct libnvme_global_ctx *ctx, static int nvmf_connect_disc_entry(libnvme_host_t h, struct nvmf_disc_log_entry *e, struct libnvme_ctrl_params *params, - bool *discover, libnvme_ctrl_t *cp) + libnvme_ctrl_t *cp) { libnvme_ctrl_t c; int ret; @@ -1880,15 +1881,13 @@ static int nvmf_connect_disc_entry(libnvme_host_t h, return ret; } + /* + * Self entries (SUBTYPE 03h) are filtered out by the caller before + * this function is ever reached -- they never need a connection of + * their own. + */ switch (e->subtype) { - case NVME_NQN_CURR: - libnvme_ctrl_set_discovered(c, true); - libnvme_ctrl_set_unique_discovery_ctrl(c, - strcmp(e->subnqn, NVME_DISC_SUBSYS_NAME)); - break; case NVME_NQN_DISC: - if (discover) - *discover = true; libnvme_ctrl_set_discovery_ctrl(c, true); libnvme_ctrl_set_unique_discovery_ctrl(c, strcmp(e->subnqn, NVME_DISC_SUBSYS_NAME)); @@ -1903,11 +1902,6 @@ static int nvmf_connect_disc_entry(libnvme_host_t h, break; } - if (libnvme_ctrl_get_discovered(c)) { - libnvme_free_ctrl(c); - return -EAGAIN; - } - if (e->treq & NVMF_TREQ_DISABLE_SQFLOW && nvmf_check_option(h->ctx, disable_sqflow)) c->cfg.disable_sqflow = true; @@ -2638,7 +2632,7 @@ static int set_discovery_kato(struct libnvmf_context *fctx, int tmo = params->cfg.keep_alive_tmo; /* * EPCSD isn't known until after the Discovery Log Page comes back, so - * auto mode optimistically requests a KATO here; dc_should_connect() + * auto mode optimistically requests a KATO here; dc_decide() * disconnects per entry afterward if EPCSD turns out to be unset. */ bool wants_kato = fctx->persistent == LIBNVMF_PERSISTENT_AUTO || @@ -2682,11 +2676,44 @@ static void nvme_parse_tls_args(const char *keyring, const char *tls_key, } } -static bool dc_should_disconnect(struct libnvmf_context *fctx, - const char *subnqn, __u16 eflags) +enum dc_ownership { + DC_OWNED, + DC_BORROWED, +}; + +/* + * Decide whether a discovery controller connection should be disconnected + * once its own Discovery Log Page has been fully walked. Pure function, + * no I/O, so it is directly unit-testable. + * + * @own: DC_BORROWED means this connection pre-existed the walk and is + * never ours to disconnect, regardless of persistence mode. + * @self_seen: whether this DC's own self entry (SUBTYPE 03h, "current + * discovery subsystem") was found in its own Discovery Log + * Page. + * @self_eflags: that self entry's EFLAGS; meaningful only if @self_seen. + * @parent_eflags: the EFLAGS this DC's own referral entry carried in its + * parent's Discovery Log Page, or NULL if this DC has no + * parent (the primary). Used only as a fallback when + * @self_seen is false. + */ +static bool dc_decide(struct libnvmf_context *fctx, const char *subnqn, + enum dc_ownership own, bool self_seen, __u16 self_eflags, + const __u16 *parent_eflags) { + __u16 eflags; bool disconnect; + if (own == DC_BORROWED) + return false; + + if (self_seen) + eflags = self_eflags; + else if (parent_eflags) + eflags = *parent_eflags; + else + eflags = 0; + switch (fctx->persistent) { case LIBNVMF_PERSISTENT_FORCE: /* Persist regardless of what EPCSD reports. */ @@ -2709,26 +2736,6 @@ static bool dc_should_disconnect(struct libnvmf_context *fctx, return disconnect; } -static bool dc_should_connect(struct libnvmf_context *fctx, - struct nvmf_disc_log_entry *e, bool *pdisconnect) -{ - __u16 eflags; - - if (e->subtype == NVME_NQN_NVME) { - *pdisconnect = false; - return fctx->connect; - } - - eflags = le16_to_cpu(e->eflags); - - /* Discovery controller returns duplicate information. */ - if (eflags & NVMF_DISC_EFLAGS_DUPRETINFO) - return false; - - *pdisconnect = dc_should_disconnect(fctx, e->subnqn, eflags); - return true; -} - /* * Bundles the controller and the decisions made about it while walking a * Discovery Log Page, so the whole outcome for one entry (or, with @e @@ -2771,16 +2778,226 @@ static void dc_log_decision(struct libnvmf_context *fctx, reason ? reason : ""); } +static void dc_already_connected(struct libnvmf_context *fctx, + libnvme_host_t h, struct nvmf_disc_log_entry *e) +{ + if (fctx->hooks.already_connected) + fctx->hooks.already_connected(fctx, h, e->subnqn, + libnvmf_trtype_str(e->trtype), e->traddr, + e->trsvcid, fctx->hooks.user_data); +} + +/* + * The spec does not require processing referral entries deeper than + * eight levels. + */ +#define NVMF_MAX_REFERRAL_DEPTH 8 + +/* + * Every discovery controller visited during one top-level walk, keyed by + * TID. The depth cap alone is not a cycle guard -- a referral graph can + * cycle back to an earlier DC within eight hops -- so this is what + * actually stops the walk from looping. + */ +SHR_PTRARRAY_DEFINE(dc_visited, struct libnvmf_tid); + +static bool dc_visited_has(const struct dc_visited *v, + const struct libnvmf_tid *tid) +{ + const char *canon = libnvmf_tid_get_canonical(tid); + + if (!canon) + return false; + + for (size_t i = 0; i < v->len; i++) { + const char *seen = libnvmf_tid_get_canonical(v->items[i]); + + if (seen && !strcmp(seen, canon)) + return true; + } + + return false; +} + +static inline void free_libnvmf_tid(struct libnvmf_tid **tid) +{ + libnvmf_tid_free(*tid); +} +#define __cleanup_libnvmf_tid __cleanup(free_libnvmf_tid) + +/* + * A leaf entry (NVME_NQN_NVME, an I/O controller) is a dead end for the + * walk: connect it if --connect was requested, and there is nothing + * further to recurse into. + */ +static void dc_connect_leaf_entry(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, struct libnvme_host *h, + struct nvmf_disc_log_entry *e, + struct libnvme_ctrl_params *params) +{ + struct dc_decision d = { 0 }; + int err; + + if (!fctx->connect) { + dc_log_decision(fctx, e, &d, "connect not requested"); + return; + } + + err = nvmf_connect_disc_entry(h, e, params, &d.c); + if (d.c) { + d.connect = true; + dc_log_decision(fctx, e, &d, NULL); + if (fctx->hooks.connected) + fctx->hooks.connected(fctx, d.c, fctx->hooks.user_data); + } else if (err == -ENVME_CONNECT_ALREADY) { + dc_already_connected(fctx, h, e); + } else { + dc_log_decision(fctx, e, &d, libnvme_strerror(-err)); + } +} + +static int _nvmf_discover(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, + const struct libnvme_ctrl_params *ctrl_params, + struct libnvme_ctrl *c, enum dc_ownership own, int depth, + struct dc_visited *visited, const __u16 *parent_eflags); + +/* + * A referral entry (NVME_NQN_DISC) is a branch point: it opens up another + * Discovery Service to walk, unlike a leaf entry. + */ +static void dc_walk_referral(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, struct libnvme_host *h, + struct nvmf_disc_log_entry *e, + struct libnvme_ctrl_params *params, int depth, + struct dc_visited *visited, __u16 eflags) +{ + __cleanup_libnvmf_tid struct libnvmf_tid *tid = NULL; + struct dc_decision d = { 0 }; + enum dc_ownership child_own; + libnvme_ctrl_t cl; + int err; + + if (eflags & NVMF_DISC_EFLAGS_DUPRETINFO) { + dc_log_decision(fctx, e, &d, "duplicate information"); + return; + } + + if (depth >= NVMF_MAX_REFERRAL_DEPTH) { + dc_log_decision(fctx, e, &d, + "referral depth limit reached, not descending"); + return; + } + + if (libnvmf_tid_from_fields(params->transport, params->traddr, + params->trsvcid, params->subsysnqn, + params->host_traddr, params->host_iface, + fctx->hostnqn, fctx->hostid, &tid)) { + dc_log_decision(fctx, e, &d, "invalid entry"); + return; + } + + if (dc_visited_has(visited, tid)) { + dc_log_decision(fctx, e, &d, "already visited this walk"); + return; + } + + cl = lookup_ctrl(h, params); + if (cl && libnvme_ctrl_get_name(cl)) { + d.c = cl; + d.already_connected = true; + child_own = DC_BORROWED; + } else { + set_discovery_kato(fctx, params); + err = nvmf_connect_disc_entry(h, e, params, &d.c); + if (!d.c) { + if (err == -ENVME_CONNECT_ALREADY) + dc_already_connected(fctx, h, e); + else + dc_log_decision(fctx, e, &d, + libnvme_strerror(-err)); + return; + } + child_own = DC_OWNED; + if (fctx->hooks.connected) + fctx->hooks.connected(fctx, d.c, fctx->hooks.user_data); + } + d.connect = true; + dc_log_decision(fctx, e, &d, NULL); + + if (tid) { + if (!dc_visited_append(visited, tid)) + tid = NULL; /* ownership transferred */ + } + + /* + * The child decides its own fate (disconnect or persist) + * internally, the same way this DC just did above for + * itself -- so the free below is unconditional, matching + * how libnvmf_discover() already treats its own primary c. + */ + _nvmf_discover(ctx, fctx, params, d.c, child_own, depth + 1, + visited, &eflags); + libnvme_free_ctrl(d.c); +} + +/* + * Pass 1: a first pass over the DLP entries to sanitize them and survey + * this DC's own self entry (SUBTYPE 03h) -- the only place this DC's own + * EPCSD is ever reported. Returns whether c should be disconnected once + * its own Discovery Log Page has been fully walked. + */ +static bool dc_survey_self_entry(struct libnvmf_context *fctx, + struct libnvme_ctrl *c, enum dc_ownership own, bool primary, + const __u16 *parent_eflags, struct nvmf_discovery_log *log, + uint64_t numrec) +{ + struct nvmf_disc_log_entry *self_entry = NULL; + struct dc_decision d = { + .c = c, + .primary = primary, + .already_connected = own == DC_BORROWED, + }; + bool self_seen; + __u16 self_eflags; + + for (uint64_t i = 0; i < numrec; i++) { + struct nvmf_disc_log_entry *e = &log->entries[i]; + + sanitize_discovery_log_entry(c->ctx, e); + if (e->subtype == NVME_NQN_CURR) + self_entry = e; + } + + self_seen = self_entry != NULL; + self_eflags = self_seen ? le16_to_cpu(self_entry->eflags) : 0; + + d.disconnect = dc_decide(fctx, libnvme_ctrl_get_subsysnqn(c), own, + self_seen, self_eflags, parent_eflags); + + if (own == DC_BORROWED) + dc_log_decision(fctx, self_entry, &d, + "pre-existing, not ours to disconnect"); + else if (self_entry) + dc_log_decision(fctx, self_entry, &d, NULL); + else + dc_log_decision(fctx, NULL, &d, + "no self entry, EPCSD assumed 0"); + + return d.disconnect; +} + static int _nvmf_discover(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, const struct libnvme_ctrl_params *ctrl_params, - struct libnvme_ctrl *c, bool primary, bool already_connected) + struct libnvme_ctrl *c, enum dc_ownership own, int depth, + struct dc_visited *visited, const __u16 *parent_eflags) { __cleanup_free struct nvmf_discovery_log *log = NULL; libnvme_subsystem_t s = libnvme_ctrl_get_subsystem(c); libnvme_host_t h = libnvme_subsystem_get_host(s); - struct nvmf_disc_log_entry *self_entry = NULL; uint64_t numrec; + bool disconnect; int err; struct libnvmf_discovery_args args = { @@ -2802,117 +3019,73 @@ static int _nvmf_discover(struct libnvme_global_ctx *ctx, fctx->hooks.discovery_log(fctx, log, numrec, fctx->hooks.user_data); - for (int i = 0; i < numrec; i++) { + /* + * Pass 1: survey c's own self entry, and determine if safe to + * disconnect. + */ + disconnect = dc_survey_self_entry(fctx, c, own, !parent_eflags, + parent_eflags, log, numrec); + if (disconnect) + libnvmf_disconnect_ctrl(c); + + /* + * Pass 2: everything else -- referrals to walk, NVM subsystem + * entries to connect per --connect. Self entries are already + * handled above. + */ + for (uint64_t i = 0; i < numrec; i++) { struct nvmf_disc_log_entry *e = &log->entries[i]; - struct dc_decision d = { 0 }; - libnvme_ctrl_t cl; - bool discover = false; __cleanup_ctrl_params struct libnvme_ctrl_params params = ctrl_params_dup(ctrl_params); + struct dc_decision d = { 0 }; + const char *transport; + __u16 eflags; - sanitize_discovery_log_entry(c->ctx, e); - - params.subsysnqn = e->subnqn; - params.transport = libnvmf_trtype_str(e->trtype); - params.traddr = e->traddr; - params.trsvcid = e->trsvcid; - - /* Already connected ? */ - cl = lookup_ctrl(h, ¶ms); - if (cl == c) { - /* - * This entry describes c's own port (SUBTYPE 03h, - * "current discovery subsystem"). It is the only - * place c's own EPCSD is ever reported, so remember - * it instead of silently discarding it below. - */ - if (!self_entry) - self_entry = e; - d.c = c; - d.already_connected = true; - dc_log_decision(fctx, e, &d, - "self entry, decision deferred to primary"); - continue; - } - if (cl && libnvme_ctrl_get_name(cl)) { - d.c = cl; - d.already_connected = true; - dc_log_decision(fctx, e, &d, "already connected"); + if (e->subtype == NVME_NQN_CURR) continue; - } /* Skip connect if the transport types don't match */ - if (strcmp(libnvme_ctrl_get_transport(c), - params.transport)) { + transport = libnvmf_trtype_str(e->trtype); + if (strcmp(libnvme_ctrl_get_transport(c), transport)) { dc_log_decision(fctx, e, &d, "transport mismatch"); continue; } - if (!dc_should_connect(fctx, e, &d.disconnect)) { - dc_log_decision(fctx, e, &d, - "not connecting (duplicate info, or connect not requested)"); + params.subsysnqn = e->subnqn; + params.transport = transport; + params.traddr = e->traddr; + params.trsvcid = e->trsvcid; + eflags = le16_to_cpu(e->eflags); + + if (e->subtype == NVME_NQN_NVME) { + dc_connect_leaf_entry(ctx, fctx, h, e, ¶ms); continue; } - d.connect = true; - err = nvmf_connect_disc_entry(h, e, ¶ms, &discover, &d.c); - if (err && err != -ENVME_CONNECT_ALREADY) - dc_log_decision(fctx, e, &d, libnvme_strerror(-err)); - else - dc_log_decision(fctx, e, &d, NULL); - - if (d.c) { - if (discover) { - set_discovery_kato(fctx, ¶ms); - _nvmf_discover(ctx, fctx, ¶ms, d.c, false, - false); - } - - if (d.disconnect) { - libnvmf_disconnect_ctrl(d.c); - libnvme_free_ctrl(d.c); - } - } else if (err == -ENVME_CONNECT_ALREADY) { - fctx->hooks.already_connected(fctx, h, e->subnqn, - libnvmf_trtype_str(e->trtype), e->traddr, - e->trsvcid, fctx->hooks.user_data); - } + /* NVME_NQN_DISC: a referral to another Discovery Service. */ + dc_walk_referral(ctx, fctx, h, e, ¶ms, depth, visited, + eflags); } - /* - * c has no parent to make this decision for it. Its own EPCSD is - * only ever reported in its own self entry (SUBTYPE 03h); an - * absent self entry means EPCSD is not reported, which the spec - * defines identically to EPCSD=0. - */ - if (primary) { - struct dc_decision d = { - .c = c, - .primary = true, - .already_connected = already_connected, - }; - - if (already_connected) { - dc_log_decision(fctx, self_entry, &d, - "pre-existing, not ours to disconnect"); - } else { - __u16 eflags = self_entry ? - le16_to_cpu(self_entry->eflags) : 0; + return 0; +} - d.disconnect = dc_should_disconnect(fctx, - libnvme_ctrl_get_subsysnqn(c), eflags); - if (self_entry) - dc_log_decision(fctx, self_entry, &d, NULL); - else - dc_log_decision(fctx, NULL, &d, - "no self entry, EPCSD assumed 0"); +/* + * Owns the whole-walk visited-set lifetime, so callers don't need to know + * it exists. c has no parent -- it is the primary discovery controller + * connection -- so the walk starts at depth 0 with no parent eflags. + */ +static int dc_walk(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, + struct libnvme_ctrl *c, enum dc_ownership own) +{ + struct dc_visited visited = { 0 }; + int err; - if (d.disconnect) - libnvmf_disconnect_ctrl(c); - } - } + err = _nvmf_discover(ctx, fctx, &fctx->ctrl_params, c, own, 0, + &visited, NULL); + dc_visited_free(&visited); - return 0; + return err; } __shr_public const char *libnvmf_get_default_trsvcid(const char *transport, @@ -3253,8 +3426,7 @@ static int nbft_discovery(struct libnvme_global_ctx *ctx, if (e->subtype == NVME_NQN_DISC) { libnvme_ctrl_t child; - ret = nvmf_connect_disc_entry(h, e, ¶ms, - NULL, &child); + ret = nvmf_connect_disc_entry(h, e, ¶ms, &child); if (ret) continue; nbft_discovery(ctx, fctx, ¶ms, dd, h, child); @@ -3621,11 +3793,6 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, return ret; } -enum dc_ownership { - DC_OWNED, - DC_BORROWED, -}; - static struct libnvme_ctrl *dc_open_by_device(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, enum dc_ownership *own) { @@ -3764,8 +3931,7 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, if (err) return err; - err = _nvmf_discover(ctx, fctx, &fctx->ctrl_params, c, true, - own == DC_BORROWED); + err = dc_walk(ctx, fctx, c, own); libnvme_free_ctrl(c); return err; diff --git a/libnvme/tests/test-fabrics.c b/libnvme/tests/test-fabrics.c index be9ed2d385..d7e22345e6 100644 --- a/libnvme/tests/test-fabrics.c +++ b/libnvme/tests/test-fabrics.c @@ -640,6 +640,79 @@ static bool test_unescape_uri(void) return pass; } +/* ------------------------------------------------------------------------- + * dc_decide — pure disconnect-decision logic for one discovery controller + * ------------------------------------------------------------------------- + */ +static bool test_dc_decide(struct libnvme_global_ctx *ctx) +{ + struct libnvmf_context fctx = { .ctx = ctx }; + __u16 epcsd_set = NVMF_DISC_EFLAGS_EPCSD; + __u16 epcsd_clear = 0; + bool pass = true, p, d; + + printf("\ntest_dc_decide:\n"); + + /* DC_BORROWED: never ours to disconnect, regardless of mode/EPCSD. */ + fctx.persistent = LIBNVMF_PERSISTENT_FORCE; + d = dc_decide(&fctx, "nqn", DC_BORROWED, true, epcsd_set, NULL); + p = !d; + CHECK(p, "DC_BORROWED, FORCE, self EPCSD=1: disconnect=%d", d); + pass &= p; + + fctx.persistent = LIBNVMF_PERSISTENT_NO; + d = dc_decide(&fctx, "nqn", DC_BORROWED, false, 0, NULL); + p = !d; + CHECK(p, "DC_BORROWED, NO, no self entry: disconnect=%d", d); + pass &= p; + + /* DC_OWNED, AUTO: self entry's own EPCSD decides. */ + fctx.persistent = LIBNVMF_PERSISTENT_AUTO; + d = dc_decide(&fctx, "nqn", DC_OWNED, true, epcsd_set, NULL); + p = !d; + CHECK(p, "DC_OWNED, AUTO, self EPCSD=1: disconnect=%d", d); + pass &= p; + + d = dc_decide(&fctx, "nqn", DC_OWNED, true, epcsd_clear, NULL); + p = d; + CHECK(p, "DC_OWNED, AUTO, self EPCSD=0: disconnect=%d", d); + pass &= p; + + /* DC_OWNED, AUTO, no self entry: fall back to the parent's view. */ + d = dc_decide(&fctx, "nqn", DC_OWNED, false, 0, &epcsd_set); + p = !d; + CHECK(p, "DC_OWNED, AUTO, no self entry, parent EPCSD=1: disconnect=%d", + d); + pass &= p; + + d = dc_decide(&fctx, "nqn", DC_OWNED, false, 0, &epcsd_clear); + p = d; + CHECK(p, "DC_OWNED, AUTO, no self entry, parent EPCSD=0: disconnect=%d", + d); + pass &= p; + + /* DC_OWNED, AUTO, no self entry and no parent (primary): assume 0. */ + d = dc_decide(&fctx, "nqn", DC_OWNED, false, 0, NULL); + p = d; + CHECK(p, "DC_OWNED, AUTO, no self entry, no parent: disconnect=%d", d); + pass &= p; + + /* FORCE/NO override EPCSD either way. */ + fctx.persistent = LIBNVMF_PERSISTENT_FORCE; + d = dc_decide(&fctx, "nqn", DC_OWNED, true, epcsd_clear, NULL); + p = !d; + CHECK(p, "DC_OWNED, FORCE, self EPCSD=0: disconnect=%d", d); + pass &= p; + + fctx.persistent = LIBNVMF_PERSISTENT_NO; + d = dc_decide(&fctx, "nqn", DC_OWNED, true, epcsd_set, NULL); + p = d; + CHECK(p, "DC_OWNED, NO, self EPCSD=1: disconnect=%d", d); + pass &= p; + + return pass; +} + /* ------------------------------------------------------------------------- * main * ------------------------------------------------------------------------- @@ -674,6 +747,7 @@ int main(int argc, char *argv[]) test_traddr_is_hostname(ctx); test_nvmf_sanitize_addrs(ctx); test_unescape_uri(); + test_dc_decide(ctx); libnvme_free_global_ctx(ctx); From bc0ffbc698a94fb8d555b502362fa737b109f9ea Mon Sep 17 00:00:00 2001 From: Martin Belanger Date: Thu, 13 Aug 2026 13:47:55 -0400 Subject: [PATCH 6/6] libnvme: fold NBFT discovery onto the shared walker nbft_discovery() was a forked copy of _nvmf_discover()'s per-DC walk, missing the depth cap and visited-set the general path just gained. Replace it with dc_open()/dc_walk(), the same machinery discover and connect-all use, via a new private connect_leaf hook so NBFT's leaf-connect quirks (DHCP retry, firing hooks.connected) stay NBFT-only without forking the walk itself. NBFT never honors --no-reuse (dc_open() gains honor_no_reuse) and never respects --persistent (fctx->persistent forced to NO before the walk): boot discovery is a one-shot operation where reusing an existing connection is strictly better, and neither flag was ever consulted by the old code either. Two real, pre-existing bugs found and fixed along the way: host_iface was freed before nbft_discovery()'s walk read it from every per-entry connect during that DC's DLP walk (moot now -- ctrl_params_dup()'s __cleanup_ctrl_params already prevents this by construction); and the Discovery Descriptor connection's own DHCP retry cleared traddr (the destination) instead of host_traddr (the local address), which cannot ever succeed. Also adds lookup_live_ctrl(), deduplicating a lookup_ctrl() + name check repeated at four call sites, one of them new here; and dc_visited_register(), so a cycle back to a DC (including the primary itself) is recognized from its own real connected identity, not just whatever a referring entry happened to report about it. Signed-off-by: Martin Belanger --- libnvme/src/nvme/fabrics.c | 556 ++++++++++++++++------------- libnvme/src/nvme/private-fabrics.h | 12 + 2 files changed, 319 insertions(+), 249 deletions(-) diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index 8b592ff913..3b4243217c 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -2536,6 +2536,19 @@ static libnvme_ctrl_t lookup_ctrl(libnvme_host_t h, return NULL; } +/* + * Same as lookup_ctrl(), but only returns a controller that's actually + * connected. lookup_ctrl() can also match a scanned-but-unconnected + * draft, which has no kernel-assigned name yet. + */ +static libnvme_ctrl_t lookup_live_ctrl(libnvme_host_t h, + const struct libnvme_ctrl_params *params) +{ + libnvme_ctrl_t c = lookup_ctrl(h, params); + + return (c && libnvme_ctrl_get_name(c)) ? c : NULL; +} + __shr_public int libnvmf_get_owner_from_tid(struct libnvme_global_ctx *ctx, const struct libnvmf_tid *tid, char **owner) { @@ -2819,6 +2832,32 @@ static bool dc_visited_has(const struct dc_visited *v, return false; } +/* + * Registers c's own real, connected identity -- not whatever a referring + * entry claimed about it -- so a cycle that loops back to c (including + * back to the primary) is recognized even if reached via a differently + * reported path. Best-effort: a TID construction failure here just means + * this one DC isn't deduplicated, not a fatal error for the walk. + */ +static void dc_visited_register(struct dc_visited *visited, + struct libnvme_ctrl *c, struct libnvmf_context *fctx) +{ + struct libnvmf_tid *tid; + int err; + + err = libnvmf_tid_from_fields(libnvme_ctrl_get_transport(c), + libnvme_ctrl_get_traddr(c), libnvme_ctrl_get_trsvcid(c), + libnvme_ctrl_get_subsysnqn(c), + libnvme_ctrl_get_host_traddr(c), + libnvme_ctrl_get_host_iface(c), + fctx->hostnqn, fctx->hostid, &tid); + if (err) + return; + + if (dc_visited_append(visited, tid)) + libnvmf_tid_free(tid); +} + static inline void free_libnvmf_tid(struct libnvmf_tid **tid) { libnvmf_tid_free(*tid); @@ -2843,12 +2882,21 @@ static void dc_connect_leaf_entry(struct libnvme_global_ctx *ctx, return; } - err = nvmf_connect_disc_entry(h, e, params, &d.c); + if (fctx->hooks.connect_leaf) { + /* + * NBFT's own leaf-connect quirks (DHCP retry, firing + * hooks.connected) apply instead -- it fires the hook + * itself. + */ + err = fctx->hooks.connect_leaf(ctx, fctx, h, e, params, &d.c); + } else { + err = nvmf_connect_disc_entry(h, e, params, &d.c); + if (d.c && fctx->hooks.connected) + fctx->hooks.connected(fctx, d.c, fctx->hooks.user_data); + } if (d.c) { d.connect = true; dc_log_decision(fctx, e, &d, NULL); - if (fctx->hooks.connected) - fctx->hooks.connected(fctx, d.c, fctx->hooks.user_data); } else if (err == -ENVME_CONNECT_ALREADY) { dc_already_connected(fctx, h, e); } else { @@ -2860,7 +2908,8 @@ static int _nvmf_discover(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, const struct libnvme_ctrl_params *ctrl_params, struct libnvme_ctrl *c, enum dc_ownership own, int depth, - struct dc_visited *visited, const __u16 *parent_eflags); + struct dc_visited *visited, const __u16 *parent_eflags, + bool *disconnected); /* * A referral entry (NVME_NQN_DISC) is a branch point: it opens up another @@ -2875,6 +2924,7 @@ static void dc_walk_referral(struct libnvme_global_ctx *ctx, __cleanup_libnvmf_tid struct libnvmf_tid *tid = NULL; struct dc_decision d = { 0 }; enum dc_ownership child_own; + bool child_disconnected = false; libnvme_ctrl_t cl; int err; @@ -2902,8 +2952,8 @@ static void dc_walk_referral(struct libnvme_global_ctx *ctx, return; } - cl = lookup_ctrl(h, params); - if (cl && libnvme_ctrl_get_name(cl)) { + cl = lookup_live_ctrl(h, params); + if (cl) { d.c = cl; d.already_connected = true; child_own = DC_BORROWED; @@ -2933,12 +2983,15 @@ static void dc_walk_referral(struct libnvme_global_ctx *ctx, /* * The child decides its own fate (disconnect or persist) * internally, the same way this DC just did above for - * itself -- so the free below is unconditional, matching - * how libnvmf_discover() already treats its own primary c. + * itself. Only free it if it actually disconnected -- + * a persisted child must stay in the tree, since a sibling + * or later referral elsewhere in this same walk may still + * need to find it via lookup_live_ctrl()/dc_visited_has(). */ _nvmf_discover(ctx, fctx, params, d.c, child_own, depth + 1, - visited, &eflags); - libnvme_free_ctrl(d.c); + visited, &eflags, &child_disconnected); + if (child_disconnected) + libnvme_free_ctrl(d.c); } /* @@ -2975,14 +3028,18 @@ static bool dc_survey_self_entry(struct libnvmf_context *fctx, d.disconnect = dc_decide(fctx, libnvme_ctrl_get_subsysnqn(c), own, self_seen, self_eflags, parent_eflags); - if (own == DC_BORROWED) - dc_log_decision(fctx, self_entry, &d, - "pre-existing, not ours to disconnect"); - else if (self_entry) + if (self_entry) { dc_log_decision(fctx, self_entry, &d, NULL); - else + } else if (own == DC_BORROWED) { + dc_log_decision(fctx, NULL, &d, + "pre-existing, not ours to disconnect"); + } else if (parent_eflags) { + dc_log_decision(fctx, NULL, &d, + "no self entry, using parent's view"); + } else { dc_log_decision(fctx, NULL, &d, "no self entry, EPCSD assumed 0"); + } return d.disconnect; } @@ -2991,7 +3048,8 @@ static int _nvmf_discover(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, const struct libnvme_ctrl_params *ctrl_params, struct libnvme_ctrl *c, enum dc_ownership own, int depth, - struct dc_visited *visited, const __u16 *parent_eflags) + struct dc_visited *visited, const __u16 *parent_eflags, + bool *disconnected) { __cleanup_free struct nvmf_discovery_log *log = NULL; libnvme_subsystem_t s = libnvme_ctrl_get_subsystem(c); @@ -3000,6 +3058,9 @@ static int _nvmf_discover(struct libnvme_global_ctx *ctx, bool disconnect; int err; + if (disconnected) + *disconnected = false; + struct libnvmf_discovery_args args = { .max_retries = fctx->default_max_discovery_retries, .lsp = NVMF_LOG_DISC_LSP_NONE, @@ -3019,14 +3080,26 @@ static int _nvmf_discover(struct libnvme_global_ctx *ctx, fctx->hooks.discovery_log(fctx, log, numrec, fctx->hooks.user_data); + dc_visited_register(visited, c, fctx); + /* * Pass 1: survey c's own self entry, and determine if safe to * disconnect. */ disconnect = dc_survey_self_entry(fctx, c, own, !parent_eflags, parent_eflags, log, numrec); - if (disconnect) + + /* + * Safe to disconnect now, before walking c's own referrals: this + * connection is only ever needed to fetch c's own DLP, and + * dc_visited already recorded c, independent of whether the + * connection stays open. + */ + if (disconnect) { libnvmf_disconnect_ctrl(c); + if (disconnected) + *disconnected = true; + } /* * Pass 2: everything else -- referrals to walk, NVM subsystem @@ -3082,7 +3155,11 @@ static int dc_walk(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, int err; err = _nvmf_discover(ctx, fctx, &fctx->ctrl_params, c, own, 0, - &visited, NULL); + &visited, NULL, NULL); + + /* dc_visited owns only the backing array, not the TIDs in it. */ + for (size_t i = 0; i < visited.len; i++) + libnvmf_tid_free(visited.items[i]); dc_visited_free(&visited); return err; @@ -3217,6 +3294,137 @@ static int nvmf_create_discovery_ctrl(struct libnvme_global_ctx *ctx, return 0; } +static struct libnvme_ctrl *dc_open_by_device(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, + struct libnvme_ctrl_params *params, enum dc_ownership *own) +{ + struct libnvme_ctrl *c; + int err; + + err = libnvme_scan_ctrl(ctx, fctx->device, &c); + if (err) { + /* No controller found, fall back to creating one. */ + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "ctrl device %s not found\n", fctx->device); + return NULL; + } + + /* Check if device matches command-line options */ + if (!libnvmf_ctrl_match_config(c, params)) { + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "ctrl device %s found, ignoring non matching command-line options\n", + fctx->device); + } + + if (!libnvme_ctrl_get_discovery_ctrl(c)) { + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "ctrl device %s found, ignoring non discovery controller\n", + fctx->device); + + libnvme_free_ctrl(c); + return NULL; + } + + /* + * The controller device was found, so this is an already-existing + * connection: it must not be disconnected on exit. Record that fact + * locally instead of overriding fctx->persistent, which must keep + * reflecting what the user actually asked for. + */ + *own = DC_BORROWED; + + /* + * When --host-traddr/--host-iface are not specified on the + * command line, use the discovery controller's (c) host- + * traddr/host-iface for the connections to controllers + * returned in the Discovery Log Pages. This is essential + * when invoking "connect-all" with --device to reuse an + * existing persistent discovery controller (as is done + * for the udev rules). This ensures that host-traddr/ + * host-iface are consistent with the discovery controller (c). + */ + if (!params->host_traddr) + params->host_traddr = (char *)libnvme_ctrl_get_host_traddr(c); + if (!params->host_iface) + params->host_iface = (char *)libnvme_ctrl_get_host_iface(c); + + return c; +} + +/* + * Ownership is reported separately (@own) rather than folded into + * fctx->persistent, which must keep reflecting what the user actually + * asked for regardless of whether this connection is reused or fresh. + * + * @honor_no_reuse: false makes this ignore --no-reuse and always attempt + * reuse first. NBFT's boot-time auto-connect has no + * concept of --no-reuse and always tries to reuse an + * existing connection first. + */ +static int dc_open(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, + struct libnvme_host *h, struct libnvme_ctrl_params *params, + bool honor_no_reuse, enum dc_ownership *own, + struct libnvme_ctrl **ctrl) +{ + struct libnvme_ctrl *c = NULL; + enum dc_ownership local_own = DC_OWNED; + int err; + + if (!honor_no_reuse || !fctx->no_reuse) { + if (fctx->device) + c = dc_open_by_device(ctx, fctx, params, &local_own); + + if (!c) { + c = lookup_ctrl(h, params); + if (c) + local_own = DC_BORROWED; + } + } + + if (c) { + if (!libnvme_ctrl_get_transport_handle(c)) { + /* + * When we found an existing controller it might not + * have a device handle yet + */ + err = libnvme_open(ctx, c->name, O_RDONLY, &c->hdl); + if (err) { + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "failed to open %s\n", c->name); + return err; + } + } + } else { + /* + * No existing controller, or --no-reuse was given: create a + * new one. + */ + err = nvmf_create_discovery_ctrl(ctx, fctx, params, h, &c); + + /* + * NBFT only: the OS's own DHCP client can obtain a different + * local address for this HFI than the firmware had when it + * built the NBFT table. Retry once without host_traddr. + * fctx->nbft_hfi is NULL for every non-NBFT caller, so this + * is a complete no-op for the general path. + */ + if (err == -ENVME_CONNECT_ADDRNOTAVAIL && fctx->nbft_hfi && + !strcmp(params->transport, "tcp") && + strlen(fctx->nbft_hfi->tcp_info.dhcp_server_ipaddr) > 0) { + params->host_traddr = NULL; + err = nvmf_create_discovery_ctrl(ctx, fctx, params, + h, &c); + } + + if (err) + return err; + } + + *own = local_own; + *ctrl = c; + return 0; +} + #define NBFT_SYSFS_FILENAME "NBFT*" static int nbft_filter(const struct dirent *dent) @@ -3309,7 +3517,7 @@ static bool validate_uri(struct libnvme_global_ctx *ctx, static int nbft_connect(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, struct libnvme_ctrl_params *params, struct libnvme_host *h, struct nvmf_disc_log_entry *e, - struct libnbft_subsystem_ns *ss) + struct libnbft_subsystem_ns *ss, libnvme_ctrl_t *cp) { libnvme_ctrl_t c; int saved_log_level; @@ -3317,12 +3525,18 @@ static int nbft_connect(struct libnvme_global_ctx *ctx, bool saved_log_pid; int ret; + if (cp) + *cp = NULL; + saved_log_level = libnvme_get_logging_level(ctx, &saved_log_pid, &saved_log_tstamp); - c = lookup_ctrl(h, params); - if (c && libnvme_ctrl_get_name(c)) + c = lookup_live_ctrl(h, params); + if (c) { + if (cp) + *cp = c; return 0; + } if (nvmf_excluded(ctx, params->transport, params->traddr, params->trsvcid, @@ -3371,101 +3585,63 @@ static int nbft_connect(struct libnvme_global_ctx *ctx, if (fctx->hooks.connected) fctx->hooks.connected(fctx, c, fctx->hooks.user_data); + if (cp) + *cp = c; return 0; } -static int nbft_discovery(struct libnvme_global_ctx *ctx, - struct libnvmf_context *fctx, - const struct libnvme_ctrl_params *ctrl_params, - struct libnbft_discovery *dd, - struct libnvme_host *h, struct libnvme_ctrl *c) +/* + * connect_leaf hook for the NBFT-driven walk: nbft_connect() plus the + * checks _nvmf_discover()'s general leaf-connect path gets for free from + * distinguishable lookup_ctrl()/nvmf_excluded() outcomes. nbft_connect() + * has its own copies of both checks internally, but folds every "nothing + * to do" outcome into a plain 0 -- fine for its other caller (the SSNS + * loop below), but not enough to fit dc_log_decision()'s + * connected/already-connected/reason contract. Re-checking here, before + * calling nbft_connect(), keeps nbft_connect() itself untouched for that + * other caller. + */ +static int nbft_connect_leaf(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, struct libnvme_host *h, + struct nvmf_disc_log_entry *e, + struct libnvme_ctrl_params *params, libnvme_ctrl_t *cp) { - struct nvmf_discovery_log *log = NULL; + libnvme_ctrl_t c; int ret; - int i; - struct libnvmf_discovery_args args = { - .max_retries = 10 /* MAX_DISC_RETRIES */, - .lsp = NVMF_LOG_DISC_LSP_NONE, - }; + *cp = NULL; - ret = nvme_discovery_log(c, &args, &log); - if (ret) { - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "Discovery Descriptor %d: failed to get discovery log: %s\n", - dd->index, libnvme_strerror(-ret)); - return ret; + c = lookup_live_ctrl(h, params); + if (c) { + *cp = c; + return -ENVME_CONNECT_ALREADY; } - for (i = 0; i < le64_to_cpu(log->numrec); i++) { - struct nvmf_disc_log_entry *e = &log->entries[i]; - __cleanup_ctrl_params struct libnvme_ctrl_params params = - ctrl_params_dup(ctrl_params); - libnvme_ctrl_t cl; - - sanitize_discovery_log_entry(c->ctx, e); - - params.subsysnqn = e->subnqn; - params.transport = libnvmf_trtype_str(e->trtype); - params.traddr = e->traddr; - params.trsvcid = e->trsvcid; - - if (e->subtype == NVME_NQN_CURR) - continue; - - /* Already connected ? */ - cl = lookup_ctrl(h, ¶ms); - if (cl && libnvme_ctrl_get_name(cl)) - continue; - - /* Skip connect if the transport types don't match */ - if (strcmp(libnvme_ctrl_get_transport(c), - params.transport)) - continue; - - if (e->subtype == NVME_NQN_DISC) { - libnvme_ctrl_t child; - - ret = nvmf_connect_disc_entry(h, e, ¶ms, &child); - if (ret) - continue; - nbft_discovery(ctx, fctx, ¶ms, dd, h, child); - libnvmf_disconnect_ctrl(child); - libnvme_free_ctrl(child); - } else { - ret = nbft_connect(ctx, fctx, ¶ms, h, e, NULL); - - /* - * With TCP/DHCP, it can happen that the OS - * obtains a different local IP address than the - * firmware had. Retry without host_traddr. - */ - if (ret == -ENVME_CONNECT_ADDRNOTAVAIL && - !strcmp(params.transport, "tcp") && - strlen(dd->hfi->tcp_info.dhcp_server_ipaddr) > 0) { - const char *htradr = params.host_traddr; - - params.host_traddr = NULL; - ret = nbft_connect(ctx, fctx, ¶ms, h, e, NULL); + if (nvmf_excluded(ctx, params->transport, params->traddr, + params->trsvcid, params->subsysnqn, + params->host_traddr, params->host_iface, + libnvme_host_get_hostnqn(h), + libnvme_host_get_hostid(h))) + return -EPERM; - if (ret == 0) - libnvme_msg(ctx, LIBNVME_LOG_INFO, - "Discovery Descriptor %d: connect with host_traddr=\"%s\" failed, success after omitting host_traddr\n", - dd->index, - htradr); - } + ret = nbft_connect(ctx, fctx, params, h, e, NULL, &c); - if (ret) - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "Discovery Descriptor %d: no controller found\n", - dd->index); - if (ret == -ENOMEM) - break; - } + /* + * With TCP/DHCP, the OS's own DHCP client can obtain a different + * local address for this HFI than the firmware had. Retry once + * without host_traddr. + */ + if (ret == -ENVME_CONNECT_ADDRNOTAVAIL && + !strcmp(params->transport, "tcp") && fctx->nbft_hfi && + strlen(fctx->nbft_hfi->tcp_info.dhcp_server_ipaddr) > 0) { + params->host_traddr = NULL; + ret = nbft_connect(ctx, fctx, params, h, e, NULL, &c); } - libnvme_free(log); - return 0; + if (!ret) + *cp = c; + + return ret; } #define VLAN_PROC_PATH "/proc/net/vlan" @@ -3643,7 +3819,8 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, "SSNS %d: could not find host interface for HFI %d\n", (*ss)->index, hfi->index); - rr = nbft_connect(ctx, fctx, ¶ms, h, NULL, *ss); + rr = nbft_connect(ctx, fctx, ¶ms, h, NULL, + *ss, NULL); /* * With TCP/DHCP, it can happen that the OS @@ -3657,7 +3834,7 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, params.host_traddr = NULL; rr = nbft_connect(ctx, fctx, ¶ms, h, - NULL, *ss); + NULL, *ss, NULL); if (rr == 0) libnvme_msg(ctx, LIBNVME_LOG_INFO, @@ -3684,7 +3861,7 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, __cleanup_free char *trsvcid = NULL; __cleanup_ctrl_params struct libnvme_ctrl_params params = ctrl_params_copy(&fctx->ctrl_params); - bool persistent = false; + enum dc_ownership own; bool linked = false; libnvme_ctrl_t c; @@ -3753,24 +3930,21 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, "Discovery Descriptor %d: could not find host interface for HFI %d\n", (*dd)->index, hfi->index); - /* Lookup existing discovery controller */ - c = lookup_ctrl(h, ¶ms); - if (c && libnvme_ctrl_get_name(c)) - persistent = true; - - if (!c) { - ret = nvmf_create_discovery_ctrl(ctx, fctx, - ¶ms, h, &c); - if (ret == -ENVME_CONNECT_ADDRNOTAVAIL && - !strcmp(params.transport, "tcp") && - strlen(hfi->tcp_info.dhcp_server_ipaddr) > 0) { - params.traddr = NULL; - ret = nvmf_create_discovery_ctrl(ctx, - fctx, ¶ms, h, &c); - } - } else - ret = 0; + /* + * NBFT boot discovery is a one-shot operation: never + * honor --no-reuse (dc_open()'s honor_no_reuse=false + * below) and never keep a freshly created connection + * alive past this walk (--persistent has no meaning + * here either). nbft_hfi and connect_leaf give + * dc_open() and the shared walker's leaf-connect step + * access to this DC's own HFI/leaf-connect quirks, + * several stack frames removed from this loop. + */ + fctx->nbft_hfi = hfi; + fctx->hooks.connect_leaf = nbft_connect_leaf; + fctx->persistent = LIBNVMF_PERSISTENT_NO; + ret = dc_open(ctx, fctx, h, ¶ms, false, &own, &c); if (ret) { libnvme_msg(ctx, LIBNVME_LOG_ERR, "Discovery Descriptor %d: failed to add discovery controller: %s\n", @@ -3778,9 +3952,7 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, goto out_free; } - rr = nbft_discovery(ctx, fctx, ¶ms, *dd, h, c); - if (!persistent) - libnvmf_disconnect_ctrl(c); + rr = dc_walk(ctx, fctx, c, own); libnvme_free_ctrl(c); if (rr == -ENOMEM) { ret = rr; @@ -3793,124 +3965,6 @@ __shr_public int libnvmf_discover_nbft(struct libnvme_global_ctx *ctx, return ret; } -static struct libnvme_ctrl *dc_open_by_device(struct libnvme_global_ctx *ctx, - struct libnvmf_context *fctx, enum dc_ownership *own) -{ - struct libnvme_ctrl *c; - int err; - - err = libnvme_scan_ctrl(ctx, fctx->device, &c); - if (err) { - /* No controller found, fall back to creating one. */ - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "ctrl device %s not found\n", fctx->device); - return NULL; - } - - /* Check if device matches command-line options */ - if (!libnvmf_ctrl_match_config(c, &fctx->ctrl_params)) { - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "ctrl device %s found, ignoring non matching command-line options\n", - fctx->device); - } - - if (!libnvme_ctrl_get_discovery_ctrl(c)) { - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "ctrl device %s found, ignoring non discovery controller\n", - fctx->device); - - libnvme_free_ctrl(c); - return NULL; - } - - /* - * The controller device was found, so this is an already-existing - * connection: it must not be disconnected on exit. Record that fact - * locally instead of overriding fctx->persistent, which must keep - * reflecting what the user actually asked for. - */ - *own = DC_BORROWED; - - /* - * When --host-traddr/--host-iface are not specified on the - * command line, use the discovery controller's (c) host- - * traddr/host-iface for the connections to controllers - * returned in the Discovery Log Pages. This is essential - * when invoking "connect-all" with --device to reuse an - * existing persistent discovery controller (as is done - * for the udev rules). This ensures that host-traddr/ - * host-iface are consistent with the discovery controller (c). - */ - if (!fctx->ctrl_params.host_traddr) - fctx->ctrl_params.host_traddr = - (char *)libnvme_ctrl_get_host_traddr(c); - if (!fctx->ctrl_params.host_iface) - fctx->ctrl_params.host_iface = - (char *)libnvme_ctrl_get_host_iface(c); - - return c; -} - -/* - * Resolve the primary discovery controller connection: reuse one via - * --device if given, reuse one found by matching connection parameters - * otherwise, or create a fresh one if --no-reuse was given or neither - * lookup found anything. Never touches fctx->persistent; ownership is - * reported separately so the caller knows what it may disconnect later. - */ -static int dc_open(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx, - struct libnvme_host *h, enum dc_ownership *own, - struct libnvme_ctrl **ctrl) -{ - struct libnvme_ctrl *c = NULL; - int err; - - *own = DC_OWNED; - - if (!fctx->no_reuse) { - if (fctx->device) - c = dc_open_by_device(ctx, fctx, own); - - if (!c) { - c = lookup_ctrl(h, &fctx->ctrl_params); - if (c) - *own = DC_BORROWED; - } - } - - if (c) { - if (!libnvme_ctrl_get_transport_handle(c)) { - /* - * When we found an existing controller it might not - * have a device handle yet - */ - err = libnvme_open(ctx, c->name, O_RDONLY, &c->hdl); - if (err) { - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "failed to open %s\n", c->name); - return err; - } - } - } else { - /* - * No existing controller, or --no-reuse was given: create a - * new one. - */ - err = nvmf_create_discovery_ctrl(ctx, fctx, &fctx->ctrl_params, - h, &c); - if (err) { - if (err != -ENVME_CONNECT_IGNORED) - libnvme_msg(ctx, LIBNVME_LOG_ERR, - "failed to add controller, error %s\n", - libnvme_strerror(-err)); - return err; - } - } - - *ctrl = c; - return 0; -} - __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, struct libnvmf_context *fctx) { @@ -3927,9 +3981,14 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx, if (err) return err; - err = dc_open(ctx, fctx, h, &own, &c); - if (err) + err = dc_open(ctx, fctx, h, &fctx->ctrl_params, true, &own, &c); + if (err) { + if (err != -ENVME_CONNECT_IGNORED) + libnvme_msg(ctx, LIBNVME_LOG_ERR, + "failed to add controller, error %s\n", + libnvme_strerror(-err)); return err; + } err = dc_walk(ctx, fctx, c, own); libnvme_free_ctrl(c); @@ -3960,9 +4019,8 @@ __shr_public int libnvmf_connect( if (err) return err; - c = lookup_ctrl(h, &fctx->ctrl_params); - if (c && libnvme_ctrl_get_name(c) && - !fctx->ctrl_params.cfg.duplicate_connect) { + c = lookup_live_ctrl(h, &fctx->ctrl_params); + if (c && !fctx->ctrl_params.cfg.duplicate_connect) { int instance = ctrl_instance(c); write_devid_file(fctx, devid_fd, c); diff --git a/libnvme/src/nvme/private-fabrics.h b/libnvme/src/nvme/private-fabrics.h index 530ff5c14b..5029dae25a 100644 --- a/libnvme/src/nvme/private-fabrics.h +++ b/libnvme/src/nvme/private-fabrics.h @@ -54,6 +54,17 @@ struct libnvmf_hooks { struct nvmf_discovery_log *log, uint64_t numrec, void *user_data); + /* + * NBFT-only: internal seam between the shared discovery walker and + * NBFT's leaf-connect quirks (DHCP retry, firing hooks.connected). + * Set only by libnvmf_discover_nbft() itself; never exposed to a + * public setter. + */ + int (*connect_leaf)(struct libnvme_global_ctx *ctx, + struct libnvmf_context *fctx, struct libnvme_host *h, + struct nvmf_disc_log_entry *e, + struct libnvme_ctrl_params *params, libnvme_ctrl_t *cp); + void *user_data; }; @@ -77,6 +88,7 @@ struct libnvmf_context { // !generate-accessors:read=generated,write=generated bool connect; // !access bool no_reuse; // !access char *nbft_path; // !access + const struct libnbft_hfi *nbft_hfi; // !access:read=none,write=none /* host configuration */ char *hostnqn; // !access:write=custom