diff --git a/Documentation/nvme-config-create.txt b/Documentation/nvme-config-create.txt index ebbc4fabb1..20663c38c0 100644 --- a/Documentation/nvme-config-create.txt +++ b/Documentation/nvme-config-create.txt @@ -10,10 +10,7 @@ SYNOPSIS [verse] 'nvme' [] 'config create' [--discovery] - [--persistent ] - [--no-persistent ] - [--epcsd ] - [--no-epcsd ] + [--persistent[=] ] [--host-symname=] [--output=] [] @@ -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[=]:: + 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 this host persona. Puts it in its own configuration drop-in. diff --git a/Documentation/nvme-connect-all.txt b/Documentation/nvme-connect-all.txt index 8d1cbe5e8e..743da99c06 100644 --- a/Documentation/nvme-connect-all.txt +++ b/Documentation/nvme-connect-all.txt @@ -12,7 +12,7 @@ SYNOPSIS [--raw= | -r ] [--device= | -d ] [--config= | -J ] - [--persistent | -p] + [--persistent[=] | -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[=]:: + 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. diff --git a/Documentation/nvme-discover.txt b/Documentation/nvme-discover.txt index 63861909cc..5068de7a41 100644 --- a/Documentation/nvme-discover.txt +++ b/Documentation/nvme-discover.txt @@ -12,7 +12,7 @@ SYNOPSIS [--raw= | -r ] [--device= | -d ] [--config= | -J ] - [--persistent | -p] + [--persistent[=] | -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[=]:: + 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. diff --git a/libnvme/libnvme3/nvme.i b/libnvme/libnvme3/nvme.i index e034805f50..301951b574 100644 --- a/libnvme/libnvme3/nvme.i +++ b/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" diff --git a/libnvme/src/accessors-fabrics.ld b/libnvme/src/accessors-fabrics.ld index 936e3c68b6..72d96838cf 100644 --- a/libnvme/src/accessors-fabrics.ld +++ b/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; diff --git a/libnvme/src/libnvmf.ld b/libnvme/src/libnvmf.ld index beb73b9172..f5c0a72e94 100644 --- a/libnvme/src/libnvmf.ld +++ b/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; diff --git a/libnvme/src/nvme/config-ini.c b/libnvme/src/nvme/config-ini.c index 2fcd8d4acc..b7557a6c6a 100644 --- a/libnvme/src/nvme/config-ini.c +++ b/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; } diff --git a/libnvme/src/nvme/config-ini.h b/libnvme/src/nvme/config-ini.h index b69ca53c05..5d5a0b3d05 100644 --- a/libnvme/src/nvme/config-ini.h +++ b/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_() 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. */ diff --git a/libnvme/src/nvme/config.c b/libnvme/src/nvme/config.c index d16323e370..bd8dcce0c9 100644 --- a/libnvme/src/nvme/config.c +++ b/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 diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index ba3cc2d1eb..58f27e41fb 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/libnvme/src/nvme/fabrics.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -410,6 +411,69 @@ __shr_public int libnvmf_host_get_ids(struct libnvme_global_ctx *ctx, return 0; } +int _libnvmf_persistent_from_str(const char *str, enum libnvmf_persistent *val) +{ + if (!str || !val) + return -EINVAL; + + if (!strcasecmp(str, "no")) + *val = LIBNVMF_PERSISTENT_NO; + else if (!strcasecmp(str, "auto")) + *val = LIBNVMF_PERSISTENT_AUTO; + else if (!strcasecmp(str, "force")) + *val = LIBNVMF_PERSISTENT_FORCE; + else + return -EINVAL; + + return 0; +} + +/** + * libnvmf_context_set_persistent() - Set the discovery controller + * persistence mode. + * @fctx: The &struct libnvmf_context instance to update. + * @persistent: One of "no", "auto", "force" (case-insensitive). + * + * Return: 0 on success, -EINVAL if @persistent matches none of the + * accepted values. + */ +__shr_public int libnvmf_context_set_persistent(struct libnvmf_context *fctx, + const char *persistent) +{ + if (!fctx) + return -EINVAL; + + return _libnvmf_persistent_from_str(persistent, &fctx->persistent); +} + +/** + * libnvmf_context_get_persistent() - Get the discovery controller + * persistence mode. + * @fctx: The &struct libnvmf_context instance to query. + * + * Return: "no", "auto", or "force" if explicitly configured; NULL if not + * (behaves the same as "no" when applied to a live connection, or if + * @fctx is NULL). + */ +__shr_public const char *libnvmf_context_get_persistent( + const struct libnvmf_context *fctx) +{ + if (!fctx) + return NULL; + + switch (fctx->persistent) { + case LIBNVMF_PERSISTENT_NO: + return "no"; + case LIBNVMF_PERSISTENT_AUTO: + return "auto"; + case LIBNVMF_PERSISTENT_FORCE: + return "force"; + case LIBNVMF_PERSISTENT_UNSET: + default: + return NULL; + } +} + const char * const trtypes[] = { [NVMF_TRTYPE_RDMA] = "rdma", [NVMF_TRTYPE_FC] = "fc", @@ -2548,15 +2612,20 @@ static int setup_connection(struct libnvmf_context *fctx, struct libnvme_host *h static int set_discovery_kato(struct libnvmf_context *fctx) { int tmo = fctx->ctrl_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() + * disconnects per entry afterward if EPCSD turns out to be unset. + */ + bool wants_kato = fctx->persistent == LIBNVMF_PERSISTENT_AUTO || + fctx->persistent == LIBNVMF_PERSISTENT_FORCE; /* Set kato to NVMF_DEF_DISC_TMO for persistent controllers */ - if (fctx->persistent == LIBNVMF_TRISTATE_TRUE && - !fctx->ctrl_params.cfg.keep_alive_tmo) + if (wants_kato && !fctx->ctrl_params.cfg.keep_alive_tmo) fctx->ctrl_params.cfg.keep_alive_tmo = fctx->default_keep_alive_timeout; /* Set kato to zero for non-persistent controllers */ - else if (fctx->persistent != LIBNVMF_TRISTATE_TRUE && - (fctx->ctrl_params.cfg.keep_alive_tmo > 0)) + else if (!wants_kato && fctx->ctrl_params.cfg.keep_alive_tmo > 0) fctx->ctrl_params.cfg.keep_alive_tmo = 0; return tmo; @@ -2607,19 +2676,23 @@ static bool dc_should_connect(struct libnvmf_context *fctx, if (eflags & NVMF_DISC_EFLAGS_DUPRETINFO) return false; - if (!strcmp(e->subnqn, NVME_DISC_SUBSYS_NAME)) { - /* - * Implicit persistent discovery controller. - * Keep it only if persistence is enabled. - */ - disconnect = fctx->persistent != LIBNVMF_TRISTATE_TRUE; - } else { - /* - * Explicit persistent discovery controller. - * Keep it only if EPCSD is supported and enabled. - */ - disconnect = !(eflags & NVMF_DISC_EFLAGS_EPCSD) || - fctx->epcsd == LIBNVMF_TRISTATE_FALSE; + switch (fctx->persistent) { + case LIBNVMF_PERSISTENT_FORCE: + /* Persist regardless of what EPCSD reports. */ + disconnect = false; + break; + case LIBNVMF_PERSISTENT_AUTO: + /* Persist only where the entry's own EPCSD flag says so. */ + disconnect = !(eflags & NVMF_DISC_EFLAGS_EPCSD); + if (disconnect) + libnvme_msg(fctx->ctx, LIBNVME_LOG_WARN, + "%s: not persisting, EPCSD=0\n", e->subnqn); + break; + case LIBNVMF_PERSISTENT_NO: + case LIBNVMF_PERSISTENT_UNSET: + default: + disconnect = true; + break; } *pdisconnect = disconnect; @@ -3431,10 +3504,11 @@ static struct libnvme_ctrl *discover_lookup_ctrl_by_device( */ libnvme_msg(ctx, LIBNVME_LOG_ERR, "ctrl device %s not found%s\n", fctx->device, - fctx->persistent == LIBNVMF_TRISTATE_TRUE ? + fctx->persistent == LIBNVMF_PERSISTENT_AUTO || + fctx->persistent == LIBNVMF_PERSISTENT_FORCE ? ", ignoring --persistent" : ""); - fctx->persistent = LIBNVMF_TRISTATE_FALSE; + fctx->persistent = LIBNVMF_PERSISTENT_NO; return NULL; } @@ -3451,7 +3525,7 @@ static struct libnvme_ctrl *discover_lookup_ctrl_by_device( "ctrl device %s found, ignoring non discovery controller\n", fctx->device); - fctx->persistent = LIBNVMF_TRISTATE_FALSE; + fctx->persistent = LIBNVMF_PERSISTENT_NO; libnvme_free_ctrl(c); return NULL; @@ -3459,9 +3533,11 @@ static struct libnvme_ctrl *discover_lookup_ctrl_by_device( /* * If the controller device is found it must be persistent, and - * shouldn't be disconnected on exit. + * shouldn't be disconnected on exit. This is an established fact + * about an already-existing connection, not a fresh EPCSD-driven + * decision, so force it regardless of what --persistent said. */ - fctx->persistent = LIBNVMF_TRISTATE_TRUE; + fctx->persistent = LIBNVMF_PERSISTENT_FORCE; /* * When --host-traddr/--host-iface are not specified on the @@ -3500,7 +3576,7 @@ static int discover_lookup_ctrl(struct libnvme_global_ctx *ctx, * Do not disconnect after use, because it was not * created by us. */ - fctx->persistent = LIBNVMF_TRISTATE_TRUE; + fctx->persistent = LIBNVMF_PERSISTENT_FORCE; } } @@ -3633,7 +3709,7 @@ __shr_public int libnvmf_connect( * this as a persistent connection and specify a KATO. */ if (!strcmp(fctx->ctrl_params.subsysnqn, NVME_DISC_SUBSYS_NAME)) { - fctx->persistent = LIBNVMF_TRISTATE_TRUE; + fctx->persistent = LIBNVMF_PERSISTENT_FORCE; set_discovery_kato(fctx); } diff --git a/libnvme/src/nvme/fabrics.h b/libnvme/src/nvme/fabrics.h index 5e28abc413..01345fa3cd 100644 --- a/libnvme/src/nvme/fabrics.h +++ b/libnvme/src/nvme/fabrics.h @@ -421,6 +421,34 @@ int libnvmf_context_set_connection(struct libnvmf_context *fctx, int libnvmf_context_set_hostnqn(struct libnvmf_context *fctx, const char *hostnqn, const char *hostid); +/** + * libnvmf_context_set_persistent() - Set the discovery controller + * persistence mode + * @fctx: Fabrics context + * @persistent: One of "no", "auto", "force" (case-insensitive) + * + * "auto" persists a Discovery Log Page entry's connection only where that + * entry's own EPCSD flag (see &enum nvmf_disc_eflags) reports support for + * it; "force" persists regardless of what EPCSD reports; "no" never + * persists. + * + * Return: 0 on success, -EINVAL if @fctx is NULL or @persistent matches + * none of the accepted values. + */ +int libnvmf_context_set_persistent(struct libnvmf_context *fctx, + const char *persistent); + +/** + * libnvmf_context_get_persistent() - Get the discovery controller + * persistence mode + * @fctx: Fabrics context + * + * Return: "no", "auto", or "force" if explicitly configured; NULL if not + * (behaves the same as "no" when applied to a live connection, or if + * @fctx is NULL). + */ +const char *libnvmf_context_get_persistent(const struct libnvmf_context *fctx); + /** * libnvmf_context_set_connection_from_tid() - Set connection and identity * from a TID diff --git a/libnvme/src/nvme/generated/accessors-fabrics.c b/libnvme/src/nvme/generated/accessors-fabrics.c index 36d8f16aca..437570f0ad 100644 --- a/libnvme/src/nvme/generated/accessors-fabrics.c +++ b/libnvme/src/nvme/generated/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) { diff --git a/libnvme/src/nvme/generated/accessors-fabrics.h b/libnvme/src/nvme/generated/accessors-fabrics.h index 4f326612c9..adf5833f46 100644 --- a/libnvme/src/nvme/generated/accessors-fabrics.h +++ b/libnvme/src/nvme/generated/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. diff --git a/libnvme/src/nvme/nvme-types-fabrics.h b/libnvme/src/nvme/nvme-types-fabrics.h index 4d569d8624..6b6334f5e0 100644 --- a/libnvme/src/nvme/nvme-types-fabrics.h +++ b/libnvme/src/nvme/nvme-types-fabrics.h @@ -105,27 +105,6 @@ enum nvmf_disc_eflags { NVMF_DISC_EFLAGS_NCC = 1 << 2, }; -/** - * enum libnvmf_tristate - Optional boolean discovery controller override. - * @LIBNVMF_TRISTATE_UNSET: Not explicitly configured by the user. The - * effective value should be derived from another - * source, e.g. the discovery log page entry flags - * (see &enum nvmf_disc_eflags). - * @LIBNVMF_TRISTATE_TRUE: Explicitly enabled by the user. - * @LIBNVMF_TRISTATE_FALSE: Explicitly disabled by the user, overriding any - * value that would otherwise be derived elsewhere. - * - * This is a libnvme-defined helper type, not an NVMe-oF wire type. It is - * used by discovery-controller-level configuration knobs (e.g. persistent, - * epcsd) that must distinguish "the user did not say" from an explicit - * false, so a default derived at runtime is not silently overridden. - */ -enum libnvmf_tristate { - LIBNVMF_TRISTATE_UNSET = 0, - LIBNVMF_TRISTATE_TRUE = 1, - LIBNVMF_TRISTATE_FALSE = 2, -}; - /** * union nvmf_tsas - Transport Specific Address Subtype * @common: Common transport specific attributes diff --git a/libnvme/src/nvme/private-fabrics.h b/libnvme/src/nvme/private-fabrics.h index 1f3bd44824..57fac13f55 100644 --- a/libnvme/src/nvme/private-fabrics.h +++ b/libnvme/src/nvme/private-fabrics.h @@ -16,6 +16,28 @@ #include "nvme/private.h" +/* + * Discovery controller persistence mode. Internal only -- both the + * getter and setter are hand-written and speak strings ("no", "auto", + * "force") at the public boundary, so this enum never appears in a + * public header. + * + * UNSET means not explicitly configured; behaves the same as NO when + * applied to a live connection. + * + * AUTO persists a DC connection only when that DC's DLP entry's own EPCSD + * reports support for it, degrading to non-persistent otherwise. + * + * FORCE persists regardless of what EPCSD reports, for a target whose + * self-reported EPCSD cannot be trusted. + */ +enum libnvmf_persistent { + LIBNVMF_PERSISTENT_UNSET = 0, + LIBNVMF_PERSISTENT_NO = 1, + LIBNVMF_PERSISTENT_AUTO = 2, + LIBNVMF_PERSISTENT_FORCE = 3, +}; + struct libnvmf_hooks { /* common hooks */ bool (*decide_retry)(struct libnvmf_context *fctx, int err, @@ -48,8 +70,7 @@ struct libnvmf_context { // !generate-accessors:read=generated,write=generated /* common fabrics configuration */ const char *device; - enum libnvmf_tristate persistent; - enum libnvmf_tristate epcsd; + enum libnvmf_persistent persistent; // !access:read=custom,write=custom const char *devid_file; // !access:write=custom /* discovery invocation options */ @@ -69,6 +90,11 @@ struct libnvmf_context { // !generate-accessors:read=generated,write=generated const char *tls_key_identity; // !access:write=custom }; +/* + * Parse a "persistent" mode string ("no", "auto", "force", case-insensitive). + */ +int _libnvmf_persistent_from_str(const char *str, enum libnvmf_persistent *val); + /** * NVMe-oF private struct definitions. * diff --git a/libnvme/test/config-api.c b/libnvme/test/config-api.c index 0c64a45ffd..96054261a4 100644 --- a/libnvme/test/config-api.c +++ b/libnvme/test/config-api.c @@ -21,6 +21,8 @@ #include +#include + #include #include #include @@ -492,8 +494,7 @@ static bool test_apply_params(struct libnvme_global_ctx *ctx) shr_assert(!libnvmf_params_set(params, "keep-alive-tmo", "30")); shr_assert(!libnvmf_params_set(params, "tls", "true")); shr_assert(!libnvmf_params_set(params, "hdr-digest", "false")); - shr_assert(!libnvmf_params_set(params, "persistent", "true")); - shr_assert(!libnvmf_params_set(params, "epcsd", "true")); + shr_assert(!libnvmf_params_set(params, "persistent", "force")); /* Explicit resets: must be skipped, not applied as "0". */ shr_assert(!libnvmf_params_set(params, "tos", "")); shr_assert(!libnvmf_params_set(params, "ctrl-loss-tmo", "")); @@ -522,14 +523,17 @@ static bool test_apply_params(struct libnvme_global_ctx *ctx) printf(" - int fields applied (hex + decimal) [PASS]\n"); } - if (!libnvmf_context_get_tls(fctx) || - libnvmf_context_get_hdr_digest(fctx) || - libnvmf_context_get_persistent(fctx) != LIBNVMF_TRISTATE_TRUE || - libnvmf_context_get_epcsd(fctx) != LIBNVMF_TRISTATE_TRUE) { - printf(" - bool fields [FAIL]\n"); - pass = false; - } else { - printf(" - bool fields applied [PASS]\n"); + { + const char *p = libnvmf_context_get_persistent(fctx); + + if (!libnvmf_context_get_tls(fctx) || + libnvmf_context_get_hdr_digest(fctx) || + !shr_streq0(p, "force")) { + printf(" - bool fields [FAIL]\n"); + pass = false; + } else { + printf(" - bool fields applied [PASS]\n"); + } } /* A fresh context starts at tos=-1, ctrl-loss-tmo=600 (see @@ -571,22 +575,21 @@ static bool test_apply_params(struct libnvme_global_ctx *ctx) } /* - * persistent/epcsd must distinguish "not configured" (UNSET, the library - * default derived elsewhere, e.g. from the discovery log page) from an - * explicit "false" -- a plain bool cannot represent that third state. + * persistent must distinguish "not configured" (UNSET, the library default + * derived elsewhere, e.g. from the discovery log page) from an explicit + * "no" -- a plain bool cannot represent that third state. */ -static bool test_persistent_epcsd_tristate(struct libnvme_global_ctx *ctx) +static bool test_persistent_tristate(struct libnvme_global_ctx *ctx) { struct libnvmf_context *fctx; struct libnvmf_params *params; bool pass = true; - printf("test_persistent_epcsd_tristate:\n"); + printf("test_persistent_tristate:\n"); shr_assert(!libnvmf_context_create(ctx, NULL, NULL, NULL, NULL, &fctx)); - if (libnvmf_context_get_persistent(fctx) != LIBNVMF_TRISTATE_UNSET || - libnvmf_context_get_epcsd(fctx) != LIBNVMF_TRISTATE_UNSET) { + if (libnvmf_context_get_persistent(fctx) != NULL) { printf(" - fresh context defaults to unset [FAIL]\n"); pass = false; } else { @@ -595,16 +598,18 @@ static bool test_persistent_epcsd_tristate(struct libnvme_global_ctx *ctx) params = libnvmf_params_new(); shr_assert(params); - shr_assert(!libnvmf_params_set(params, "persistent", "false")); - shr_assert(!libnvmf_params_set(params, "epcsd", "false")); + shr_assert(!libnvmf_params_set(params, "persistent", "no")); shr_assert(!libnvmf_context_apply_params(fctx, params)); - if (libnvmf_context_get_persistent(fctx) != LIBNVMF_TRISTATE_FALSE || - libnvmf_context_get_epcsd(fctx) != LIBNVMF_TRISTATE_FALSE) { - printf(" - explicit \"false\" recorded distinctly from unset [FAIL]\n"); - pass = false; - } else { - printf(" - explicit \"false\" recorded distinctly from unset [PASS]\n"); + { + const char *p = libnvmf_context_get_persistent(fctx); + + if (!shr_streq0(p, "no")) { + printf(" - explicit \"no\" != unset [FAIL]\n"); + pass = false; + } else { + printf(" - explicit \"no\" != unset [PASS]\n"); + } } libnvmf_context_free(fctx); @@ -834,7 +839,7 @@ int main(void) pass &= test_emit(ctx, &fx); pass &= test_hostnqn_precedence(ctx, &fx); pass &= test_apply_params(ctx); - pass &= test_persistent_epcsd_tristate(ctx); + pass &= test_persistent_tristate(ctx); pass &= test_set_connection_from_tid(ctx); pass &= test_edge_cases(ctx, &fx); diff --git a/libnvme/test/config-ini.c b/libnvme/test/config-ini.c index 7ca48d41a6..3bdfc62870 100644 --- a/libnvme/test/config-ini.c +++ b/libnvme/test/config-ini.c @@ -140,7 +140,6 @@ static bool test_key_table(void) { "ctrl-loss-tmo", LIBNVMF_KEY_TUNABLE }, { "hdr-digest", LIBNVMF_KEY_TUNABLE }, { "persistent", LIBNVMF_KEY_DC_TUNABLE }, - { "epcsd", LIBNVMF_KEY_DC_TUNABLE }, { "tls-key", LIBNVMF_KEY_SECURITY }, { "dhchap-secret", LIBNVMF_KEY_SECURITY }, { "hostnqn", LIBNVMF_KEY_IDENTITY }, @@ -284,8 +283,7 @@ static bool test_parse_model(struct libnvme_global_ctx *ctx) "dhchap-secret = DHHC-1:00:abc\n" "\n" "[Discovery Controller]\n" - "persistent = true\n" - "epcsd = true\n" + "persistent = auto\n" "controller = transport=tcp;traddr=10.0.0.5;trsvcid=8009\n" "\n" "[Subsystem]\n" @@ -344,21 +342,13 @@ static bool test_parse_model(struct libnvme_global_ctx *ctx) } if (!dc || !dc->params || - strcmp(libnvmf_params_get(dc->params, "persistent"), "true")) { + strcmp(libnvmf_params_get(dc->params, "persistent"), "auto")) { printf(" - [Discovery Controller] persistent [FAIL]\n"); pass = false; } else { printf(" - persistent recorded on the DC endpoint [PASS]\n"); } - if (!dc || !dc->params || - strcmp(libnvmf_params_get(dc->params, "epcsd"), "true")) { - printf(" - [Discovery Controller] epcsd [FAIL]\n"); - pass = false; - } else { - printf(" - epcsd recorded on the DC endpoint [PASS]\n"); - } - if (!ss || !ss->params || strcmp(libnvmf_params_get(ss->params, "tls"), "true") || strcmp(libnvmf_params_get(ss->params, "ctrl-loss-tmo"), "1800")) { @@ -466,19 +456,16 @@ static bool test_parse_errors(struct libnvme_global_ctx *ctx) "[Host]\nhostnqn = nqn.2014-08.org.nvmexpress:h\n" "hostid = not-a-uuid\n" }, { "persistent in [I/O Controller Defaults]", - "[I/O Controller Defaults]\npersistent = true\n" }, + "[I/O Controller Defaults]\npersistent = auto\n" }, { "persistent in [Subsystem]", - "[Subsystem]\nnqn = nqn.2014-08.org.nvmexpress:test\npersistent = true\n" }, - { "persistent as a [Subsystem] per-path override", "[Subsystem]\nnqn = nqn.2014-08.org.nvmexpress:test\n" - "controller = transport=tcp;traddr=1.2.3.4;persistent=true\n" }, - { "epcsd in [I/O Controller Defaults]", - "[I/O Controller Defaults]\nepcsd = true\n" }, - { "epcsd in [Subsystem]", - "[Subsystem]\nnqn = nqn.2014-08.org.nvmexpress:test\nepcsd = true\n" }, - { "epcsd as a [Subsystem] per-path override", + "persistent = auto\n" }, + { "persistent as a [Subsystem] per-path override", "[Subsystem]\nnqn = nqn.2014-08.org.nvmexpress:test\n" - "controller = transport=tcp;traddr=1.2.3.4;epcsd=true\n" }, + "controller = transport=tcp;traddr=1.2.3.4;" + "persistent=auto\n" }, + { "bad persistent value", + "[Discovery Controller]\npersistent = maybe\n" }, }; bool pass = true; size_t i; diff --git a/src/argconfig.h b/src/argconfig.h index 246abdd866..11ab23ff4d 100644 --- a/src/argconfig.h +++ b/src/argconfig.h @@ -91,6 +91,16 @@ enum argconfig_types { #define OPT_STRING(l, s, m, v, d, ...) \ {l, s, m, CFG_STRING, v, required_argument, d, false, __VA_ARGS__} +/* + * Like OPT_STRING(), but the value is optional. getopt_long() only + * recognizes an attached value ("--long=value" or "-svalue"), never a + * separate argv token -- initialize *v to a sentinel distinct from NULL + * before parsing to tell "not given" (still the sentinel) apart from + * "given with no value" (optarg is NULL). + */ +#define OPT_STRING_OPTIONAL(l, s, m, v, d, ...) \ + {l, s, m, CFG_STRING, v, optional_argument, d, false, __VA_ARGS__} + #define OPT_FMT(l, s, v, d, ...) OPT_STRING(l, s, "FMT", v, d, __VA_ARGS__) #define OPT_FILE(l, s, v, d, ...) OPT_STRING(l, s, "FILE", v, d, __VA_ARGS__) #define OPT_LIST(l, s, v, d, ...) OPT_STRING(l, s, "LIST", v, d, __VA_ARGS__) diff --git a/src/config-convert.c b/src/config-convert.c index 1d5dba13e7..f0eeb99d7e 100644 --- a/src/config-convert.c +++ b/src/config-convert.c @@ -137,6 +137,12 @@ static void apply_dhchap_default(struct libnvmf_params *params, } } +/* + * The legacy config.json format predates EPCSD; its boolean "persistent" + * meant unconditional persistence. Map true to "force", not the new + * best-effort "auto" default, so migrating an existing config.json doesn't + * silently change behavior for a connection that was persistent before. + */ static void apply_dc_persistent(struct libnvmf_params *params, struct json_object *port_obj) { @@ -144,7 +150,7 @@ static void apply_dc_persistent(struct libnvmf_params *params, if (val) libnvmf_params_set(params, "persistent", - json_object_get_boolean(val) ? "true" : "false"); + json_object_get_boolean(val) ? "force" : "no"); } static int convert_port(struct libnvmf_config_emitter *emitter, @@ -315,8 +321,7 @@ int nvme_config_convert_json(struct libnvmf_config_emitter *emitter, #endif /* CONFIG_JSONC */ int nvme_config_convert_discovery_args(struct libnvmf_config_emitter *emitter, - const struct nvmf_args *fa, enum libnvmf_tristate persistent, - enum libnvmf_tristate epcsd) + const struct nvmf_args *fa, const char *persistent) { struct libnvmf_params *params; int ret; @@ -326,12 +331,14 @@ int nvme_config_convert_discovery_args(struct libnvmf_config_emitter *emitter, return -ENOMEM; nvmf_args_to_params(params, fa); - if (persistent != LIBNVMF_TRISTATE_UNSET) - libnvmf_params_set(params, "persistent", - persistent == LIBNVMF_TRISTATE_TRUE ? "true" : "false"); - if (epcsd != LIBNVMF_TRISTATE_UNSET) - libnvmf_params_set(params, "epcsd", - epcsd == LIBNVMF_TRISTATE_TRUE ? "true" : "false"); + if (persistent && + libnvmf_params_set(params, "persistent", persistent)) { + nvme_show_error( + "discovery.conf: skipping a line with an invalid persistent value '%s'", + persistent); + libnvmf_params_free(params); + return 0; + } ret = libnvmf_config_emit_add(emitter, true, fa->transport, fa->traddr, fa->trsvcid, fa->subsysnqn, fa->host_traddr, diff --git a/src/config-convert.h b/src/config-convert.h index 99799c75c9..27d5cde7e2 100644 --- a/src/config-convert.h +++ b/src/config-convert.h @@ -30,8 +30,7 @@ int nvme_config_convert_discovery(struct libnvmf_config_emitter *emitter, * entries are logged and skipped. Only -ENOMEM is treated as a fatal error. */ int nvme_config_convert_discovery_args(struct libnvmf_config_emitter *emitter, - const struct nvmf_args *fa, enum libnvmf_tristate persistent, - enum libnvmf_tristate epcsd); + const struct nvmf_args *fa, const char *persistent); /* * Auto-convert on first use, called by connect-all/discover/connect diff --git a/src/config-create.c b/src/config-create.c index 913559d37e..7aa05838fa 100644 --- a/src/config-create.c +++ b/src/config-create.c @@ -172,19 +172,13 @@ int nvme_config_create(const char *desc, int argc, char **argv) "own configuration drop-in"; const char *desc_output = "add the entry to this INI configuration " "file (default: " PATH_NVMF_INI ")"; - const char *desc_persistent = "keep the discovery controller " - "connected to receive Asynchronous Event Notifications " - "instead of disconnecting after the discovery log page " - "fetch; requires --discovery"; - const char *desc_no_persistent = "explicitly record that this " - "discovery controller is not persistent, overriding any " - "default that would otherwise apply; requires --discovery"; - const char *desc_epcsd = "mark this discovery controller as " - "supporting Explicit Persistent Connection Support for " - "Discovery (EPCSD); requires --discovery"; - const char *desc_no_epcsd = "explicitly record that this discovery " - "controller does not support EPCSD, overriding any default " - "that would otherwise apply; requires --discovery"; + const char *desc_persistent = "keep the discovery controller connected " + "to receive Asynchronous Event Notifications instead of " + "disconnecting after the discovery log page fetch; \"auto\" " + "(the default when given bare) persists only where the " + "target's own EPCSD flag supports it, \"force\" persists " + "regardless, \"no\" explicitly records non-persistence; " + "requires --discovery"; __cleanup_nvme_global_ctx struct libnvme_global_ctx *ctx = NULL; struct libnvmf_config_emitter *emitter = NULL; @@ -193,10 +187,8 @@ int nvme_config_create(const char *desc, int argc, char **argv) char *output_file = NULL; char *hostsymname = NULL; bool discovery = false; - bool persistent = false; - bool no_persistent = false; - bool epcsd = false; - bool no_epcsd = false; + char *persistent_arg = NULL; + const char *persistent; bool duplicate = false; bool replaced = false; const char *target; @@ -204,10 +196,8 @@ int nvme_config_create(const char *desc, int argc, char **argv) NVMF_ARGS(opts, fa, OPT_FLAG("discovery", 0, &discovery, desc_discovery), - OPT_FLAG("persistent", 0, &persistent, desc_persistent), - OPT_FLAG("no-persistent", 0, &no_persistent, desc_no_persistent), - OPT_FLAG("epcsd", 0, &epcsd, desc_epcsd), - OPT_FLAG("no-epcsd", 0, &no_epcsd, desc_no_epcsd), + OPT_STRING_OPTIONAL("persistent", 0, "no|auto|force", + &persistent_arg, desc_persistent), OPT_STRING("host-symname", 0, "STR", &hostsymname, desc_symname), OPT_STRING("output", 0, "FILE", &output_file, desc_output)); @@ -217,23 +207,10 @@ int nvme_config_create(const char *desc, int argc, char **argv) if (err) return err; - if (persistent && no_persistent) { - nvme_show_error("--persistent and --no-persistent are mutually exclusive"); - return -EINVAL; - } - - if (epcsd && no_epcsd) { - nvme_show_error("--epcsd and --no-epcsd are mutually exclusive"); - return -EINVAL; - } + persistent = nvmf_resolve_persistent_arg(opts, persistent_arg); - if ((persistent || no_persistent) && !discovery) { - nvme_show_error("--persistent/--no-persistent requires --discovery"); - return -EINVAL; - } - - if ((epcsd || no_epcsd) && !discovery) { - nvme_show_error("--epcsd/--no-epcsd requires --discovery"); + if (persistent && !discovery) { + nvme_show_error("--persistent requires --discovery"); return -EINVAL; } @@ -253,14 +230,14 @@ int nvme_config_create(const char *desc, int argc, char **argv) goto out; } nvmf_args_to_params(params, &fa); - if (persistent) - libnvmf_params_set(params, "persistent", "true"); - else if (no_persistent) - libnvmf_params_set(params, "persistent", "false"); - if (epcsd) - libnvmf_params_set(params, "epcsd", "true"); - else if (no_epcsd) - libnvmf_params_set(params, "epcsd", "false"); + if (persistent && + libnvmf_params_set(params, "persistent", persistent)) { + nvme_show_error( + "invalid --persistent value '%s' (expected no, auto, or force)", + persistent); + err = -EINVAL; + goto out; + } err = reload_existing(emitter, ctx, target, discovery, &fa, params, &duplicate, &replaced); diff --git a/src/fabrics.c b/src/fabrics.c index c949d0e803..2e4cc0637e 100644 --- a/src/fabrics.c +++ b/src/fabrics.c @@ -495,6 +495,15 @@ static void consume_conn(const struct libnvmf_config_conn *conn, } } +const char *nvmf_resolve_persistent_arg( + struct argconfig_commandline_options *opts, const char *arg) +{ + if (!argconfig_parse_seen(opts, "persistent")) + return NULL; + + return arg ? arg : "auto"; +} + /* * Parse one discovery.conf line -- the same argv-style syntax 'nvme * discover'/'connect-all' accept, reusing NVMF_ARGS so every short and long @@ -512,18 +521,13 @@ 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 persistent = false, force = false, epcsd = false; - bool no_persistent = false, no_epcsd = false; + bool force = false; + char *persistent_arg = NULL; NVMF_ARGS(opts, fa, - OPT_FLAG("persistent", 'p', &persistent, - "persistent discovery connection"), - OPT_FLAG("no-persistent", 0, &no_persistent, - "explicitly not a persistent discovery connection"), - OPT_FLAG("epcsd", 0, &epcsd, - "Explicit Persistent Connection Support for Discovery"), - OPT_FLAG("no-epcsd", 0, &no_epcsd, - "explicitly does not support Explicit Persistent Connection Support for Discovery"), + OPT_STRING_OPTIONAL("persistent", 'p', "no|auto|force", + &persistent_arg, + "persistent discovery connection mode"), OPT_FLAG("force", 0, &force, "Force persistent discovery controller creation")); @@ -533,7 +537,13 @@ int nvmf_convert_discovery_line(struct libnvmf_config_emitter *emitter, fa.tos = -1; fa.ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO; - while ((ptr = strsep(&p, " =\n")) != NULL && argc < MAX_DISC_ARGS - 1) + /* + * Split on whitespace only, not '=' -- a glued "--opt=value" token + * (e.g. "--persistent=force") must reach argconfig_parse() intact. + * getopt_long() parses '=' within a single token natively for + * every option shape, so splitting it out here is never required. + */ + while ((ptr = strsep(&p, "\n ")) != NULL && argc < MAX_DISC_ARGS - 1) argv[argc++] = ptr; argv[argc] = NULL; @@ -545,10 +555,7 @@ int nvmf_convert_discovery_line(struct libnvmf_config_emitter *emitter, return 0; return nvme_config_convert_discovery_args(emitter, &fa, - persistent ? LIBNVMF_TRISTATE_TRUE : - no_persistent ? LIBNVMF_TRISTATE_FALSE : LIBNVMF_TRISTATE_UNSET, - epcsd ? LIBNVMF_TRISTATE_TRUE : - no_epcsd ? LIBNVMF_TRISTATE_FALSE : LIBNVMF_TRISTATE_UNSET); + nvmf_resolve_persistent_arg(opts, persistent_arg)); } static int setup_common_context(struct libnvmf_context *fctx, @@ -790,8 +797,9 @@ 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 persistent = false, force = false, epcsd = false; - bool no_persistent = false, no_epcsd = false; + bool force = false; + char *persistent_arg = NULL; + const char *persistent; bool nbft = false, nonbft = false; char *nbft_path = NBFT_SYSFS_PATH; char *owner = NULL; @@ -799,14 +807,9 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) NVMF_ARGS(opts, fa, OPT_STRING("device", 'd', "DEV", &device, "use existing discovery controller device"), OPT_FILE("raw", 'r', &raw, "save raw output to file"), - OPT_FLAG("persistent", 'p', &persistent, - "persistent discovery connection"), - OPT_FLAG("no-persistent", 0, &no_persistent, - "explicitly not a persistent discovery connection"), - OPT_FLAG("epcsd", 0, &epcsd, - "Explicit Persistent Connection Support for Discovery"), - OPT_FLAG("no-epcsd", 0, &no_epcsd, - "explicitly does not support Explicit Persistent Connection Support for Discovery"), + OPT_STRING_OPTIONAL("persistent", 'p', "no|auto|force", + &persistent_arg, + "persistent discovery connection mode"), OPT_STRING("config", 'J', "FILE", &config_file, nvmf_config_file), OPT_FLAG("force", 0, &force, "Force persistent discovery controller creation"), OPT_FLAG("nbft", 0, &nbft, "Only look at NBFT tables"), @@ -828,15 +831,7 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) return ret; } - if (epcsd && no_epcsd) { - fprintf(stderr, "--epcsd and --no-epcsd options are mutually exclusive\n"); - return -EINVAL; - } - - if (persistent && no_persistent) { - fprintf(stderr, "--persistent and --no-persistent options are mutually exclusive\n"); - return -EINVAL; - } + persistent = nvmf_resolve_persistent_arg(opts, persistent_arg); if (!strcmp(config_file, "none")) config_file = NULL; @@ -894,17 +889,12 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect) libnvmf_context_set_connect(fctx, connect); libnvmf_context_set_force(fctx, force); - if (epcsd) - libnvmf_context_set_epcsd(fctx, LIBNVMF_TRISTATE_TRUE); - - if (no_epcsd) - libnvmf_context_set_epcsd(fctx, LIBNVMF_TRISTATE_FALSE); - - if (persistent) - libnvmf_context_set_persistent(fctx, LIBNVMF_TRISTATE_TRUE); - - if (no_persistent) - libnvmf_context_set_persistent(fctx, LIBNVMF_TRISTATE_FALSE); + if (persistent && libnvmf_context_set_persistent(fctx, persistent)) { + nvme_show_error( + "invalid --persistent value '%s' (expected no, auto, or force)", + persistent); + return -EINVAL; + } if (!device && !fa.transport && !fa.traddr) { if (!nonbft) { diff --git a/src/fabrics.h b/src/fabrics.h index 31a60c8d00..57aa7efe55 100644 --- a/src/fabrics.h +++ b/src/fabrics.h @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ #pragma once +struct argconfig_commandline_options; + /* Parsed "nvme connect"/"discover"/"connect-all" argv-style arguments. */ struct nvmf_args { const char *subsysnqn; @@ -109,6 +111,15 @@ struct nvmf_args { ##__VA_ARGS__ \ ) +/* + * Resolve an OPT_STRING_OPTIONAL() "--persistent[=no|auto|force]" argument + * to the string libnvmf_context_set_persistent()/libnvmf_params_set() + * expect: NULL if not given at all, "auto" if given bare, or the value as + * typed otherwise. + */ +const char *nvmf_resolve_persistent_arg( + struct argconfig_commandline_options *opts, const char *arg); + int fabrics_discover(const char *desc, int argc, char **argv, bool connect); int fabrics_connect(const char *desc, int argc, char **argv); int fabrics_disconnect(const char *desc, int argc, char **argv); diff --git a/tests/nvme_config_convert_test.py b/tests/nvme_config_convert_test.py index 6d16d101ad..cd02fa7d6c 100644 --- a/tests/nvme_config_convert_test.py +++ b/tests/nvme_config_convert_test.py @@ -203,7 +203,12 @@ def test_dhchap_port_value_overrides_default(self): self.assertIn('dhchap-secret = DHHC-1:00:port-specific-key:', content) self.assertNotIn('DHHC-1:00:host-default-key:', content) - def test_discovery_persistent_true_is_mapped(self): + def test_discovery_persistent_true_is_mapped_to_force(self): + # The legacy format predates EPCSD; its boolean "true" meant + # unconditional persistence, so it maps to "force", not the new + # best-effort "auto" default -- migrating an existing config.json + # must not silently change behavior for a connection that was + # persistent before. self._write_json({ 'hosts': [{ 'subsystems': [{ @@ -220,7 +225,7 @@ def test_discovery_persistent_true_is_mapped(self): self._convert() content = self._read_output() self.assertIn('[Discovery Controller]', content) - self.assertIn('persistent = true', content) + self.assertIn('persistent = force', content) def test_discovery_persistent_false_is_mapped_distinctly(self): self._write_json({ @@ -239,7 +244,7 @@ def test_discovery_persistent_false_is_mapped_distinctly(self): self._convert() content = self._read_output() self.assertIn('[Discovery Controller]', content) - self.assertIn('persistent = false', content) + self.assertIn('persistent = no', content) def test_discovery_without_persistent_key_omits_it(self): self._write_json({ diff --git a/tests/nvme_config_create_test.py b/tests/nvme_config_create_test.py index f0a9f451f5..fcca3b3657 100644 --- a/tests/nvme_config_create_test.py +++ b/tests/nvme_config_create_test.py @@ -116,85 +116,67 @@ def test_host_symname_gets_own_dropin(self): content = self._read_output() self.assertIn('hostsymname = lab-host-01', content) - def test_discovery_persistent_flag_is_stored(self): + def test_discovery_persistent_bare_defaults_to_auto(self): self._create('--transport', 'fc', '--traddr=nn-0x2:pn-0x2', '--discovery', '--persistent') content = self._read_output() self.assertIn('[Discovery Controller]', content) - self.assertIn('persistent = true', content) + self.assertIn('persistent = auto', content) + + def test_discovery_persistent_force_is_stored(self): + self._create('--transport', 'fc', + '--traddr=nn-0x3:pn-0x3', '--discovery', + '--persistent=force') + content = self._read_output() + self.assertIn('[Discovery Controller]', content) + self.assertIn('persistent = force', content) def test_persistent_flag_updates_existing_entry(self): self._create('--transport', 'fc', - '--traddr=nn-0x3:pn-0x3', - '--host-traddr=nn-0x4:pn-0x4', '--discovery') + '--traddr=nn-0x4:pn-0x4', + '--host-traddr=nn-0x5:pn-0x5', '--discovery') content = self._read_output() self.assertNotIn('persistent', content) self._create('--transport', 'fc', - '--traddr=nn-0x3:pn-0x3', - '--host-traddr=nn-0x4:pn-0x4', '--discovery', - '--persistent') + '--traddr=nn-0x4:pn-0x4', + '--host-traddr=nn-0x5:pn-0x5', '--discovery', + '--persistent=force') content = self._read_output() self.assertEqual(content.count('[Discovery Controller]'), 1, f'expected exactly one entry, got content:\n{content}') - self.assertIn('persistent = true', content) + self.assertIn('persistent = force', content) - def test_discovery_no_persistent_flag_is_stored(self): + def test_discovery_persistent_no_is_stored(self): self._create('--transport', 'fc', '--traddr=nn-0x8:pn-0x8', '--discovery', - '--no-persistent') + '--persistent=no') content = self._read_output() self.assertIn('[Discovery Controller]', content) - self.assertIn('persistent = false', content) + self.assertIn('persistent = no', content) - def test_no_persistent_updates_existing_true_entry(self): + def test_persistent_no_updates_existing_force_entry(self): self._create('--transport', 'fc', '--traddr=nn-0xa:pn-0xa', '--discovery', - '--persistent') + '--persistent=force') content = self._read_output() - self.assertIn('persistent = true', content) + self.assertIn('persistent = force', content) self._create('--transport', 'fc', '--traddr=nn-0xa:pn-0xa', '--discovery', - '--no-persistent') + '--persistent=no') content = self._read_output() self.assertEqual(content.count('[Discovery Controller]'), 1, f'expected exactly one entry, got content:\n{content}') - self.assertIn('persistent = false', content) - self.assertNotIn('persistent = true', content) + self.assertIn('persistent = no', content) + self.assertNotIn('persistent = force', content) - def test_discovery_no_persistent_flag_is_stored(self): - self._create('--transport', 'fc', - '--traddr=nn-0x8:pn-0x8', '--discovery', - '--no-persistent') - content = self._read_output() - self.assertIn('[Discovery Controller]', content) - self.assertIn('persistent = false', content) - - def test_discovery_no_epcsd_flag_is_stored(self): + def test_discovery_persistent_bad_value_errors(self): self._create('--transport', 'fc', '--traddr=nn-0x9:pn-0x9', '--discovery', - '--no-epcsd') - content = self._read_output() - self.assertIn('[Discovery Controller]', content) - self.assertIn('epcsd = false', content) - - def test_no_persistent_updates_existing_true_entry(self): - self._create('--transport', 'fc', - '--traddr=nn-0xa:pn-0xa', '--discovery', - '--persistent') - content = self._read_output() - self.assertIn('persistent = true', content) - - self._create('--transport', 'fc', - '--traddr=nn-0xa:pn-0xa', '--discovery', - '--no-persistent') - content = self._read_output() - self.assertEqual(content.count('[Discovery Controller]'), 1, - f'expected exactly one entry, got content:\n{content}') - self.assertIn('persistent = false', content) - self.assertNotIn('persistent = true', content) + '--persistent=maybe', expect_fail=True) + self.assertFalse(os.path.exists(self.output_ini)) def test_second_create_preserves_first_entry(self): self._create('--transport', 'fc', @@ -242,34 +224,16 @@ def test_persistent_without_discovery_errors(self): '--persistent', expect_fail=True) self.assertFalse(os.path.exists(self.output_ini)) - def test_epcsd_without_discovery_errors(self): + def test_persistent_force_without_discovery_errors(self): self._create('--transport', 'tcp', '--traddr=192.168.1.42', '--nqn=nqn.2024-01.com.example:data.vol5', - '--epcsd', expect_fail=True) + '--persistent=force', expect_fail=True) self.assertFalse(os.path.exists(self.output_ini)) - def test_no_persistent_without_discovery_errors(self): + def test_persistent_no_without_discovery_errors(self): self._create('--transport', 'tcp', '--traddr=192.168.1.43', '--nqn=nqn.2024-01.com.example:data.vol6', - '--no-persistent', expect_fail=True) - self.assertFalse(os.path.exists(self.output_ini)) - - def test_no_epcsd_without_discovery_errors(self): - self._create('--transport', 'tcp', '--traddr=192.168.1.44', - '--nqn=nqn.2024-01.com.example:data.vol7', - '--no-epcsd', expect_fail=True) - self.assertFalse(os.path.exists(self.output_ini)) - - def test_persistent_and_no_persistent_mutually_exclusive(self): - self._create('--transport', 'fc', '--traddr=nn-0xb:pn-0xb', - '--discovery', '--persistent', '--no-persistent', - expect_fail=True) - self.assertFalse(os.path.exists(self.output_ini)) - - def test_epcsd_and_no_epcsd_mutually_exclusive(self): - self._create('--transport', 'fc', '--traddr=nn-0xc:pn-0xc', - '--discovery', '--epcsd', '--no-epcsd', - expect_fail=True) + '--persistent=no', expect_fail=True) self.assertFalse(os.path.exists(self.output_ini))