[PATCH rfc 0/6] nvme-cli: nvmf auto-connect scripts

[PATCH rfc 0/6] nvme-cli: nvmf auto-connect scripts

[James Smart]

This posting is a combination of the nvme-fc auto-connect scripts
posted by Hannes, the RFC from Sagi to allow auto-connect for
persistent discovery controllers that send AENs, and the RFC that
I posted addressing comments. It does not contain the code that
handles the persistent discovery controller AEN and generates the
event.

The auto-connect scripts are now commonized with the exception
of the nvme-fc boot connections service.  It's expected that both
the persistent discovery controllers and nvme-fc can post the same
udev event. Whenever the discovery aen generates an event, the
new --device=<name> argument will be used to identify the persistent
discovery controller. When --device is used, the other connect
attributes will be specified, set to the values of the discovery
controller, and will be used by the cli to validate the device is
who it should be or a new discovery controller will be created.
When nvme-fc generates it's events, it will specify --device=none
and specify the discovery controller addressing arguments.  If a
persistent discovery controller exists on nvme-fc, will do nothing
special and AEN's from it will follow the --device=<name> syntax.

The udev event handler supports both the new event to be used
by discovery controllers as well as the existing nvme-fc transport
event. The nvme-fc transport will be migrated to issue the new
event syntax. The udev script will continue to support the older
style nvme-fc event info in case the cli is running against an
older kernel.



James Smart (4):
  nvme-cli: ignore arguments that pass in "none"
  nvme-cli: allow discover to address discovery controller by persistent
    name
  nvme-cli: expand --device argument processing
  nvme-cli: nvmf auto-connect scripts

Sagi Grimberg (2):
  nvme-cli: support persistent connections to a discovery controller
  nvme-cli: add --quiet option

 Makefile                                         |  16 +-
 fabrics.c                                        | 115 ++++++++++---
 fabrics.h                                        |   2 +
 nvme.c                                           | 207 +++++++++++++++++++----
 nvme.h                                           |  21 +++
 nvme.spec.in                                     |   9 +
 nvmf-autoconnect/70-nvmf-autoconnect.conf        |   1 +
 nvmf-autoconnect/70-nvmf-autoconnect.rules       |  18 ++
 nvmf-autoconnect/nvmefc-boot-connections.service |   9 +
 nvmf-autoconnect/nvmf-connect.target             |   2 +
 nvmf-autoconnect/nvmf-connect at .service           |  14 ++
 11 files changed, 359 insertions(+), 55 deletions(-)
 create mode 100644 nvmf-autoconnect/70-nvmf-autoconnect.conf
 create mode 100644 nvmf-autoconnect/70-nvmf-autoconnect.rules
 create mode 100644 nvmf-autoconnect/nvmefc-boot-connections.service
 create mode 100644 nvmf-autoconnect/nvmf-connect.target
 create mode 100644 nvmf-autoconnect/nvmf-connect at .service

-- 
2.13.7

[PATCH rfc 1/6] nvme-cli: ignore arguments that pass in "none"

[James Smart]

As we want to support discovery uevents over different
transports, we want to allow the kernel to provide missing
information in the form of none and have nvme-cli properly
ignore it.

One example is the host_traddr. If it is not set (which means
that the default route determined the host address) we will
want to do the same for newly discovered controllers.

So allow users to pass 'none' arguments as well.

Example:
  nvme connect-all ... --hostnqn=none --hostid=none --host_traddr=none

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: James Smart <jsmart2021 at gmail.com>
Reviewed-by: Hannes Reinecke <hare at suse.com>

---
Patch originated from Sagi in an RFC
Fixed typo in Sagi's patch on cfg.hostid
---
 fabrics.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 573a6ef..cc91d00 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -565,7 +565,7 @@ add_argument(char **argstr, int *max_len, char *arg_str, char *arg)
 {
 	int len;
 
-	if (arg) {
+	if (arg && strcmp(arg, "none")) {
 		len = snprintf(*argstr, *max_len, ",%s=%s", arg_str, arg);
 		if (len < 0)
 			return -EINVAL;
@@ -658,14 +658,14 @@ retry:
 		return -EINVAL;
 	p += len;
 
-	if (cfg.hostnqn) {
+	if (cfg.hostnqn && strcmp(cfg.hostnqn, "none")) {
 		len = sprintf(p, ",hostnqn=%s", cfg.hostnqn);
 		if (len < 0)
 			return -EINVAL;
 		p += len;
 	}
 
-	if (cfg.hostid) {
+	if (cfg.hostid && strcmp(cfg.hostid, "none")) {
 		len = sprintf(p, ",hostid=%s", cfg.hostid);
 		if (len < 0)
 			return -EINVAL;
@@ -700,7 +700,7 @@ retry:
 		p += len;
 	}
 
-	if (cfg.host_traddr) {
+	if (cfg.host_traddr && strcmp(cfg.host_traddr, "none")) {
 		len = sprintf(p, ",host_traddr=%s", cfg.host_traddr);
 		if (len < 0)
 			return -EINVAL;
-- 
2.13.7

[PATCH rfc 2/6] nvme-cli: support persistent connections to a discovery controller

[James Smart]

From: Sagi Grimberg <sagi@grimberg.me>

Simply don't destroy the discovery controller after getting the
log pages. Note that persistent connection to a discovery subsystem
require to pass in a non-zero kato value, so if not provided we
simply use a default of 30 seconds kato.

Example:
  nvme connect-all ... --persistent

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
Reviewed-by: James Smart <jsmart2021 at gmail.com>
Reviewed-by: Hannes Reinecke <hare at suse.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

---
resolved merge error
---
 fabrics.c | 15 ++++++++++++---
 fabrics.h |  2 ++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index cc91d00..c4bbe2c 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -65,6 +65,7 @@ static struct config {
 	int  disable_sqflow;
 	int  hdr_digest;
 	int  data_digest;
+	bool persistent;
 } cfg = { NULL };
 
 #define BUF_SIZE		4096
@@ -858,9 +859,11 @@ static int do_discover(char *argstr, bool connect)
 		return -errno;
 	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec);
 	free(dev_name);
-	err = remove_ctrl(instance);
-	if (err)
-		return err;
+	if (!cfg.persistent) {
+		err = remove_ctrl(instance);
+		if (err)
+			return err;
+	}
 
 	switch (ret) {
 	case DISC_OK:
@@ -936,6 +939,9 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 		if (err)
 			continue;
 
+		if (cfg.persistent && !cfg.keep_alive_tmo)
+			cfg.keep_alive_tmo = NVMF_DEF_DISC_TMO;
+
 		err = build_options(argstr, BUF_SIZE);
 		if (err) {
 			ret = err;
@@ -978,6 +984,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"nr-write-queues", 'W', "LIST", CFG_INT, &cfg.nr_write_queues,    required_argument, "number of write queues to use (default 0)" },
 		{"nr-poll-queues",  'P', "LIST", CFG_INT, &cfg.nr_poll_queues,    required_argument, "number of poll queues to use (default 0)" },
 		{"queue-size",      'Q', "LIST", CFG_INT, &cfg.queue_size,      required_argument, "number of io queue elements to use (default 128)" },
+		{"persistent",  'p', "LIST", CFG_NONE, &cfg.persistent,  no_argument, "persistent discovery connection" },
 		{NULL},
 	};
 
@@ -992,6 +999,8 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		return discover_from_conf_file(desc, argstr,
 				command_line_options, connect);
 	} else {
+		if (cfg.persistent && !cfg.keep_alive_tmo)
+			cfg.keep_alive_tmo = NVMF_DEF_DISC_TMO;
 		ret = build_options(argstr, BUF_SIZE);
 		if (ret)
 			return ret;
diff --git a/fabrics.h b/fabrics.h
index 988f3ef..7c1664b 100644
--- a/fabrics.h
+++ b/fabrics.h
@@ -1,6 +1,8 @@
 #ifndef _DISCOVER_H
 #define _DISCOVER_H
 
+#define NVMF_DEF_DISC_TMO	30
+
 extern int discover(const char *desc, int argc, char **argv, bool connect);
 extern int connect(const char *desc, int argc, char **argv);
 extern int disconnect(const char *desc, int argc, char **argv);
-- 
2.13.7

[PATCH rfc 3/6] nvme-cli: allow discover to address discovery controller by persistent name

[James Smart]

To support discovery (connect/connect-all) to operate against a
persistent discovery controller, let the discovery controller to
be specified by its device node name rather than new connection
attributes.

Example:
  nvme connect-all ... --device=nvme5

Also centralize extraction of controller instance from the controller
name to a common helper.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: James Smart <jsmart2021 at gmail.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

---
Patch originated from Sagi in an RFC
fix merge issue
fix contoller->controller
add cfg.device check for "none"
---
 fabrics.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index c4bbe2c..1c5ca9a 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -189,6 +189,19 @@ static const char *cms_str(__u8 cm)
 
 static int do_discover(char *argstr, bool connect);
 
+static int ctrl_instance(char *device)
+{
+	int ret, instance;
+
+	device = basename(device);
+	ret = sscanf(device, "nvme%d", &instance);
+	if (ret < 0)
+		return ret;
+	if (!ret)
+		return -1;
+	return instance;
+}
+
 static int add_ctrl(const char *argstr)
 {
 	substring_t args[MAX_OPT_ARGS];
@@ -851,7 +864,10 @@ static int do_discover(char *argstr, bool connect)
 	char *dev_name;
 	int instance, numrec = 0, ret, err;
 
-	instance = add_ctrl(argstr);
+	if (!cfg.device)
+		instance = add_ctrl(argstr);
+	else
+		instance = ctrl_instance(cfg.device);
 	if (instance < 0)
 		return instance;
 
@@ -859,7 +875,7 @@ static int do_discover(char *argstr, bool connect)
 		return -errno;
 	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec);
 	free(dev_name);
-	if (!cfg.persistent) {
+	if (!cfg.device && !cfg.persistent) {
 		err = remove_ctrl(instance);
 		if (err)
 			return err;
@@ -975,6 +991,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"hostnqn",     'q', "LIST", CFG_STRING, &cfg.hostnqn,     required_argument, "user-defined hostnqn (if default not used)" },
 		{"hostid",      'I', "LIST", CFG_STRING, &cfg.hostid,      required_argument, "user-defined hostid (if default not used)"},
 		{"raw",         'r', "LIST", CFG_STRING, &cfg.raw,         required_argument, "raw output file" },
+		{"device",      'd', "LIST", CFG_STRING, &cfg.device, required_argument, "use existing discovery controller device" },
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
@@ -993,6 +1010,9 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 	if (ret)
 		return ret;
 
+	if (cfg.device && !strcmp(cfg.device, "none"))
+		cfg.device = NULL;
+
 	cfg.nqn = NVME_DISC_SUBSYS_NAME;
 
 	if (!cfg.transport && !cfg.traddr) {
@@ -1130,15 +1150,10 @@ static int disconnect_by_nqn(char *nqn)
 static int disconnect_by_device(char *device)
 {
 	int instance;
-	int ret;
-
-	device = basename(device);
-	ret = sscanf(device, "nvme%d", &instance);
-	if (ret < 0)
-		return ret;
-	if (!ret)
-		return -1;
 
+	instance = ctrl_instance(device);
+	if (instance < 0)
+		return instance;
 	return remove_ctrl(instance);
 }
 
-- 
2.13.7

[PATCH rfc 4/6] nvme-cli: expand --device argument processing

[James Smart]

The connect-all --device argument was used to specify a specific device
to be used as the discovery controller. The device is typically a
long-lived discovery controller that posted a discovery event.
No attempt was made to ensure the device is who it is supposed to be
before using it.

Revised the code to use the other arguments in the connect-all request
to validate that the device is the entity that was expected. If the
device doesn't match, the cli will look for an existing matching device
(will be a discovery controller due to nqn) with the same connect
parameters and use it.  If one is not found on the system, a new
discovery controller will be created for the connect-all request.

To implement this:
- The code that existed in the subsystem listing that gathers
  controller data is split into a service routine that gets a
  controllers info. The subsystem routine uses the new routine.
- The controller list element is extended to have the connect
  argument fields, and the new routine extended to set them.
- As the routine may be called independently of a subsystem,
  the new routine obtains the subsysnqn.
- A simple parse routine was created to search for the connect
  tokens in a connect-all string or the address string for a
  controller.
- a new struct was added containing connect args. This is used
  for subsequent searches and matches.
- A new routine was created that compares the new connect arg
  struct vs a controller list entry's connect args.
- A new routine was created that loops through all controllers
  and attempts to match a controller to a set of connect
  args passed in to search for.
- The --device handling was augmented to extract the connect
  args from the connect-all request, attempt to match the
  device vs the args, and if no match, looks for a match on
  the system. If not match, the default path will create a
  new discovery controller instance (non-persistent).

Signed-off-by: James Smart <jsmart2021 at gmail.com>
CC: Sagi Grimberg <sagi at grimberg.me>
CC: Hannes Reinecke <hare at suse.com>
---
 fabrics.c |  38 ++++++++++++
 nvme.c    | 207 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 nvme.h    |  21 +++++++
 3 files changed, 236 insertions(+), 30 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 1c5ca9a..73c3a49 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -858,12 +858,50 @@ static int connect_ctrls(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 	return ret;
 }
 
+static const char delim_comma  = ',';
+
 static int do_discover(char *argstr, bool connect)
 {
 	struct nvmf_disc_rsp_page_hdr *log = NULL;
 	char *dev_name;
 	int instance, numrec = 0, ret, err;
 
+	if (cfg.device) {
+		struct connect_args cargs;
+
+		memset(&cargs, 0, sizeof(cargs));
+		cargs.subsysnqn = __parse_connect_arg(argstr, delim_comma,
+						conarg_nqn);
+		cargs.transport = __parse_connect_arg(argstr, delim_comma,
+						conarg_transport);
+		cargs.traddr = __parse_connect_arg(argstr, delim_comma,
+						conarg_traddr);
+		cargs.trsvcid = __parse_connect_arg(argstr, delim_comma,
+						conarg_trsvcid);
+		cargs.host_traddr = __parse_connect_arg(argstr, delim_comma,
+						conarg_host_traddr);
+
+		/*
+		 * if the cfg.device passed in matches the connect args
+		 *    cfg.device is left as-is
+		 * else if there exists a controller that matches the
+		 *         connect args
+		 *    cfg.device is the matching ctrl name
+		 * else if no ctrl matches the connect args
+		 *    cfg.device is set to null. This will attempt to
+		 *    create a new ctrl.
+		 * endif
+		 */
+		if (!ctrl_matches_connectargs(cfg.device, &cargs))
+			cfg.device = find_ctrl_with_connectargs(&cargs);
+
+		free(cargs.subsysnqn);
+		free(cargs.transport);
+		free(cargs.traddr);
+		free(cargs.trsvcid);
+		free(cargs.host_traddr);
+	}
+
 	if (!cfg.device)
 		instance = add_ctrl(argstr);
 	else
diff --git a/nvme.c b/nvme.c
index 9819fcb..cfe1479 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1495,12 +1495,120 @@ static void free_ctrl_list_item(struct ctrl_list_item *ctrls)
 	free(ctrls->address);
 	free(ctrls->state);
 	free(ctrls->ana_state);
+	free(ctrls->subsysnqn);
+	free(ctrls->traddr);
+	free(ctrls->trsvcid);
+	free(ctrls->host_traddr);
+}
+
+static const char delim_space  = ' ';
+const char *conarg_nqn = "nqn";
+const char *conarg_transport = "transport";
+const char *conarg_traddr = "traddr";
+const char *conarg_trsvcid = "trsvcid";
+const char *conarg_host_traddr = "host_traddr";
+
+char *__parse_connect_arg(char *conargs, const char delim, const char *fieldnm)
+{
+	char *s, *e;
+	size_t cnt;
+
+	/*
+	 * Note: by chance, both connect arg strings are set up to
+	 * have traddr field followed by host_traddr field. Thus field
+	 * name matching doesn't overlap. So technically, the loop below
+	 * isn't necessary. But just in case...
+	 */
+	do {
+		s = strstr(conargs, fieldnm);
+		if (!s)
+			goto empty_field;
+		/*
+		 * As there are like field names (traddr vs host_traddr)
+		 * validate prior character is the delimiter. If not,
+		 * skip to next delimeter and resume.
+		 */
+		if (s == conargs || *(s - 1) == delim)
+			break;
+		conargs = strchr(s, delim);
+		if (!conargs)
+			goto empty_field;
+		conargs++;
+	} while (1);
+	s += strlen(fieldnm);
+	if (*s++ != '=')
+		goto empty_field;
+	e = strchr(s, delim);
+	if (e)
+		cnt = e - s;
+	else
+		cnt = strlen(s);
+
+	return strndup(s, cnt);
+
+empty_field:
+	return strdup("\0");
+}
+
+
+static int get_nvme_ctrl_info(char *name, char *path,
+			struct ctrl_list_item *ctrl, __u32 nsid)
+{
+	char ctrl_path[512];
+
+	ctrl->name = strdup(name);
+
+	snprintf(ctrl_path, sizeof(ctrl_path), "%s/%s", path, ctrl->name);
+
+	ctrl->address = get_nvme_ctrl_attr(ctrl_path, "address");
+	if (!ctrl->address) {
+		fprintf(stderr, "%s: failed to get controller address.\n",
+			ctrl->name);
+		goto free_ctrl_items;
+	}
+
+	ctrl->transport = get_nvme_ctrl_attr(ctrl_path, "transport");
+	if (!ctrl->transport) {
+		fprintf(stderr, "%s: failed to get controller transport.\n",
+			ctrl->name);
+		goto free_ctrl_items;
+	}
+
+	ctrl->state = get_nvme_ctrl_attr(ctrl_path, "state");
+	if (!ctrl->state) {
+		fprintf(stderr, "%s: failed to get controller state.\n",
+			ctrl->name);
+		goto free_ctrl_items;
+	}
+
+	if (nsid != NVME_NSID_ALL)
+		ctrl->ana_state = get_nvme_ctrl_path_ana_state(ctrl_path, nsid);
+
+	ctrl->subsysnqn = get_nvme_ctrl_attr(ctrl_path, "subsysnqn");
+	if (!ctrl->subsysnqn) {
+		fprintf(stderr, "%s: failed to get controller subsysnqn.\n",
+			ctrl->name);
+		goto free_ctrl_items;
+	}
+
+	ctrl->traddr = __parse_connect_arg(ctrl->address, delim_space,
+					conarg_traddr);
+	ctrl->trsvcid = __parse_connect_arg(ctrl->address, delim_space,
+					conarg_trsvcid);
+	ctrl->host_traddr = __parse_connect_arg(ctrl->address, delim_space,
+					conarg_host_traddr);
+
+	return 0;	/* success */
+
+free_ctrl_items:
+	free_ctrl_list_item(ctrl);
+
+	return 1;	/* failure */
 }
 
 static int get_nvme_subsystem_info(char *name, char *path,
 				struct subsys_list_item *item, __u32 nsid)
 {
-	char ctrl_path[512];
 	struct dirent **ctrls;
 	int n, i, ret = 1, ccnt = 0;
 
@@ -1527,38 +1635,13 @@ static int get_nvme_subsystem_info(char *name, char *path,
 	item->nctrls = n;
 
 	for (i = 0; i < n; i++) {
-		item->ctrls[ccnt].name = strdup(ctrls[i]->d_name);
-
-		snprintf(ctrl_path, sizeof(ctrl_path), "%s/%s", path,
-			 item->ctrls[ccnt].name);
-
-		item->ctrls[ccnt].address =
-				get_nvme_ctrl_attr(ctrl_path, "address");
-		if (!item->ctrls[ccnt].address) {
-			fprintf(stderr, "failed to get controller[%d] address.\n", i);
-			free_ctrl_list_item(&item->ctrls[ccnt]);
-			continue;
-		}
-
-		item->ctrls[ccnt].transport =
-				get_nvme_ctrl_attr(ctrl_path, "transport");
-		if (!item->ctrls[ccnt].transport) {
-			fprintf(stderr, "failed to get controller[%d] transport.\n", i);
-			free_ctrl_list_item(&item->ctrls[ccnt]);
-			continue;
-		}
-
-		item->ctrls[ccnt].state =
-				get_nvme_ctrl_attr(ctrl_path, "state");
-		if (!item->ctrls[ccnt].state) {
-			fprintf(stderr, "failed to get controller[%d] state.\n", i);
-			free_ctrl_list_item(&item->ctrls[ccnt]);
+		if (get_nvme_ctrl_info(ctrls[i]->d_name, path,
+				&item->ctrls[ccnt], nsid)) {
+			fprintf(stderr, "failed to get controller[%d] info.\n",
+					i);
 			continue;
 		}
 
-		if (nsid != NVME_NSID_ALL)
-			item->ctrls[ccnt].ana_state =
-				get_nvme_ctrl_path_ana_state(ctrl_path, nsid);
 		ccnt++;
 	}
 
@@ -1871,6 +1954,70 @@ static int list(int argc, char **argv, struct command *cmd, struct plugin *plugi
 	return ret;
 }
 
+static char *nvme_ctrl_dir = "/sys/class/nvme/";
+
+bool ctrl_matches_connectargs(char *name, struct connect_args *args)
+{
+	struct ctrl_list_item *ctrl;
+	bool found = false;
+
+	ctrl = malloc(sizeof(*ctrl));
+	if (!ctrl) {
+		fprintf(stderr, "Failed to allocate controller list element\n");
+		return false;
+	}
+	memset(ctrl, 0, sizeof(*ctrl));
+
+	if (get_nvme_ctrl_info(name, nvme_ctrl_dir, ctrl, NVME_NSID_ALL))
+		goto cleanup_exit;
+
+	if (!strcmp(ctrl->subsysnqn, args->subsysnqn) &&
+	    !strcmp(ctrl->transport, args->transport) &&
+	    (!strcmp(ctrl->traddr, args->traddr) ||
+	     !strcmp(args->traddr, "none")) &&
+	    (!strcmp(ctrl->trsvcid, args->trsvcid) ||
+	     !strcmp(args->trsvcid, "none")) &&
+	    (!strcmp(ctrl->host_traddr, args->host_traddr) ||
+	     !strcmp(args->host_traddr, "none")))
+		found = true;
+
+cleanup_exit:
+	free_ctrl_list_item(ctrl);
+	free(ctrl);
+
+	return found;
+}
+
+char *find_ctrl_with_connectargs(struct connect_args *args)
+{
+	struct dirent **devices;
+	char *devname = NULL;
+	int i, n;
+
+	n = scandir(nvme_ctrl_dir, &devices, scan_ctrls_filter, alphasort);
+	if (n < 0) {
+		fprintf(stderr, "no NVMe controller(s) detected.\n");
+		return NULL;
+	}
+
+	for (i = 0; i < n; i++) {
+		if (ctrl_matches_connectargs(devices[i]->d_name, args)) {
+			devname = strdup(devices[i]->d_name);
+			if (devname == NULL)
+				fprintf(stderr, "no memory for ctlr name %s\n",
+						devices[i]->d_name);
+			goto cleanup_devices;
+		}
+	}
+
+cleanup_devices:
+	for (i = 0; i < n; i++)
+		free(devices[i]);
+	free(devices);
+
+	return devname;
+}
+
 int __id_ctrl(int argc, char **argv, struct command *cmd, struct plugin *plugin, void (*vs)(__u8 *vs, struct json_object *root))
 {
 	const char *desc = "Send an Identify Controller command to "\
diff --git a/nvme.h b/nvme.h
index a149005..5d71541 100644
--- a/nvme.h
+++ b/nvme.h
@@ -148,6 +148,10 @@ struct ctrl_list_item {
 	char *transport;
 	char *state;
 	char *ana_state;
+	char *subsysnqn;
+	char *traddr;
+	char *trsvcid;
+	char *host_traddr;
 };
 
 struct subsys_list_item {
@@ -163,6 +167,23 @@ enum {
 	BINARY,
 };
 
+struct connect_args {
+	char *subsysnqn;
+	char *transport;
+	char *traddr;
+	char *trsvcid;
+	char *host_traddr;
+};
+
+bool ctrl_matches_connectargs(char *name, struct connect_args *args);
+char *find_ctrl_with_connectargs(struct connect_args *args);
+char *__parse_connect_arg(char *conargs, const char delim, const char *fieldnm);
+extern const char *conarg_nqn;
+extern const char *conarg_transport;
+extern const char *conarg_traddr;
+extern const char *conarg_trsvcid;
+extern const char *conarg_host_traddr;
+
 void register_extension(struct plugin *plugin);
 
 #include "argconfig.h"
-- 
2.13.7

[PATCH rfc 5/6] nvme-cli: add --quiet option

[James Smart]

From: Sagi Grimberg <sagi@grimberg.me>

Now we are going to have discovery log change events, so
instead of trying to figure out which NVM subsystem ports
are already connected, we let the driver do that and
only suppress the failure messages.

Example:
  nvme connect-all ... --quiet

This option will be used by the discovery log change corresponding
udev rule.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
Reviewed-by: James Smart <jsmart2021 at gmail.com>
Reviewed-by: Hannes Reinecke <hare at suse.com>
---
 fabrics.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 73c3a49..86802b4 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -66,6 +66,7 @@ static struct config {
 	int  hdr_digest;
 	int  data_digest;
 	bool persistent;
+	bool quiet;
 } cfg = { NULL };
 
 #define BUF_SIZE		4096
@@ -216,9 +217,11 @@ static int add_ctrl(const char *argstr)
 		goto out;
 	}
 
-	if (write(fd, argstr, len) != len) {
-		fprintf(stderr, "Failed to write to %s: %s\n",
-			 PATH_NVME_FABRICS, strerror(errno));
+	ret = write(fd, argstr, len);
+	if (ret != len) {
+		if (errno != EALREADY || !cfg.quiet)
+			fprintf(stderr, "Failed to write to %s: %s\n",
+				 PATH_NVME_FABRICS, strerror(errno));
 		ret = -errno;
 		goto out_close;
 	}
@@ -840,10 +843,13 @@ static int connect_ctrls(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 		/* already connected print message	*/
 		if (instance == -EALREADY) {
 			const char *traddr = log->entries[i].traddr;
-			fprintf(stderr,
-				"traddr=%.*s is already connected\n",
-				space_strip_len(NVMF_TRADDR_SIZE, traddr),
-				traddr);
+
+			if (!cfg.quiet)
+				fprintf(stderr,
+					"traddr=%.*s is already connected\n",
+					space_strip_len(NVMF_TRADDR_SIZE,
+							traddr),
+					traddr);
 			continue;
 		}
 
@@ -1040,6 +1046,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"nr-poll-queues",  'P', "LIST", CFG_INT, &cfg.nr_poll_queues,    required_argument, "number of poll queues to use (default 0)" },
 		{"queue-size",      'Q', "LIST", CFG_INT, &cfg.queue_size,      required_argument, "number of io queue elements to use (default 128)" },
 		{"persistent",  'p', "LIST", CFG_NONE, &cfg.persistent,  no_argument, "persistent discovery connection" },
+		{"quiet",       'Q', "LIST", CFG_NONE, &cfg.quiet,  no_argument, "suppress already connected errors" },
 		{NULL},
 	};
 
-- 
2.13.7

[PATCH rfc 6/6] nvme-cli: nvmf auto-connect scripts

[James Smart]

This set of scripts is a combination of those sent by Hannes, Sagi,
and I in earlier patches and RFC's.

Auto-connect operates by the nvme core layer or nvme-fc transport
generating a udev event with directory-controller addressing
information. The nvme core layer generates an event when a
persistent discovery controller generates a Discovery Log Change
Notification AEN.  The nvme-fc transport generates an event when
an FC rport that has a NVME Discovery controller is detected or
when a FC state change event occurs for for an FC rport that has
a NVME Discovery controller

The udev event is handled by a script that extracts the Discovery
controller addressing information and initiates a systemd service
to perform a "nvme connect-all" to the Discovery controller.
The "nvme connect-all" request is not called directly from the udev
handler itself as the request may take some time or stall altogether,
which would block other udev event handling.  By transitioning to
a sytemd service, the call can take as much time as needed to
complete.

The scripts consist of:
- A udev script that handles nvme core and nvme-fc udev events.
  The udev handler starts a nvmf-connect systemd service.
- A nvmf-connect systemd service. The service, in its instance
  name, is passed the connect arguments for the discovery
  controller. The service performs a "nvme connect-all" to the
  discovery controller.
- A nvmefc-boot-connections systemd service. This is a run-once
  service run after udev is enabled, which will replay events
  generated by NVME-FC devices detected during boot while udev
  is not yet running.
- To stop autoconnect an additional nvmefc-connect.target has
  been added, which will instruct systemd to cancel all
  outstanding autoconnect services.

Note: Although the nvme-fc subsystem is converting to use the
  same nvme core layer event mechanism, the nvme-fc-specific
  udev event that has been in existence for a while is contained
  in in the script so that the utilities may run against older
  kernels.

Signed-off-by: James Smart <jsmart2021 at gmail.com>
CC: Simon Schricker <sschricker at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.com>
CC: Sagi Grimberg <sagi at grimberg.me>

---
Added install lines for 70-nvmf-autoconnect.conf and nvmf-connect.target
Revised Makefile to set/use LIBDIR. Hardset to /usr/lib to match the
  nvme.spec.in file
Revised nvme.spec.in to use %{_libdir}
Added --quiet option udev scripts that specify nvme connect-all args
---
 Makefile                                         | 16 ++++++++++++++--
 nvme.spec.in                                     |  9 +++++++++
 nvmf-autoconnect/70-nvmf-autoconnect.conf        |  1 +
 nvmf-autoconnect/70-nvmf-autoconnect.rules       | 18 ++++++++++++++++++
 nvmf-autoconnect/nvmefc-boot-connections.service |  9 +++++++++
 nvmf-autoconnect/nvmf-connect.target             |  2 ++
 nvmf-autoconnect/nvmf-connect at .service           | 14 ++++++++++++++
 7 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 nvmf-autoconnect/70-nvmf-autoconnect.conf
 create mode 100644 nvmf-autoconnect/70-nvmf-autoconnect.rules
 create mode 100644 nvmf-autoconnect/nvmefc-boot-connections.service
 create mode 100644 nvmf-autoconnect/nvmf-connect.target
 create mode 100644 nvmf-autoconnect/nvmf-connect at .service

diff --git a/Makefile b/Makefile
index ebb6b75..0389497 100644
--- a/Makefile
+++ b/Makefile
@@ -8,6 +8,10 @@ DESTDIR =
 PREFIX ?= /usr/local
 SYSCONFDIR = /etc
 SBINDIR = $(PREFIX)/sbin
+LIBDIR ?= /usr/lib
+SYSTEMDDIR ?= $(LIBDIR)/systemd
+UDEVDIR ?= $(LIBDIR)/udev
+DRACUTDIR ?= $(LIBDIR)/dracut
 LIB_DEPENDS =
 
 ifeq ($(LIBUUID),0)
@@ -87,6 +91,14 @@ install-bash-completion:
 	$(INSTALL) -d $(DESTDIR)$(PREFIX)/share/bash-completion/completions
 	$(INSTALL) -m 644 -T ./completions/bash-nvme-completion.sh $(DESTDIR)$(PREFIX)/share/bash-completion/completions/nvme
 
+install-udev:
+	$(INSTALL) -d $(DESTDIR)$(SYSTEMDDIR)/system
+	$(INSTALL) -m 644 ./nvmf-autoconnect/*.service ./nvmf-autoconnect/*.target $(DESTDIR)$(SYSTEMDDIR)/system
+	$(INSTALL) -d $(DESTDIR)$(UDEVDIR)/rules.d
+	$(INSTALL) -m 644 ./nvmf-autoconnect/*.rules $(DESTDIR)$(UDEVDIR)/rules.d
+	$(INSTALL) -d $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
+	$(INSTALL) -m 644 ./nvmf-autoconnect/*.conf $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
+
 install-zsh-completion:
 	$(INSTALL) -d $(DESTDIR)$(PREFIX)/share/zsh/site-functions
 	$(INSTALL) -m 644 -T ./completions/_nvme $(DESTDIR)$(PREFIX)/share/zsh/site-functions/_nvme
@@ -107,7 +119,7 @@ install-etc:
 		$(INSTALL) -m 644 -T ./etc/discovery.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/discovery.conf; \
 	fi
 
-install-spec: install-bin install-man install-bash-completion install-zsh-completion install-etc
+install-spec: install-bin install-man install-bash-completion install-zsh-completion install-etc install-udev
 install: install-spec install-hostparams
 
 nvme.spec: nvme.spec.in NVME-VERSION-FILE
@@ -175,7 +187,7 @@ deb-light: $(NVME) pkg nvme.control.in
 	dpkg-deb --build nvme-$(NVME_VERSION)
 
 rpm: dist
-	$(RPMBUILD) -ta nvme-$(NVME_VERSION).tar.gz
+	$(RPMBUILD) --define '_libdir ${LIBDIR}' -ta nvme-$(NVME_VERSION).tar.gz
 
 .PHONY: default doc all clean clobber install-man install-bin install
 .PHONY: dist pkg dist-orig deb deb-light rpm FORCE test
diff --git a/nvme.spec.in b/nvme.spec.in
index 6934f8f..1286e58 100644
--- a/nvme.spec.in
+++ b/nvme.spec.in
@@ -35,6 +35,11 @@ make install-spec DESTDIR=%{buildroot} PREFIX=/usr
 %{_sysconfdir}/nvme/hostnqn
 %{_sysconfdir}/nvme/hostid
 %{_sysconfdir}/nvme/discovery.conf
+%{_libdir}/udev/rules.d/70-nvmf-autoconnect.rules
+%{_libdir}/dracut/dracut.conf.d/70-nvmf-autoconnect.conf
+%{_libdir}/systemd/system/nvmf-connect at .service
+%{_libdir}/systemd/system/nvmefc-boot-connections.service
+%{_libdir}/systemd/system/nvmf-connect.target
 
 %clean
 rm -rf $RPM_BUILD_ROOT
@@ -47,6 +52,10 @@ if [ $1 -eq 1 ]; then # 1 : This package is being installed for the first time
         if [ ! -s %{_sysconfdir}/nvme/hostid ]; then
                 uuidgen > %{_sysconfdir}/nvme/hostid
         fi
+
+	# apply udev and systemd changes that we did
+	udevadm control --reload-rules && udevadm trigger
+	systemctl daemon-reload
 fi
 
 %changelog
diff --git a/nvmf-autoconnect/70-nvmf-autoconnect.conf b/nvmf-autoconnect/70-nvmf-autoconnect.conf
new file mode 100644
index 0000000..844f3d9
--- /dev/null
+++ b/nvmf-autoconnect/70-nvmf-autoconnect.conf
@@ -0,0 +1 @@
+install_items+="/usr/lib/udev/rules.d/70-nvmf-autoconnect.rules"
diff --git a/nvmf-autoconnect/70-nvmf-autoconnect.rules b/nvmf-autoconnect/70-nvmf-autoconnect.rules
new file mode 100644
index 0000000..b241b12
--- /dev/null
+++ b/nvmf-autoconnect/70-nvmf-autoconnect.rules
@@ -0,0 +1,18 @@
+#
+# nvmf-autoconnect.rules:
+#   Handles udev events which invoke automatically scan via discovery
+#   controller and connect to elements in the discovery log.
+#
+#
+
+# Events from persistent discovery controllers or nvme-fc transport events
+ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_EVENT}=="discovery",\
+  ENV{NVME_CTLR_NAME}=="*", ENV{NVME_TRTYPE}=="*", ENV{NVME_TRADDR}=="*", \
+  ENV{NVME_TRSVCID}=="*", ENV{NVME_HOST_TRADDR}=="*", \
+  RUN+="/usr/bin/systemctl --no-block start nvmf-connect at --quiet\t--device=$env{NVME_CTLR_NAME}\t--transport=$env{NVME_TRTYPE}\t--traddr=$env{NVME_TRADDR}\t--trsvcid=$env{NVME_TRSVCID}\t--host-traddr=$env{NVME_HOST_TRADDR}.service"
+
+# nvme-fc transport generated events (old-style for compatibility)
+ACTION=="change", SUBSYSTEM=="fc", ENV{FC_EVENT}=="nvmediscovery", \
+  ENV{NVMEFC_HOST_TRADDR}=="*",  ENV{NVMEFC_TRADDR}=="*", \
+  RUN+="/usr/bin/systemctl --no-block start nvmf-connect at --quiet\t--device=none\t--transport=fc\t--traddr=$env{NVMEFC_TRADDR}\t--trsvcid=none\t--host-traddr=$env{NVMEFC_HOST_TRADDR}.service"
+
diff --git a/nvmf-autoconnect/nvmefc-boot-connections.service b/nvmf-autoconnect/nvmefc-boot-connections.service
new file mode 100644
index 0000000..84f6486
--- /dev/null
+++ b/nvmf-autoconnect/nvmefc-boot-connections.service
@@ -0,0 +1,9 @@
+[Unit]
+Description=Auto-connect to subsystems on FC-NVME devices found during boot
+
+[Service]
+Type=oneshot
+ExecStart=/bin/sh -c "echo add > /sys/class/fc/fc_udev_device/nvme_discovery"
+
+[Install]
+WantedBy=default.target
diff --git a/nvmf-autoconnect/nvmf-connect.target b/nvmf-autoconnect/nvmf-connect.target
new file mode 100644
index 0000000..f64a37c
--- /dev/null
+++ b/nvmf-autoconnect/nvmf-connect.target
@@ -0,0 +1,2 @@
+[Unit]
+Description=All instances of nvmf-autoconnect daemon
diff --git a/nvmf-autoconnect/nvmf-connect at .service b/nvmf-autoconnect/nvmf-connect at .service
new file mode 100644
index 0000000..7d9f856
--- /dev/null
+++ b/nvmf-autoconnect/nvmf-connect at .service
@@ -0,0 +1,14 @@
+#
+# Unit file used by 70-nvmf-autoconnect.rules.
+#
+
+[Unit]
+Description=NVMf auto-connect scan upon nvme discovery controller Events
+After=syslog.target
+PartOf=nvmf-connect.target
+Requires=nvmf-connect.target
+
+[Service]
+Type=simple
+Environment="CONNECT_ARGS=%i"
+ExecStart=/bin/sh -c "/usr/sbin/nvme connect-all `/usr/bin/echo -e $CONNECT_ARGS`"
-- 
2.13.7

[PATCH rfc 6/6] nvme-cli: nvmf auto-connect scripts

[Sagi Grimberg]

> +	$(INSTALL) -d $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
> +	$(INSTALL) -m 644 ./nvmf-autoconnect/*.conf $(DESTDIR)$(DRACUTDIR)/dracut.conf.d

Where is this file?

[PATCH rfc 4/6] nvme-cli: expand --device argument processing

[Sagi Grimberg]

> The connect-all --device argument was used to specify a specific device
> to be used as the discovery controller. The device is typically a
> long-lived discovery controller that posted a discovery event.
> No attempt was made to ensure the device is who it is supposed to be
> before using it.
> 
> Revised the code to use the other arguments in the connect-all request
> to validate that the device is the entity that was expected. If the
> device doesn't match, the cli will look for an existing matching device
> (will be a discovery controller due to nqn) with the same connect
> parameters and use it.  If one is not found on the system, a new
> discovery controller will be created for the connect-all request.

This can really use some splitting up, not easy to review this patch...

[PATCH rfc 1/6] nvme-cli: ignore arguments that pass in "none"

[Minwoo Im]

Loos good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH rfc 2/6] nvme-cli: support persistent connections to a discovery controller

[Minwoo Im]

On 19-07-10 16:27:36, James Smart wrote:
> From: Sagi Grimberg <sagi at grimberg.me>
> 
> Simply don't destroy the discovery controller after getting the
> log pages. Note that persistent connection to a discovery subsystem
> require to pass in a non-zero kato value, so if not provided we
> simply use a default of 30 seconds kato.
> 
> Example:
>   nvme connect-all ... --persistent
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> Reviewed-by: James Smart <jsmart2021 at gmail.com>
> Reviewed-by: Hannes Reinecke <hare at suse.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

This is what I really wanted to make it support!

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

Thanks!

[PATCH rfc 3/6] nvme-cli: allow discover to address discovery controller by persistent name

[Minwoo Im]

On 19-07-10 16:27:37, James Smart wrote:
> To support discovery (connect/connect-all) to operate against a
> persistent discovery controller, let the discovery controller to
> be specified by its device node name rather than new connection
> attributes.
> 
> Example:
>   nvme connect-all ... --device=nvme5
> 
> Also centralize extraction of controller instance from the controller
> name to a common helper.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> Signed-off-by: James Smart <jsmart2021 at gmail.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Hi Sagi and James,

I think it might be too late review on this, but please have a look my
comments below :)

> +static int ctrl_instance(char *device)
> +{
> +	int ret, instance;
> +
> +	device = basename(device);
> +	ret = sscanf(device, "nvme%d", &instance);
> +	if (ret < 0)
> +		return ret;

It's just nitpick, but could we just do like:

	ret = sscanf(device, "nvme%d", &instance);
	if (ret < 0)
		return -errno;

> +	if (!ret)
> +		return -1;

Same here.  Can we please do:

	if (!ret)
		return -EINVAL;


These two things are not from this commit, but if you don't like it to
be involved in this scope of the commit, I think I can make it later, if
you don't mind.

> +	return instance;
> +}

I have a doubt here.  In case of multipath, if this function is given an
argument like "nvme0n1", Is "0" really an instance of that controller?
I think it could be an instance of the subsystem.  If so, can we just
prevent the argstr as a namespace node?

Please correct me if I'm wrong here.

[PATCH rfc 5/6] nvme-cli: add --quiet option

[Minwoo Im]

On 19-07-10 16:27:39, James Smart wrote:
> From: Sagi Grimberg <sagi at grimberg.me>
> 
> Now we are going to have discovery log change events, so
> instead of trying to figure out which NVM subsystem ports
> are already connected, we let the driver do that and
> only suppress the failure messages.
> 
> Example:
>   nvme connect-all ... --quiet
> 
> This option will be used by the discovery log change corresponding
> udev rule.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> Reviewed-by: James Smart <jsmart2021 at gmail.com>
> Reviewed-by: Hannes Reinecke <hare at suse.com>

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH rfc 3/6] nvme-cli: allow discover to address discovery controller by persistent name

[James Smart]

On 7/11/2019 5:41 AM, Minwoo Im wrote:
> On 19-07-10 16:27:37, James Smart wrote:
>> To support discovery (connect/connect-all) to operate against a
>> persistent discovery controller, let the discovery controller to
>> be specified by its device node name rather than new connection
>> attributes.
>>
>> Example:
>>    nvme connect-all ... --device=nvme5
>>
>> Also centralize extraction of controller instance from the controller
>> name to a common helper.
>>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> Signed-off-by: James Smart <jsmart2021 at gmail.com>
>> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> 
> Hi Sagi and James,
> 
> I think it might be too late review on this, but please have a look my
> comments below :)

well,  it was posted as an RFC, so....


> 
>> +static int ctrl_instance(char *device)
>> +{
>> +	int ret, instance;
>> +
>> +	device = basename(device);
>> +	ret = sscanf(device, "nvme%d", &instance);
>> +	if (ret < 0)
>> +		return ret;
> 
> It's just nitpick, but could we just do like:
> 
> 	ret = sscanf(device, "nvme%d", &instance);
> 	if (ret < 0)
> 		return -errno;

I would think that what sscanf might report as a problem may not be 
something understandable for someone using the cli. I'd like to replace
this with something like -EINVAL for an invalid argument rather than 
ret/-errorno.


> 
>> +	if (!ret)
>> +		return -1;
> 
> Same here.  Can we please do:
> 
> 	if (!ret)
> 		return -EINVAL;

yep


> 
> 
> These two things are not from this commit, but if you don't like it to
> be involved in this scope of the commit, I think I can make it later, if
> you don't mind.
> 
>> +	return instance;
>> +}
> 
> I have a doubt here.  In case of multipath, if this function is given an
> argument like "nvme0n1", Is "0" really an instance of that controller?
> I think it could be an instance of the subsystem.  If so, can we just
> prevent the argstr as a namespace node?
> 
> Please correct me if I'm wrong here.

it should never be given an argument with the n? suffix. We're dealing 
with controllers only. And multipath won't confuse that.  Perhaps we 
should verify it ends at the controller name.

-- james

[PATCH rfc 6/6] nvme-cli: nvmf auto-connect scripts

[James Smart]

On 7/10/2019 5:42 PM, Sagi Grimberg wrote:
> 
>> +??? $(INSTALL) -d $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
>> +??? $(INSTALL) -m 644 ./nvmf-autoconnect/*.conf 
>> $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
> 
> Where is this file?

?? it's not a new file   it's installing 70-nvmf-autoconnect.conf into 
the dracut directories so that dracut will install the udev files in the 
ramdisk.

-- james

[PATCH rfc 3/6] nvme-cli: allow discover to address discovery controller by persistent name

[Minwoo Im]

On 19-07-11 09:16:37, James Smart wrote:
> On 7/11/2019 5:41 AM, Minwoo Im wrote:
> > On 19-07-10 16:27:37, James Smart wrote:
> > > To support discovery (connect/connect-all) to operate against a
> > > persistent discovery controller, let the discovery controller to
> > > be specified by its device node name rather than new connection
> > > attributes.
> > > 
> > > Example:
> > >    nvme connect-all ... --device=nvme5
> > > 
> > > Also centralize extraction of controller instance from the controller
> > > name to a common helper.
> > > 
> > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> > > Signed-off-by: James Smart <jsmart2021 at gmail.com>
> > > Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> > 
> > Hi Sagi and James,
> > 
> > I think it might be too late review on this, but please have a look my
> > comments below :)
> 
> well,  it was posted as an RFC, so....

Yup, Thanks :)

> > 
> > > +static int ctrl_instance(char *device)
> > > +{
> > > +	int ret, instance;
> > > +
> > > +	device = basename(device);
> > > +	ret = sscanf(device, "nvme%d", &instance);
> > > +	if (ret < 0)
> > > +		return ret;
> > 
> > It's just nitpick, but could we just do like:
> > 
> > 	ret = sscanf(device, "nvme%d", &instance);
> > 	if (ret < 0)
> > 		return -errno;
> 
> I would think that what sscanf might report as a problem may not be
> something understandable for someone using the cli. I'd like to replace
> this with something like -EINVAL for an invalid argument rather than
> ret/-errorno.

-EINVAL would be fine to me also.

> > 
> > > +	if (!ret)
> > > +		return -1;
> > 
> > Same here.  Can we please do:
> > 
> > 	if (!ret)
> > 		return -EINVAL;
> 
> yep
> 
> 
> > 
> > 
> > These two things are not from this commit, but if you don't like it to
> > be involved in this scope of the commit, I think I can make it later, if
> > you don't mind.
> > 
> > > +	return instance;
> > > +}
> > 
> > I have a doubt here.  In case of multipath, if this function is given an
> > argument like "nvme0n1", Is "0" really an instance of that controller?
> > I think it could be an instance of the subsystem.  If so, can we just
> > prevent the argstr as a namespace node?
> > 
> > Please correct me if I'm wrong here.
> 
> it should never be given an argument with the n? suffix. We're dealing with
> controllers only. And multipath won't confuse that.  Perhaps we should
> verify it ends at the controller name.

Yeah.. It should not be given like that.  But I think it would be great
if we can verify the given argument indicates a controller, not a
namespace node in somewhere because people are sometimes get confused
and may give some ns node here ;(

[PATCH rfc 6/6] nvme-cli: nvmf auto-connect scripts

[Sagi Grimberg]

>>> +??? $(INSTALL) -d $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
>>> +??? $(INSTALL) -m 644 ./nvmf-autoconnect/*.conf 
>>> $(DESTDIR)$(DRACUTDIR)/dracut.conf.d
>>
>> Where is this file?
> 
> ?? it's not a new file?? it's installing 70-nvmf-autoconnect.conf into 
> the dracut directories so that dracut will install the udev files in the 
> ramdisk.
> 
> -- james

Right :)

Disregard..

[PATCH rfc 4/6] nvme-cli: expand --device argument processing

[Sagi Grimberg]

> The connect-all --device argument was used to specify a specific device
> to be used as the discovery controller. The device is typically a
> long-lived discovery controller that posted a discovery event.
> No attempt was made to ensure the device is who it is supposed to be
> before using it.
> 
> Revised the code to use the other arguments in the connect-all request
> to validate that the device is the entity that was expected. If the
> device doesn't match, the cli will look for an existing matching device
> (will be a discovery controller due to nqn) with the same connect
> parameters and use it.  If one is not found on the system, a new
> discovery controller will be created for the connect-all request.

Remind me James, this is to protect the case where the discovery
controller is deleted and a new one was added instead?

> To implement this:
> - The code that existed in the subsystem listing that gathers
>    controller data is split into a service routine that gets a
>    controllers info. The subsystem routine uses the new routine.

Separate patch

> - The controller list element is extended to have the connect
>    argument fields, and the new routine extended to set the > - As the routine may be called independently of a subsystem,
>    the new routine obtains the subsysnqn.

Squashed with the first patch.

> - A simple parse routine was created to search for the connect
>    tokens in a connect-all string or the address string for a
>    controller.
> - a new struct was added containing connect args. This is used
>    for subsequent searches and matches.

Separate patch?

> - A new routine was created that compares the new connect arg
>    struct vs a controller list entry's connect args.

Separate patch?

> - A new routine was created that loops through all controllers
>    and attempts to match a controller to a set of connect
>    args passed in to search for.

Separate patch?

> - The --device handling was augmented to extract the connect
>    args from the connect-all request, attempt to match the
>    device vs the args, and if no match, looks for a match on
>    the system. If not match, the default path will create a
>    new discovery controller instance (non-persistent).

Final patch?