libnvme: rewrite the discovery walk as a depth-capped, deduped walker

Documentation/nvme-connect-all.txt

@@ -14,6 +14,7 @@ SYNOPSIS
 			[--config=<filename> | -J <cfg>]
 			[--persistent[=<no|auto|force>] | -p]
 			[--quiet]
+			[--no-reuse]
 			[--nbft]
 			[--no-nbft]
 			[--nbft-path=<STR>]
@@ -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
 

Documentation/nvme-discover.txt

@@ -14,6 +14,7 @@ SYNOPSIS
 			[--config=<filename> | -J <filename>]
 			[--persistent[=<no|auto|force>] | -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

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;

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;
@@ -3568,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;
@@ -3619,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;
 	}
 
@@ -3655,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;
 	}
@@ -3667,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
@@ -3689,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;
 		}
 	}
@@ -3737,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);
@@ -3748,38 +3760,12 @@ __shr_public int libnvmf_discover(struct libnvme_global_ctx *ctx,
 	if (err)
 		return err;
 
-	if (!fctx->force) {
-		/*
-		 * When --force 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 --force 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;
@@ -3816,11 +3802,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 +3860,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;
 }

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(

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.

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);

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 */