nvme: redesign --persistent, remove --epcsd/--no-epcsd

Documentation/nvme-config-create.txt

@@ -10,10 +10,7 @@ SYNOPSIS
 [verse]
 'nvme' [<global-options>] 'config create'
 			[--discovery]
-			[--persistent ]
-			[--no-persistent ]
-			[--epcsd ]
-			[--no-epcsd ]
+			[--persistent[=<no|auto|force>] ]
 			[--host-symname=<name>]
 			[--output=<file>]
 			[<fabrics-options>]
@@ -43,27 +40,23 @@ OPTIONS
 	Create a discovery controller entry instead of an I/O controller
 	entry. Without this, --nqn is required.
 
---persistent::
-	Keep the discovery controller connected to receive Asynchronous
-	Event Notifications instead of disconnecting after the discovery
-	log page fetch. Requires --discovery. Mutually exclusive with
-	--no-persistent.
-
---no-persistent::
-	Explicitly record that this discovery controller is not
-	persistent, overriding any default that would otherwise apply
-	(e.g. one derived from the discovery log page). Requires
-	--discovery. Mutually exclusive with --persistent.
-
---epcsd::
-	Mark this discovery controller as supporting Explicit Persistent
-	Connection Support for Discovery (EPCSD). Requires --discovery.
-	Mutually exclusive with --no-epcsd.
-
---no-epcsd::
-	Explicitly record that this discovery controller does not support
-	EPCSD, overriding any default that would otherwise apply. Requires
-	--discovery. Mutually exclusive with --epcsd.
+--persistent[=<no|auto|force>]::
+	Record this discovery controller's persistence mode. Requires
+	--discovery. Because the value is optional, it must be attached
+	directly to the option (--persistent=force). A value given as a
+	separate argument (--persistent force) is silently NOT applied
+	-- it's dropped, and --persistent is treated as if given bare
+	(mode "auto"), with no error.
++
+--
+* 'auto' (the default when --persistent is given with no value): persist
+  the connection only where the target's own EPCSD (Explicit Persistent
+  Connection Support for Discovery) flag reports support for it.
+* 'force': persist regardless of what EPCSD reports, for a target whose
+  self-reported EPCSD cannot be trusted.
+* 'no': explicitly record that this discovery controller is not
+  persistent, overriding any default that would otherwise apply.
+--
 
 --host-symname=<name>::
 	Name this host persona. Puts it in its own configuration drop-in.

Documentation/nvme-connect-all.txt

@@ -12,7 +12,7 @@ SYNOPSIS
 			[--raw=<filename> | -r <filename>]
 			[--device=<device> | -d <device>]
 			[--config=<filename> | -J <cfg>]
-			[--persistent | -p]
+			[--persistent[=<no|auto|force>] | -p]
 			[--quiet]
 			[--nbft]
 			[--no-nbft]
@@ -62,9 +62,26 @@ OPTIONS
 	https://github.com/linux-nvme/nvme-cli/blob/master/libnvme/doc/config-schema.json
 
 -p::
---persistent::
-	Don't remove the discovery controller after retrieving the discovery
-	log page.
+--persistent[=<no|auto|force>]::
+	Keep the discovery controller connected after retrieving the
+	discovery log page, instead of disconnecting once it's done.
+	Because the value is optional, it must be attached directly to
+	the option: --persistent=force for the long form, -pforce for
+	the short form. A value given as a separate argument (--persistent
+	force, -p force) is silently NOT applied -- it's dropped, and
+	--persistent/-p is treated as if given bare (mode "auto"), with
+	no error.
++
+--
+* 'no' (the default when the option is omitted entirely): never persist.
+* 'auto' (the default when --persistent is given with no value): persist
+  a Discovery Log Page entry's connection only where that entry's own
+  EPCSD (Explicit Persistent Connection Support for Discovery) flag
+  reports support for it; degrade to non-persistent, with a log message,
+  for entries that don't.
+* 'force': persist regardless of what EPCSD reports, for a target whose
+  self-reported EPCSD cannot be trusted.
+--
 
 --quiet::
 	Suppress error messages.

Documentation/nvme-discover.txt

@@ -12,7 +12,7 @@ SYNOPSIS
 			[--raw=<filename> | -r <filename>]
 			[--device=<device> | -d <device>]
 			[--config=<filename> | -J <filename>]
-			[--persistent | -p]
+			[--persistent[=<no|auto|force>] | -p]
 			[--quiet]
 			[--force]
 			[--nbft]
@@ -83,9 +83,26 @@ OPTIONS
 	https://github.com/linux-nvme/nvme-cli/blob/master/libnvme/doc/config-schema.json
 
 -p::
---persistent::
-	Don't remove the discovery controller after retrieving the discovery
-	log page.
+--persistent[=<no|auto|force>]::
+	Keep the discovery controller connected after retrieving the
+	discovery log page, instead of disconnecting once it's done.
+	Because the value is optional, it must be attached directly to
+	the option: --persistent=force for the long form, -pforce for
+	the short form. A value given as a separate argument (--persistent
+	force, -p force) is silently NOT applied -- it's dropped, and
+	--persistent/-p is treated as if given bare (mode "auto"), with
+	no error.
++
+--
+* 'no' (the default when the option is omitted entirely): never persist.
+* 'auto' (the default when --persistent is given with no value): persist
+  a Discovery Log Page entry's connection only where that entry's own
+  EPCSD (Explicit Persistent Connection Support for Discovery) flag
+  reports support for it; degrade to non-persistent, with a log message,
+  for entries that don't.
+* 'force': persist regardless of what EPCSD reports, for a target whose
+  self-reported EPCSD cannot be trusted.
+--
 
 --quiet::
 	Suppress already connected errors.

libnvme/libnvme3/nvme.i

@@ -181,6 +181,7 @@ static int set_fctx_host_params(struct libnvme_global_ctx *ctx,
 	const char *hostnqn = NULL, *hostid = NULL;
 	const char *hostkey = NULL, *ctrlkey = NULL;
 	const char *keyring = NULL, *tls_key = NULL, *tls_key_identity = NULL;
+	const char *persistent = NULL;
 	int err;
 	str_fields_t tbl[] = {
 		{"hostnqn",          &hostnqn},
@@ -190,19 +191,21 @@ static int set_fctx_host_params(struct libnvme_global_ctx *ctx,
 		{"keyring",          &keyring},
 		{"tls_key",          &tls_key},
 		{"tls_key_identity", &tls_key_identity},
+		{"persistent",       &persistent},
 		{NULL, NULL}, /* sentinel */
 	};
 	str_fields_t *p;
-	PyObject *val;
-
-	val = PyDict_GetItemString(dict, "persistent");
-	if (val)
-		fctx->persistent = PyObject_IsTrue(val) ?
-			LIBNVMF_TRISTATE_TRUE : LIBNVMF_TRISTATE_FALSE;
 
 	for (p = tbl; p->key; p++)
 		*p->val = dict_get_str(dict, p->key);
 
+	if (persistent && libnvmf_context_set_persistent(fctx, persistent)) {
+		PyErr_Format(PyExc_ValueError,
+			"invalid 'persistent' value '%s' (expected 'no', 'auto', or 'force')",
+			persistent);
+		return -1;
+	}
+
 	/* Fall back to the ctx default only when the dict supplies neither
 	 * field. Never splice a dict-given hostnqn with a ctx-default
 	 * hostid (or vice versa) -- that pairing was never validated
@@ -1687,7 +1690,10 @@ struct libnvme_ns *libnvme_ctrl_next_ns(struct libnvme_ctrl *c, struct libnvme_n
 		"      tls_key_identity (str)  -- TLS key identity string\n"
 		"\n"
 		"    Persistence (optional):\n"
-		"      persistent (bool)  -- Keep connection alive after process exit\n"
+		"      persistent (str)  -- 'no', 'auto', or 'force': keep the\n"
+		"                           connection alive after process exit;\n"
+		"                           'auto' does so only where the target's\n"
+		"                           own EPCSD flag supports it\n"
 		"\n"
 		"  Examples::\n"
 		"\n"

libnvme/src/accessors-fabrics.ld

@@ -28,7 +28,6 @@ LIBNVMF_ACCESSORS_3 {
 		libnvmf_context_get_devid_file;
 		libnvmf_context_get_disable_sqflow;
 		libnvmf_context_get_duplicate_connect;
-		libnvmf_context_get_epcsd;
 		libnvmf_context_get_fast_io_fail_tmo;
 		libnvmf_context_get_force;
 		libnvmf_context_get_hdr_digest;
@@ -44,7 +43,6 @@ LIBNVMF_ACCESSORS_3 {
 		libnvmf_context_get_nr_io_queues;
 		libnvmf_context_get_nr_poll_queues;
 		libnvmf_context_get_nr_write_queues;
-		libnvmf_context_get_persistent;
 		libnvmf_context_get_queue_size;
 		libnvmf_context_get_reconnect_delay;
 		libnvmf_context_get_subsysnqn;
@@ -65,7 +63,6 @@ LIBNVMF_ACCESSORS_3 {
 		libnvmf_context_set_default_max_discovery_retries;
 		libnvmf_context_set_disable_sqflow;
 		libnvmf_context_set_duplicate_connect;
-		libnvmf_context_set_epcsd;
 		libnvmf_context_set_fast_io_fail_tmo;
 		libnvmf_context_set_force;
 		libnvmf_context_set_hdr_digest;
@@ -75,7 +72,6 @@ LIBNVMF_ACCESSORS_3 {
 		libnvmf_context_set_nr_io_queues;
 		libnvmf_context_set_nr_poll_queues;
 		libnvmf_context_set_nr_write_queues;
-		libnvmf_context_set_persistent;
 		libnvmf_context_set_queue_size;
 		libnvmf_context_set_reconnect_delay;
 		libnvmf_context_set_tls;

libnvme/src/libnvmf.ld

@@ -32,6 +32,7 @@ LIBNVMF_3 {
 		libnvmf_context_apply_params;
 		libnvmf_context_create;
 		libnvmf_context_free;
+		libnvmf_context_get_persistent;
 		libnvmf_context_set_connection;
 		libnvmf_context_set_connection_from_tid;
 		libnvmf_context_set_crypto;
@@ -40,6 +41,7 @@ LIBNVMF_3 {
 		libnvmf_context_set_discovery_hooks;
 		libnvmf_context_set_hostnqn;
 		libnvmf_context_set_io_queues;
+		libnvmf_context_set_persistent;
 		libnvmf_context_set_reconnect_policy;
 		libnvmf_create_ctrl;
 		libnvmf_create_raw_secret;

libnvme/src/nvme/accessors-fabrics.c

@@ -325,32 +325,6 @@ __shr_public const char *libnvmf_context_get_device(
 	return p->device;
 }
 
-__shr_public void libnvmf_context_set_persistent(
-		struct libnvmf_context *p,
-		enum libnvmf_tristate persistent)
-{
-	p->persistent = persistent;
-}
-
-__shr_public enum libnvmf_tristate libnvmf_context_get_persistent(
-		const struct libnvmf_context *p)
-{
-	return p->persistent;
-}
-
-__shr_public void libnvmf_context_set_epcsd(
-		struct libnvmf_context *p,
-		enum libnvmf_tristate epcsd)
-{
-	p->epcsd = epcsd;
-}
-
-__shr_public enum libnvmf_tristate libnvmf_context_get_epcsd(
-		const struct libnvmf_context *p)
-{
-	return p->epcsd;
-}
-
 __shr_public const char *libnvmf_context_get_devid_file(
 		const struct libnvmf_context *p)
 {

libnvme/src/nvme/accessors-fabrics.h

@@ -422,42 +422,6 @@ int libnvmf_context_get_default_keep_alive_timeout(
  */
 const char *libnvmf_context_get_device(const struct libnvmf_context *p);
 
-/**
- * libnvmf_context_set_persistent() - Set persistent.
- * @p: The &struct libnvmf_context instance to update.
- * @persistent: Value to assign to the persistent field.
- */
-void libnvmf_context_set_persistent(
-		struct libnvmf_context *p,
-		enum libnvmf_tristate persistent);
-
-/**
- * libnvmf_context_get_persistent() - Get persistent.
- * @p: The &struct libnvmf_context instance to query.
- *
- * Return: The value of the persistent field.
- */
-enum libnvmf_tristate libnvmf_context_get_persistent(
-		const struct libnvmf_context *p);
-
-/**
- * libnvmf_context_set_epcsd() - Set epcsd.
- * @p: The &struct libnvmf_context instance to update.
- * @epcsd: Value to assign to the epcsd field.
- */
-void libnvmf_context_set_epcsd(
-		struct libnvmf_context *p,
-		enum libnvmf_tristate epcsd);
-
-/**
- * libnvmf_context_get_epcsd() - Get epcsd.
- * @p: The &struct libnvmf_context instance to query.
- *
- * Return: The value of the epcsd field.
- */
-enum libnvmf_tristate libnvmf_context_get_epcsd(
-		const struct libnvmf_context *p);
-
 /**
  * libnvmf_context_get_devid_file() - Get devid_file.
  * @p: The &struct libnvmf_context instance to query.

libnvme/src/nvme/config-ini.c

@@ -29,6 +29,8 @@
 #include "private-fabrics.h"
 #include "util.h"
 
+static int check_persistent(const char *value);
+
 static const struct libnvmf_key keys[] = {
 	/* connection tunables; only class overridable per controller= line */
 	{ "nr-io-queues",		LIBNVMF_KEY_INT,	LIBNVMF_KEY_TUNABLE },
@@ -46,8 +48,8 @@ static const struct libnvmf_key keys[] = {
 	{ "data-digest",		LIBNVMF_KEY_BOOL,	LIBNVMF_KEY_TUNABLE },
 
 	/* discovery controller  */
-	{ "persistent",			LIBNVMF_KEY_BOOL,	LIBNVMF_KEY_DC_TUNABLE },
-	{ "epcsd",			LIBNVMF_KEY_BOOL,	LIBNVMF_KEY_DC_TUNABLE },
+	{ "persistent", LIBNVMF_KEY_STRENUM, LIBNVMF_KEY_DC_TUNABLE,
+	  check_persistent },
 
 	/* security -- bound to (hostnqn, subsysnqn); never per-path */
 	{ "tls",			LIBNVMF_KEY_BOOL,	LIBNVMF_KEY_SECURITY },
@@ -95,6 +97,13 @@ static int check_int(const char *value)
 	return 0;
 }
 
+static int check_persistent(const char *value)
+{
+	enum libnvmf_persistent p;
+
+	return _libnvmf_persistent_from_str(value, &p);
+}
+
 int libnvmf_key_check_value(const struct libnvmf_key *key, const char *value)
 {
 	bool b;
@@ -115,6 +124,8 @@ int libnvmf_key_check_value(const struct libnvmf_key *key, const char *value)
 		return check_int(value);
 	case LIBNVMF_KEY_BOOL:
 		return shr_parse_bool(value, &b);
+	case LIBNVMF_KEY_STRENUM:
+		return key->strenum_validate(value);
 	case LIBNVMF_KEY_STRING:
 		return 0;
 	}

libnvme/src/nvme/config-ini.h

@@ -48,7 +48,11 @@ int libnvmf_params_merge(struct libnvmf_params *dst,
 enum libnvmf_key_type {
 	LIBNVMF_KEY_STRING,
 	LIBNVMF_KEY_INT,
-	LIBNVMF_KEY_BOOL
+	LIBNVMF_KEY_BOOL,
+	LIBNVMF_KEY_STRENUM,	/* string decoding to one of a fixed set of
+				 * values, e.g. "persistent" -> no/auto/force;
+				 * see struct libnvmf_key.validate
+				 */
 };
 
 /* See the keys[] table in config-ini.c for each key's class. */
@@ -65,6 +69,15 @@ struct libnvmf_key {
 	const char *name;
 	enum libnvmf_key_type type;
 	enum libnvmf_key_class class;
+	/*
+	 * Required for LIBNVMF_KEY_STRENUM, unused otherwise. Returns 0 if
+	 * @value is one of this key's accepted values, -EINVAL otherwise.
+	 * Add a new STRENUM key by writing a small check_<name>() function
+	 * (see check_persistent() in config-ini.c for the pattern) and
+	 * pointing this field at it -- the switch in
+	 * libnvmf_key_check_value() does not change.
+	 */
+	int (*strenum_validate)(const char *value);
 };
 
 /* Return the table entry for @name, or NULL for an unknown key. */

libnvme/src/nvme/config.c

@@ -311,13 +311,7 @@ static void apply_param(const char *key, const char *value, void *user_data)
 		if (!shr_parse_bool(value, &bval))
 			cfg->concat = bval;
 	} else if (!strcmp(key, "persistent")) {
-		if (!shr_parse_bool(value, &bval))
-			fctx->persistent = bval ? LIBNVMF_TRISTATE_TRUE :
-						  LIBNVMF_TRISTATE_FALSE;
-	} else if (!strcmp(key, "epcsd")) {
-		if (!shr_parse_bool(value, &bval))
-			fctx->epcsd = bval ? LIBNVMF_TRISTATE_TRUE :
-					     LIBNVMF_TRISTATE_FALSE;
+		libnvmf_context_set_persistent(fctx, value);
 	}
 	/* Identity/addressing and crypto keys never reach here -- only
 	 * tunable keys are handled in this loop. Crypto keys are read