[PATCH v3 0/9] Add Subsystem/Ctrl/Namespace relations (nvme-cli)

[PATCH v3 0/9] Add Subsystem/Ctrl/Namespace relations (nvme-cli)

[Max Gurtovoy]

This patchset introduce few small bug fixes for memory leaks, improve
coding style, improve code readability and mainly focus on the "nvme list-subsys"
command. This command will show the whole NVM subsystem list (same as "nvme list"
does for namespaces).
The patchset ends with adding namespaces entry per each ctrl in the system.
Actually, at first stage, we'll print only the namespace handle (nvme0n1 for
example). Later on, we can add more attribute per demand (uuid, nguid, capacity,
etc...).

Changes from V2:
 - Added one more Reviewed-by sign (thanks Minwoo Im)
 - Added debug print for invalid nsid (proposed by Kenneth)
 - Updated the output for native NVMe multipath device to print "head" instead
   of hidden devices (proposed by Keith and Christopth)

Changes from V1:
 - Added Reviewed-by sign (thanks Minwoo Im)
 - Added check for invalid nsid (proposed by Minwoo Im)
 - Fixed the output for native NVMe multipath

Max Gurtovoy (9):
  nvme: update list-ns nsid option
  nvme: update description for "nvme list" command
  fabrics: Fix memory leak of subsys list
  nvme-print: fix json object memory leak
  nvme: fix coding style issue
  nvme: update list-subsys command to show the entire list
  nvme-print: Introduce show_nvme_ctrl helper
  nvme-print: Rename "Paths" --> "Ctrls" for json output in list-subsys
  nvme: Retrieve namespaces during list-subsys cmd

 fabrics.c    |   6 +-
 nvme-print.c |  52 ++++++----
 nvme.c       | 305 ++++++++++++++++++++++++++---------------------------------
 nvme.h       |   9 +-
 4 files changed, 181 insertions(+), 191 deletions(-)

-- 
1.8.3.1

[PATCH 1/9] nvme: update list-ns nsid option

[Max Gurtovoy]

This commit updates the optional nsid argument to define the wanted
nsid for start, instead of starting from nsid + 1. E.g. in case we've
wanted to get the list of namespaces starting from 1, before this
commit, we used the "--namespace-id=0" option. Nsid 0 is not valid in
NVMe spec, thus change it to start counting from the given nsid.

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/nvme.c b/nvme.c
index 9819fcb..9d763f5 100644
--- a/nvme.c
+++ b/nvme.c
@@ -950,9 +950,9 @@ close_fd:
 
 static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *plugin)
 {
-	const char *desc = "For the specified device, show the "\
-		"namespace list in a NVMe subsystem, optionally starting with a given namespace";
-	const char *namespace_id = "namespace number returned list should to start after";
+	const char *desc = "For the specified controller handle, show the "\
+		"namespace list in the associated NVMe subsystem, optionally starting with a given nsid.";
+	const char *namespace_id = "first nsid returned list should start from";
 	const char *all = "show all namespaces in the subsystem, whether attached or inactive";
 	int err, i, fd;
 	__u32 ns_list[1024];
@@ -963,7 +963,7 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	};
 
 	struct config cfg = {
-		.namespace_id = 0,
+		.namespace_id = 1,
 	};
 
 	const struct argconfig_commandline_options command_line_options[] = {
@@ -976,7 +976,14 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	if (fd < 0)
 		return fd;
 
-	err = nvme_identify_ns_list(fd, cfg.namespace_id, !!cfg.all, ns_list);
+	if (!cfg.namespace_id) {
+		err = -EINVAL;
+		fprintf(stderr, "invalid nsid parameter\n");
+		goto close_fd;
+	}
+
+	err = nvme_identify_ns_list(fd, cfg.namespace_id - 1, !!cfg.all,
+				    ns_list);
 	if (!err) {
 		for (i = 0; i < 1024; i++)
 			if (ns_list[i])
@@ -987,6 +994,7 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	else
 		perror("id namespace list");
 
+close_fd:
 	close(fd);
 
 	return err;
-- 
1.8.3.1

[PATCH 2/9] nvme: update description for "nvme list" command

[Max Gurtovoy]

The "nvme list" command doesn't get any device handle as an input
argument. This operation prints out a basic information for all NVMe
namespaces in the system.

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nvme.c b/nvme.c
index 9d763f5..3310abd 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1801,7 +1801,7 @@ static int list(int argc, char **argv, struct command *cmd, struct plugin *plugi
 	struct list_item *list_items;
 	unsigned int list_cnt = 0;
 	int fmt, ret, fd, i, n;
-	const char *desc = "Retrieve basic information for the given device";
+	const char *desc = "Retrieve basic information for all NVMe namespaces";
 	struct config {
 		char *output_format;
 	};
-- 
1.8.3.1

[PATCH 3/9] fabrics: Fix memory leak of subsys list

[Max Gurtovoy]

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 fabrics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fabrics.c b/fabrics.c
index 511de06..b42a3ce 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1203,5 +1203,6 @@ int disconnect_all(const char *desc, int argc, char **argv)
 		}
 	}
 out:
+	free_subsys_list(slist, subcnt);
 	return ret;
 }
-- 
1.8.3.1

[PATCH 4/9] nvme-print: fix json object memory leak

[Max Gurtovoy]

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme-print.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nvme-print.c b/nvme-print.c
index 2c4822e..6f85e73 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2918,6 +2918,7 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
 		json_object_add_value_array(root, "Subsystems", subsystems);
 	json_print_object(root, NULL);
 	printf("\n");
+	json_free_object(root);
 }
 
 static void show_registers_cap(struct nvme_bar_cap *cap)
-- 
1.8.3.1

[PATCH 5/9] nvme: fix coding style issue

[Max Gurtovoy]

It's more common to use the following coding style:
if (condition) {
   do_that;
   do_this;
} else if (condition2) {
   do_this;
} else {
   do_that;
}

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/nvme.c b/nvme.c
index 3310abd..2fd4362 100644
--- a/nvme.c
+++ b/nvme.c
@@ -988,11 +988,11 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 		for (i = 0; i < 1024; i++)
 			if (ns_list[i])
 				printf("[%4u]:%#x\n", i, le32_to_cpu(ns_list[i]));
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+	} else {
 		perror("id namespace list");
+	}
 
 close_fd:
 	close(fd);
-- 
1.8.3.1

[PATCH 6/9] nvme: update list-subsys command to show the entire list

[Max Gurtovoy]

Make it similar to "nvme list" command. For that, remove the
unnecessary ana_state parsing (was introduced only if specific namespace
handle was given).

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 fabrics.c    |   5 +-
 nvme-print.c |  10 +---
 nvme.c       | 149 +++++++----------------------------------------------------
 nvme.h       |   3 +-
 4 files changed, 23 insertions(+), 144 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index b42a3ce..3dadb4e 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1190,7 +1190,10 @@ int disconnect_all(const char *desc, int argc, char **argv)
 	if (ret)
 		return ret;
 
-	slist = get_subsys_list(&subcnt, NULL, NVME_NSID_ALL);
+	ret = get_subsys_list(&subcnt, &slist);
+	if (ret)
+		return ret;
+
 	for (i = 0; i < subcnt; i++) {
 		struct subsys_list_item *subsys = &slist[i];
 
diff --git a/nvme-print.c b/nvme-print.c
index 6f85e73..7316117 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2851,12 +2851,10 @@ static void show_nvme_subsystem(struct subsys_list_item *item)
 	printf("\\\n");
 
 	for (i = 0; i < item->nctrls; i++) {
-		printf(" +- %s %s %s %s %s\n", item->ctrls[i].name,
+		printf(" +- %s %s %s %s\n", item->ctrls[i].name,
 				item->ctrls[i].transport,
 				item->ctrls[i].address,
-				item->ctrls[i].state,
-				item->ctrls[i].ana_state ?
-					item->ctrls[i].ana_state : "");
+				item->ctrls[i].state);
 	}
 
 }
@@ -2902,10 +2900,6 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
 					slist[i].ctrls[j].address);
 			json_object_add_value_string(path_attrs, "State",
 					slist[i].ctrls[j].state);
-			if (slist[i].ctrls[j].ana_state)
-				json_object_add_value_string(path_attrs,
-						"ANAState",
-						slist[i].ctrls[j].ana_state);
 			json_array_add_value_object(paths, path_attrs);
 		}
 		if (j) {
diff --git a/nvme.c b/nvme.c
index 2fd4362..518a396 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1401,85 +1401,6 @@ err_free_path:
 	return NULL;
 }
 
-static int scan_ctrl_paths_filter(const struct dirent *d)
-{
-	int id, cntlid, nsid;
-
-	if (d->d_name[0] == '.')
-		return 0;
-
-	if (strstr(d->d_name, "nvme")) {
-		if (sscanf(d->d_name, "nvme%dc%dn%d", &id, &cntlid, &nsid) == 3)
-			return 1;
-		if (sscanf(d->d_name, "nvme%dn%d", &id, &nsid) == 2)
-			return 1;
-	}
-
-	return 0;
-}
-
-static char *get_nvme_ctrl_path_ana_state(char *path, int nsid)
-{
-	struct dirent **paths;
-	char *ana_state;
-	int i, n;
-
-	ana_state = calloc(1, 16);
-	if (!ana_state)
-		return NULL;
-
-	n = scandir(path, &paths, scan_ctrl_paths_filter, alphasort);
-	if (n <= 0) {
-		free(ana_state);
-		return NULL;
-	}
-	for (i = 0; i < n; i++) {
-		int id, cntlid, ns, fd;
-		ssize_t ret;
-		char *ctrl_path;
-
-		if (sscanf(paths[i]->d_name, "nvme%dc%dn%d",
-			   &id, &cntlid, &ns) != 3) {
-			if (sscanf(paths[i]->d_name, "nvme%dn%d",
-				   &id, &ns) != 2) {
-				continue;
-			}
-		}
-		if (ns != nsid)
-			continue;
-
-		ret = asprintf(&ctrl_path, "%s/%s/ana_state",
-			       path, paths[i]->d_name);
-		if (ret < 0) {
-			free(ana_state);
-			ana_state = NULL;
-			break;
-		}
-		fd = open(ctrl_path, O_RDONLY);
-		if (fd < 0) {
-			free(ctrl_path);
-			free(ana_state);
-			ana_state = NULL;
-			break;
-		}
-		ret = read(fd, ana_state, 16);
-		if (ret < 0) {
-			fprintf(stderr, "Failed to read ANA state from %s\n",
-				ctrl_path);
-			free(ana_state);
-			ana_state = NULL;
-		} else if (ana_state[strlen(ana_state) - 1] == '\n')
-			ana_state[strlen(ana_state) - 1] = '\0';
-		close(fd);
-		free(ctrl_path);
-		break;
-	}
-	for (i = 0; i < n; i++)
-		free(paths[i]);
-	free(paths);
-	return ana_state;
-}
-
 static int scan_ctrls_filter(const struct dirent *d)
 {
 	int id, nsid;
@@ -1502,11 +1423,10 @@ static void free_ctrl_list_item(struct ctrl_list_item *ctrls)
 	free(ctrls->transport);
 	free(ctrls->address);
 	free(ctrls->state);
-	free(ctrls->ana_state);
 }
 
 static int get_nvme_subsystem_info(char *name, char *path,
-				struct subsys_list_item *item, __u32 nsid)
+				   struct subsys_list_item *item)
 {
 	char ctrl_path[512];
 	struct dirent **ctrls;
@@ -1564,9 +1484,6 @@ static int get_nvme_subsystem_info(char *name, char *path,
 			continue;
 		}
 
-		if (nsid != NVME_NSID_ALL)
-			item->ctrls[ccnt].ana_state =
-				get_nvme_ctrl_path_ana_state(ctrl_path, nsid);
 		ccnt++;
 	}
 
@@ -1631,8 +1548,7 @@ void free_subsys_list(struct subsys_list_item *slist, int n)
 	free(slist);
 }
 
-struct subsys_list_item *get_subsys_list(int *subcnt, char *subsysnqn,
-					 __u32 nsid)
+int get_subsys_list(int *subcnt, struct subsys_list_item **pslist)
 {
 	char path[310];
 	struct dirent **subsys;
@@ -1642,36 +1558,37 @@ struct subsys_list_item *get_subsys_list(int *subcnt, char *subsysnqn,
 	n = scandir(subsys_dir, &subsys, scan_subsys_filter, alphasort);
 	if (n < 0) {
 		fprintf(stderr, "no NVMe subsystem(s) detected.\n");
-		return NULL;
+		return -EINVAL;
 	}
 
 	slist = calloc(n, sizeof(struct subsys_list_item));
-	if (!slist)
+	if (!slist) {
+		ret = -ENOMEM;
 		goto free_subsys;
+	}
 
 	for (i = 0; i < n; i++) {
 		snprintf(path, sizeof(path), "%s%s", subsys_dir,
 			subsys[i]->d_name);
 		ret = get_nvme_subsystem_info(subsys[i]->d_name, path,
-				&slist[*subcnt], nsid);
+				&slist[*subcnt]);
 		if (ret) {
 			fprintf(stderr,
 				"%s: failed to get subsystem info: %s\n",
 				path, strerror(errno));
 			free_subsys_list_item(&slist[*subcnt]);
-		} else if (subsysnqn &&
-			   strncmp(slist[*subcnt].subsysnqn, subsysnqn, 255))
-			free_subsys_list_item(&slist[*subcnt]);
-		else
+		} else {
 			(*subcnt)++;
+		}
 	}
 
+	*pslist = slist;
 free_subsys:
 	for (i = 0; i < n; i++)
 		free(subsys[i]);
 	free(subsys);
 
-	return slist;
+	return ret;
 }
 
 static int list_subsys(int argc, char **argv, struct command *cmd,
@@ -1679,15 +1596,12 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
 {
 	struct subsys_list_item *slist;
 	int fmt, ret, subcnt = 0;
-	char *subsysnqn = NULL;
-	const char *desc = "Retrieve information for subsystems";
+	const char *desc = "Retrieve information for all NVMe subsystems.";
 	struct config {
-		__u32 namespace_id;
 		char *output_format;
 	};
 
 	struct config cfg = {
-		.namespace_id  = NVME_NSID_ALL,
 		.output_format = "normal",
 	};
 
@@ -1701,42 +1615,13 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
 	if (ret < 0)
 		return ret;
 
-	devicename = NULL;
-	if (optind < argc) {
-		char path[512];
-		int id;
-
-		devicename = basename(argv[optind]);
-		if (sscanf(devicename, "nvme%dn%d", &id,
-			   &cfg.namespace_id) != 2) {
-			fprintf(stderr, "%s is not a NVMe namespace device\n",
-				argv[optind]);
-			return -EINVAL;
-		}
-		sprintf(path, "/sys/block/%s/device", devicename);
-		subsysnqn = get_nvme_subsnqn(path);
-		if (!subsysnqn) {
-			fprintf(stderr, "Cannot read subsys NQN from %s\n",
-				devicename);
-			return -EINVAL;
-		}
-		optind++;
-	}
-
-	if (ret < 0) {
-		argconfig_print_help(desc, opts);
-		if (subsysnqn)
-			free(subsysnqn);
-		return ret;
-	}
 	fmt = validate_output_format(cfg.output_format);
-	if (fmt != JSON && fmt != NORMAL) {
-		if (subsysnqn)
-			free(subsysnqn);
+	if (fmt != JSON && fmt != NORMAL)
 		return -EINVAL;
-	}
 
-	slist = get_subsys_list(&subcnt, subsysnqn, cfg.namespace_id);
+	ret = get_subsys_list(&subcnt, &slist);
+	if (ret)
+		return ret;
 
 	if (fmt == JSON)
 		json_print_nvme_subsystem_list(slist, subcnt);
@@ -1744,8 +1629,6 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
 		show_nvme_subsystem_list(slist, subcnt);
 
 	free_subsys_list(slist, subcnt);
-	if (subsysnqn)
-		free(subsysnqn);
 	return ret;
 }
 
diff --git a/nvme.h b/nvme.h
index a149005..7c444a4 100644
--- a/nvme.h
+++ b/nvme.h
@@ -147,7 +147,6 @@ struct ctrl_list_item {
 	char *address;
 	char *transport;
 	char *state;
-	char *ana_state;
 };
 
 struct subsys_list_item {
@@ -174,7 +173,7 @@ extern const char *devicename;
 int __id_ctrl(int argc, char **argv, struct command *cmd, struct plugin *plugin, void (*vs)(__u8 *vs, struct json_object *root));
 int	validate_output_format(char *format);
 
-struct subsys_list_item *get_subsys_list(int *subcnt, char *subsysnqn, __u32 nsid);
+int get_subsys_list(int *subcnt, struct subsys_list_item **pslist);
 void free_subsys_list(struct subsys_list_item *slist, int n);
 char *nvme_char_from_block(char *block);
 
-- 
1.8.3.1

[PATCH 7/9] nvme-print: Introduce show_nvme_ctrl helper

[Max Gurtovoy]

This is a preparation patch for adding namespaces attribute for each
controller.

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme-print.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/nvme-print.c b/nvme-print.c
index 7316117..7e1f9ea 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2843,6 +2843,13 @@ void json_sanitize_log(struct nvme_sanitize_log_page *sanitize_log, const char *
 	json_free_object(root);
 }
 
+static void show_nvme_ctrl(struct ctrl_list_item *item)
+{
+	printf(" +- %s %s %s %s\n", item->name, item->transport, item->address,
+	       item->state);
+}
+
+
 static void show_nvme_subsystem(struct subsys_list_item *item)
 {
 	int i;
@@ -2850,13 +2857,8 @@ static void show_nvme_subsystem(struct subsys_list_item *item)
 	printf("%s - NQN=%s\n", item->name, item->subsysnqn);
 	printf("\\\n");
 
-	for (i = 0; i < item->nctrls; i++) {
-		printf(" +- %s %s %s %s\n", item->ctrls[i].name,
-				item->ctrls[i].transport,
-				item->ctrls[i].address,
-				item->ctrls[i].state);
-	}
-
+	for (i = 0; i < item->nctrls; i++)
+		show_nvme_ctrl(&item->ctrls[i]);
 }
 
 void show_nvme_subsystem_list(struct subsys_list_item *slist, int n)
-- 
1.8.3.1

[PATCH 8/9] nvme-print: Rename "Paths" --> "Ctrls" for json output in list-subsys

[Max Gurtovoy]

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme-print.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/nvme-print.c b/nvme-print.c
index 7e1f9ea..b3f08cf 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2904,10 +2904,8 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
 					slist[i].ctrls[j].state);
 			json_array_add_value_object(paths, path_attrs);
 		}
-		if (j) {
-			json_object_add_value_array(subsystem_attrs, "Paths", paths);
-		}
-
+		if (j)
+			json_object_add_value_array(subsystem_attrs, "Ctrls", paths);
 	}
 
 	if (i)
-- 
1.8.3.1

[PATCH 9/9] nvme: Retrieve namespaces during list-subsys cmd

[Max Gurtovoy]

Add an association between subsystems/ctrls/namespaces using
"nvme list-subsys" command. Now this command will show the following
(notice that nvme-subsys4 is a dual controller subsystem that runs a
native multipath configuration. Hidden block devices not presented):

nvme-subsys3 - NQN=nqn.2014.08.org.nvmexpress:80868086CVCQ5234001C400AGN INTEL SSDPEDMW400G4
\
 +- nvme0 pcie 0000:08:00.0 live
 \
  +- nvme3n1
nvme-subsys4 - NQN=testsubsystem_0
\
 +- nvme4 rdma traddr=1.212.99.85 trsvcid=4420 live
 \
  +- nvme4n1
  +- nvme4n2
  +- nvme4n3
 +- nvme5 rdma traddr=1.212.99.85 trsvcid=4420 live
 \
  +- nvme4n1
  +- nvme4n2
  +- nvme4n3

Instead of:
----------------
nvme-subsys3 - NQN=nqn.2014.08.org.nvmexpress:80868086CVCQ5234001C400AGN INTEL SSDPEDMW400G4
\
 +- nvme0 pcie 0000:08:00.0
nvme-subsys4 - NQN=testsubsystem_0
\
 +- nvme4 rdma traddr=1.212.99.85 trsvcid=4420
 +- nvme5 rdma traddr=1.212.99.85 trsvcid=4420

The new json output is (partial):
--------------------------
{
      "Name" : "nvme-subsys4",
      "NQN" : "testsubsystem_0",
      "Ctrls" : [
        {
          "Name" : "nvme4",
          "Transport" : "rdma",
          "Address" : "traddr=1.212.99.85 trsvcid=4420",
          "State" : "live",
          "Namespaces" : [
            {
              "Name" : "nvme4n1"
            },
            {
              "Name" : "nvme4n2"
            },
            {
              "Name" : "nvme4n3"
            }
          ]
        },
        {
          "Name" : "nvme5",
          "Transport" : "rdma",
          "Address" : "traddr=1.212.99.85 trsvcid=4420",
          "State" : "live",
          "Namespaces" : [
            {
              "Name" : "nvme4n1"
            },
            {
              "Name" : "nvme4n2"
            },
            {
              "Name" : "nvme4n3"
            }
          ]
        }
      ]
}

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme-print.c |  23 ++++++++++-
 nvme.c       | 130 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 nvme.h       |   6 +++
 3 files changed, 130 insertions(+), 29 deletions(-)

diff --git a/nvme-print.c b/nvme-print.c
index b3f08cf..7625559 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2845,8 +2845,15 @@ void json_sanitize_log(struct nvme_sanitize_log_page *sanitize_log, const char *
 
 static void show_nvme_ctrl(struct ctrl_list_item *item)
 {
+	int i;
+
 	printf(" +- %s %s %s %s\n", item->name, item->transport, item->address,
 	       item->state);
+	for (i = 0; i < item->ns_num; i++) {
+		if (!i)
+			printf(" \\\n");
+		printf("  +- %s\n", item->namespaces[i].name);
+	}
 }
 
 
@@ -2875,7 +2882,9 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
 	struct json_object *subsystem_attrs;
 	struct json_array *paths;
 	struct json_object *path_attrs;
-	int i, j;
+	struct json_array *namespaces;
+	struct json_object *ns_attrs;
+	int i, j, k;
 
 	root = json_create_object();
 	subsystems = json_create_array();
@@ -2902,6 +2911,18 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
 					slist[i].ctrls[j].address);
 			json_object_add_value_string(path_attrs, "State",
 					slist[i].ctrls[j].state);
+
+			namespaces = json_create_array();
+			for (k = 0; k < slist[i].ctrls[j].ns_num; k++) {
+				ns_attrs = json_create_object();
+				json_object_add_value_string(ns_attrs, "Name",
+							     slist[i].ctrls[j].namespaces[k].name);
+				json_array_add_value_object(namespaces, ns_attrs);
+			}
+			if (k)
+				json_object_add_value_array(path_attrs, "Namespaces",
+							    namespaces);
+
 			json_array_add_value_object(paths, path_attrs);
 		}
 		if (j)
diff --git a/nvme.c b/nvme.c
index 518a396..ef01ead 100644
--- a/nvme.c
+++ b/nvme.c
@@ -60,6 +60,9 @@ const char *devicename;
 
 static const char nvme_version_string[] = NVME_VERSION;
 
+static void free_ctrl_list_item(struct ctrl_list_item *ctrls);
+static char *get_nvme_ctrl_attr(char *path, const char *attr);
+
 #define CREATE_CMD
 #include "nvme-builtin.h"
 
@@ -1313,6 +1316,82 @@ static void *get_registers(void)
 
 static const char *subsys_dir = "/sys/class/nvme-subsystem/";
 
+static int scan_ns_filter(const struct dirent *d)
+{
+	int id, nsid;
+
+	if (strstr(d->d_name, "nvme")) {
+		if (sscanf(d->d_name, "nvme%dn%d", &id, &nsid) == 2)
+			return 1;
+	}
+
+	return 0;
+}
+
+static int get_nvme_ctrl_info(char *name, char *ctrl_path,
+			      struct ctrl_list_item *item, int heads_num,
+			      struct dirent **heads)
+{
+	struct dirent **namespaces;
+	int ret = 0, n, i;
+
+	item->name = strdup(name);
+
+	if (heads_num) {
+		n = heads_num;
+		namespaces = heads;
+	} else {
+		n = scandir(ctrl_path, &namespaces, scan_ns_filter, alphasort);
+		if (n < 0) {
+			fprintf(stderr, "failed to scan namespace(s).\n");
+			return n;
+		}
+	}
+
+	item->namespaces = calloc(n, sizeof(struct ns_list_item));
+	if (!item->namespaces) {
+		ret = -ENOMEM;
+		fprintf(stderr, "failed to allocate controller namespace(s)\n");
+		goto free_namespaces;
+	}
+
+	item->ns_num = n;
+
+	for (i = 0; i < n; i++)
+		item->namespaces[i].name = strdup(namespaces[i]->d_name);
+
+	item->address = get_nvme_ctrl_attr(ctrl_path, "address");
+	if (!item->address) {
+		fprintf(stderr, "failed to get %s address.\n", name);
+		free_ctrl_list_item(item);
+		ret = -EAGAIN;
+		goto free_namespaces;
+	}
+	item->transport = get_nvme_ctrl_attr(ctrl_path, "transport");
+	if (!item->transport) {
+		fprintf(stderr, "failed to get %s transport.\n", name);
+		free_ctrl_list_item(item);
+		ret = -EAGAIN;
+		goto free_namespaces;
+	}
+	item->state = get_nvme_ctrl_attr(ctrl_path, "state");
+	if (!item->state) {
+		fprintf(stderr, "failed to get %s state.\n", name);
+		free_ctrl_list_item(item);
+		ret = -EAGAIN;
+		goto free_namespaces;
+	}
+
+free_namespaces:
+	if (!heads) {
+		for (i = 0; i < n; i++)
+			free(namespaces[i]);
+		free(namespaces);
+	}
+
+	return ret;
+}
+
 static char *get_nvme_subsnqn(char *path)
 {
 	char sspath[320];
@@ -1419,10 +1498,15 @@ static int scan_ctrls_filter(const struct dirent *d)
 
 static void free_ctrl_list_item(struct ctrl_list_item *ctrls)
 {
+	int i;
+
 	free(ctrls->name);
 	free(ctrls->transport);
 	free(ctrls->address);
 	free(ctrls->state);
+	for (i = 0; i < ctrls->ns_num; i++)
+		free(ctrls->namespaces[i].name);
+	free(ctrls->namespaces);
 }
 
 static int get_nvme_subsystem_info(char *name, char *path,
@@ -1430,7 +1514,8 @@ static int get_nvme_subsystem_info(char *name, char *path,
 {
 	char ctrl_path[512];
 	struct dirent **ctrls;
-	int n, i, ret = 1, ccnt = 0;
+	struct dirent **heads;
+	int n, heads_num, i, ret = 1, ccnt = 0;
 
 	item->subsysnqn = get_nvme_subsnqn(path);
 	if (!item->subsysnqn) {
@@ -1454,43 +1539,32 @@ static int get_nvme_subsystem_info(char *name, char *path,
 
 	item->nctrls = n;
 
+	heads_num = scandir(path, &heads, scan_ns_filter, alphasort);
+	if (heads_num < 0) {
+		fprintf(stderr, "failed to scan heads(s).\n");
+		goto free_ctrls;
+	}
+
 	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;
-		}
+			 ctrls[i]->d_name);
 
-		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]);
-			continue;
-		}
-
-		ccnt++;
+		ret = get_nvme_ctrl_info(ctrls[i]->d_name, ctrl_path,
+					 &(item->ctrls[ccnt]), heads_num, heads);
+		if (!ret)
+			ccnt++;
 	}
 
 	item->nctrls = ccnt;
 
 	ret = 0;
 
+	if (heads_num) {
+		for (i = 0; i < heads_num; i++)
+			free(heads[i]);
+		free(heads);
+	}
 free_ctrls:
 	for (i = 0; i < n; i++)
 		free(ctrls[i]);
diff --git a/nvme.h b/nvme.h
index 7c444a4..84eaa9f 100644
--- a/nvme.h
+++ b/nvme.h
@@ -142,11 +142,17 @@ struct list_item {
 	unsigned            block;
 };
 
+struct ns_list_item {
+	char *name;
+};
+
 struct ctrl_list_item {
 	char *name;
 	char *address;
 	char *transport;
 	char *state;
+	int ns_num;
+	struct ns_list_item *namespaces;
 };
 
 struct subsys_list_item {
-- 
1.8.3.1

[PATCH 6/9] nvme: update list-subsys command to show the entire list

[Sagi Grimberg]

> Make it similar to "nvme list" command. For that, remove the
> unnecessary ana_state parsing (was introduced only if specific namespace
> handle was given).

No, this is useful! You're arbitrarily removing useful information from
the display...

nak from me on this one...

[PATCH 9/9] nvme: Retrieve namespaces during list-subsys cmd

[Sagi Grimberg]

> Add an association between subsystems/ctrls/namespaces using
> "nvme list-subsys" command. Now this command will show the following
> (notice that nvme-subsys4 is a dual controller subsystem that runs a
> native multipath configuration. Hidden block devices not presented):

I'm having a problem with the removal of the ANA state and the namespace
handle removal.

If you insist of having this display, please introduce it in a separate
command, don't just change how list-subsys acts (which is actually kinda
useful).

> nvme-subsys3 - NQN=nqn.2014.08.org.nvmexpress:80868086CVCQ5234001C400AGN INTEL SSDPEDMW400G4
> \
>   +- nvme0 pcie 0000:08:00.0 live
>   \
>    +- nvme3n1
> nvme-subsys4 - NQN=testsubsystem_0
> \
>   +- nvme4 rdma traddr=1.212.99.85 trsvcid=4420 live
>   \
>    +- nvme4n1
>    +- nvme4n2
>    +- nvme4n3
>   +- nvme5 rdma traddr=1.212.99.85 trsvcid=4420 live
>   \
>    +- nvme4n1
>    +- nvme4n2
>    +- nvme4n3

This is definitely better than the hidden devices though...

[PATCH 1/9] nvme: update list-ns nsid option

[Sagi Grimberg]

Is this v4 btw?

[PATCH 1/9] nvme: update list-ns nsid option

[Max Gurtovoy]

On 5/28/2019 10:54 PM, Sagi Grimberg wrote:
> Is this v4 btw?


I think it's V3 :)

Can you merge patches 1-5 meanwhile or should I send them separately ?

We can continue discussing regarding the other patches..

[PATCH 6/9] nvme: update list-subsys command to show the entire list

[Max Gurtovoy]

On 5/28/2019 10:48 PM, Sagi Grimberg wrote:
>
>> Make it similar to "nvme list" command. For that, remove the
>> unnecessary ana_state parsing (was introduced only if specific namespace
>> handle was given).
>
> No, this is useful! You're arbitrarily removing useful information from
> the display...
>
> nak from me on this one...

I intended to add it to different command (maybe to new "nvme multipath" 
command you suggested).

I don't understand why ANA state is so important attribute for a user 
that only asked "nvme list-subsys".

One can always run "nvme ana-log" command to get a full log page (I'll 
take a look on it and see if it can be improved per given namespace)

[PATCH 6/9] nvme: update list-subsys command to show the entire list

[Sagi Grimberg]

>>> Make it similar to "nvme list" command. For that, remove the
>>> unnecessary ana_state parsing (was introduced only if specific namespace
>>> handle was given).
>>
>> No, this is useful! You're arbitrarily removing useful information from
>> the display...
>>
>> nak from me on this one...
> 
> I intended to add it to different command (maybe to new "nvme multipath" 
> command you suggested).

I think it should be the other way around, we already have an interface
which is useful, if we add information it should either be exposed via
an argument or a different command.

> I don't understand why ANA state is so important attribute for a user 
> that only asked "nvme list-subsys".

It gives you the layout of the subsystem with respect to the volume you
asked for.

> One can always run "nvme ana-log" command to get a full log page (I'll 
> take a look on it and see if it can be improved per given namespace)

This is the reverse info (controller view), this is the
subsystem/namespace view.

[PATCH 1/9] nvme: update list-ns nsid option

[Max Gurtovoy]

On 5/23/2019 8:27 PM, Heitke, Kenneth wrote:
> Would it be useful to include an error message here? I don't like
> playing the guessing game about what I did wrong when I just get the
> help message back. 

Yup good idea. I'll fix that in V3.

-Max.

[PATCH 1/9] nvme: update list-ns nsid option

[Heitke, Kenneth]

On 5/23/2019 3:00 AM, Max Gurtovoy wrote:
> This commit updates the optional nsid argument to define the wanted
> nsid for start, instead of starting from nsid + 1. E.g. in case we've
> wanted to get the list of namespaces starting from 1, before this
> commit, we used the "--namespace-id=0" option. Nsid 0 is not valid in
> NVMe spec, thus change it to start counting from the given nsid.
> 
> Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
> Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
> ---
>   nvme.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/nvme.c b/nvme.c
> index 9819fcb..3c1bcb1 100644
> --- a/nvme.c
> +++ b/nvme.c
> @@ -950,9 +950,9 @@ close_fd:
>   
>   static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *plugin)
>   {
> -	const char *desc = "For the specified device, show the "\
> -		"namespace list in a NVMe subsystem, optionally starting with a given namespace";
> -	const char *namespace_id = "namespace number returned list should to start after";
> +	const char *desc = "For the specified controller handle, show the "\
> +		"namespace list in the associated NVMe subsystem, optionally starting with a given nsid.";
> +	const char *namespace_id = "first nsid returned list should start from";
>   	const char *all = "show all namespaces in the subsystem, whether attached or inactive";
>   	int err, i, fd;
>   	__u32 ns_list[1024];
> @@ -963,7 +963,7 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
>   	};
>   
>   	struct config cfg = {
> -		.namespace_id = 0,
> +		.namespace_id = 1,
>   	};
>   
>   	const struct argconfig_commandline_options command_line_options[] = {
> @@ -976,7 +976,11 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
>   	if (fd < 0)
>   		return fd;
>   
> -	err = nvme_identify_ns_list(fd, cfg.namespace_id, !!cfg.all, ns_list);
> +	if (!cfg.namespace_id)
> +		return -EINVAL;

Would it be useful to include an error message here? I don't like
playing the guessing game about what I did wrong when I just get the
help message back.

> +
> +	err = nvme_identify_ns_list(fd, cfg.namespace_id - 1, !!cfg.all,
> +				    ns_list);
>   	if (!err) {
>   		for (i = 0; i < 1024; i++)
>   			if (ns_list[i])
> 

[PATCH 1/9] nvme: update list-ns nsid option

[Max Gurtovoy]

This commit updates the optional nsid argument to define the wanted
nsid for start, instead of starting from nsid + 1. E.g. in case we've
wanted to get the list of namespaces starting from 1, before this
commit, we used the "--namespace-id=0" option. Nsid 0 is not valid in
NVMe spec, thus change it to start counting from the given nsid.

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>
Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/nvme.c b/nvme.c
index 9819fcb..3c1bcb1 100644
--- a/nvme.c
+++ b/nvme.c
@@ -950,9 +950,9 @@ close_fd:
 
 static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *plugin)
 {
-	const char *desc = "For the specified device, show the "\
-		"namespace list in a NVMe subsystem, optionally starting with a given namespace";
-	const char *namespace_id = "namespace number returned list should to start after";
+	const char *desc = "For the specified controller handle, show the "\
+		"namespace list in the associated NVMe subsystem, optionally starting with a given nsid.";
+	const char *namespace_id = "first nsid returned list should start from";
 	const char *all = "show all namespaces in the subsystem, whether attached or inactive";
 	int err, i, fd;
 	__u32 ns_list[1024];
@@ -963,7 +963,7 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	};
 
 	struct config cfg = {
-		.namespace_id = 0,
+		.namespace_id = 1,
 	};
 
 	const struct argconfig_commandline_options command_line_options[] = {
@@ -976,7 +976,11 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	if (fd < 0)
 		return fd;
 
-	err = nvme_identify_ns_list(fd, cfg.namespace_id, !!cfg.all, ns_list);
+	if (!cfg.namespace_id)
+		return -EINVAL;
+
+	err = nvme_identify_ns_list(fd, cfg.namespace_id - 1, !!cfg.all,
+				    ns_list);
 	if (!err) {
 		for (i = 0; i < 1024; i++)
 			if (ns_list[i])
-- 
1.8.3.1

[PATCH 1/9] nvme: update list-ns nsid option

[Minwoo Im]

> Subject: [PATCH 1/9] nvme: update list-ns nsid option
> 
> This commit updates the optional nsid argument to define the wanted
> nsid for start, instead of starting from nsid + 1. E.g. in case we've
> wanted to get the list of namespaces starting from 1, before this
> commit, we used the "--namespace-id=0" option. Nsid 0 is not valid in
> NVMe spec, thus change it to start counting from the given nsid.
> 
> Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
> ---
>  nvme.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/nvme.c b/nvme.c
> index 9819fcb..de77956 100644
> --- a/nvme.c
> +++ b/nvme.c
> @@ -950,9 +950,9 @@ close_fd:
> 
>  static int list_ns(int argc, char **argv, struct command *cmd, struct plugin
> *plugin)
>  {
> -	const char *desc = "For the specified device, show the "\
> -		"namespace list in a NVMe subsystem, optionally starting
> with a given namespace";
> -	const char *namespace_id = "namespace number returned list
> should to start after";
> +	const char *desc = "For the specified controller handle, show the "\
> +		"namespace list in the associated NVMe subsystem,
> optionally starting with a given nsid.";
> +	const char *namespace_id = "first nsid returned list should start
> from";
>  	const char *all = "show all namespaces in the subsystem, whether
> attached or inactive";
>  	int err, i, fd;
>  	__u32 ns_list[1024];
> @@ -963,7 +963,7 @@ static int list_ns(int argc, char **argv, struct command
> *cmd, struct plugin *pl
>  	};
> 
>  	struct config cfg = {
> -		.namespace_id = 0,
> +		.namespace_id = 1,
>  	};
> 
>  	const struct argconfig_commandline_options
> command_line_options[] = {
> @@ -976,7 +976,8 @@ static int list_ns(int argc, char **argv, struct command
> *cmd, struct plugin *pl
>  	if (fd < 0)
>  		return fd;
> 

Can we have invalid --namespace-id check here?

if (!cfg.namespace_id)
	return -EINVAL;

Otherwise, It looks good to me.

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

> -	err = nvme_identify_ns_list(fd, cfg.namespace_id, !!cfg.all, ns_list);
> +	err = nvme_identify_ns_list(fd, cfg.namespace_id - 1, !!cfg.all,
> +				    ns_list);
>  	if (!err) {
>  		for (i = 0; i < 1024; i++)
>  			if (ns_list[i])
> --
> 1.8.3.1
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/9] nvme: update list-ns nsid option

[Max Gurtovoy]

This commit updates the optional nsid argument to define the wanted
nsid for start, instead of starting from nsid + 1. E.g. in case we've
wanted to get the list of namespaces starting from 1, before this
commit, we used the "--namespace-id=0" option. Nsid 0 is not valid in
NVMe spec, thus change it to start counting from the given nsid.

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 nvme.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/nvme.c b/nvme.c
index 9819fcb..de77956 100644
--- a/nvme.c
+++ b/nvme.c
@@ -950,9 +950,9 @@ close_fd:
 
 static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *plugin)
 {
-	const char *desc = "For the specified device, show the "\
-		"namespace list in a NVMe subsystem, optionally starting with a given namespace";
-	const char *namespace_id = "namespace number returned list should to start after";
+	const char *desc = "For the specified controller handle, show the "\
+		"namespace list in the associated NVMe subsystem, optionally starting with a given nsid.";
+	const char *namespace_id = "first nsid returned list should start from";
 	const char *all = "show all namespaces in the subsystem, whether attached or inactive";
 	int err, i, fd;
 	__u32 ns_list[1024];
@@ -963,7 +963,7 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	};
 
 	struct config cfg = {
-		.namespace_id = 0,
+		.namespace_id = 1,
 	};
 
 	const struct argconfig_commandline_options command_line_options[] = {
@@ -976,7 +976,8 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 	if (fd < 0)
 		return fd;
 
-	err = nvme_identify_ns_list(fd, cfg.namespace_id, !!cfg.all, ns_list);
+	err = nvme_identify_ns_list(fd, cfg.namespace_id - 1, !!cfg.all,
+				    ns_list);
 	if (!err) {
 		for (i = 0; i < 1024; i++)
 			if (ns_list[i])
-- 
1.8.3.1