[PATCH 0/9] Add Subsystem/Ctrl/Namespace relations

[PATCH 0/9] Add Subsystem/Ctrl/Namespace relations

[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...).

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 |  51 +++++++----
 nvme.c       | 274 +++++++++++++++++++++++------------------------------------
 nvme.h       |   9 +-
 4 files changed, 150 insertions(+), 190 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.

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

[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.

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 de77956..7cf7225 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1794,7 +1794,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]

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]

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

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 7cf7225..62f01c8 100644
--- a/nvme.c
+++ b/nvme.c
@@ -982,11 +982,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);
 
-- 
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 62f01c8..0fa141b 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1394,85 +1394,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;
@@ -1495,11 +1416,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;
@@ -1557,9 +1477,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++;
 	}
 
@@ -1624,8 +1541,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;
@@ -1635,36 +1551,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,
@@ -1672,15 +1589,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",
 	};
 
@@ -1694,42 +1608,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);
@@ -1737,8 +1622,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.

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:

nvme-subsys4 - NQN=testsubsystem_0
\
 +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420 live
 \
  +- nvme4n1
  +- nvme4n2
  +- nvme4n3
 +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420 live
 \
  +- nvme5n1
  +- nvme5n2
  +- nvme5n3

Instead of:
----------------
nvme-subsys4 - NQN=testsubsystem_0
\
 +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420
 +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420

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

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

diff --git a/nvme-print.c b/nvme-print.c
index b3f08cf..e578d0b 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2845,8 +2845,14 @@ 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);
+	printf(" \\\n");
+	for (i = 0; i < item->ns_num; i++) {
+		printf("  +- %s\n", item->namespaces[i].name);
+	}
 }
 
 
@@ -2875,7 +2881,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 +2910,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 0fa141b..08d0cc2 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"
 
@@ -1306,6 +1309,72 @@ 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") &&
+	    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)
+{
+	struct dirent **namespaces;
+	int ret = 0, n, i;
+
+	item->name = strdup(name);
+
+	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:
+	for (i = 0; i < n; i++)
+		free(namespaces[i]);
+	free(namespaces);
+
+	return ret;
+}
+
 static char *get_nvme_subsnqn(char *path)
 {
 	char sspath[320];
@@ -1412,10 +1481,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,
@@ -1448,36 +1522,14 @@ 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]);
-			continue;
-		}
+			 ctrls[i]->d_name);
 
-		ccnt++;
+		ret = get_nvme_ctrl_info(ctrls[i]->d_name, ctrl_path,
+					 &(item->ctrls[ccnt]));
+		if (!ret)
+			ccnt++;
 	}
 
 	item->nctrls = ccnt;
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 9/9] nvme: Retrieve namespaces during list-subsys cmd

[Keith Busch]

On Wed, May 22, 2019@07:39:53PM +0300, Max Gurtovoy wrote:
> Add an association between subsystems/ctrls/namespaces using
> "nvme list-subsys" command. Now this command will show the following:
> 
> nvme-subsys4 - NQN=testsubsystem_0
> \
>  +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420 live
>  \
>   +- nvme4n1
>   +- nvme4n2
>   +- nvme4n3
>  +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420 live
>  \
>   +- nvme5n1
>   +- nvme5n2
>   +- nvme5n3
> 
> Instead of:
> ----------------
> nvme-subsys4 - NQN=testsubsystem_0
> \
>  +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420
>  +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420

This is a terrific start. Your output indicates you are not using
nvme native multipathing, though. Could you retry this command with
that enabled (I'll try it tomorrow as well)?

[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 2/9] nvme: update description for "nvme list" command

[Minwoo Im]

This looks good to me.

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

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

[Minwoo Im]

This looks good to me.

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

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

[Minwoo Im]

This looks good to me.

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

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

[Minwoo Im]

This looks good to me.

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

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

[Max Gurtovoy]

On 5/23/2019 1:49 AM, Keith Busch wrote:
> On Wed, May 22, 2019@07:39:53PM +0300, Max Gurtovoy wrote:
>> Add an association between subsystems/ctrls/namespaces using
>> "nvme list-subsys" command. Now this command will show the following:
>>
>> nvme-subsys4 - NQN=testsubsystem_0
>> \
>>   +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420 live
>>   \
>>    +- nvme4n1
>>    +- nvme4n2
>>    +- nvme4n3
>>   +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420 live
>>   \
>>    +- nvme5n1
>>    +- nvme5n2
>>    +- nvme5n3
>>
>> Instead of:
>> ----------------
>> nvme-subsys4 - NQN=testsubsystem_0
>> \
>>   +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420
>>   +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420
> This is a terrific start. Your output indicates you are not using
> nvme native multipathing, though. Could you retry this command with
> that enabled (I'll try it tomorrow as well)?

Yup I'll configure the native multipath. But what is expected ?

should we print nvme4c1n1/nvme4c1n2/nvme4c1n3 for nvme4

and nvme4c2n1/nvme4c2n2/nvme4c2n3 for nvme5 ?

if so, I guess the information on the "IO-ble" devices 
nvme4n1/nvme4n2/nvme4n3 will not be printed.

something like:

nvme-subsys4 - NQN=testsubsystem_0
\
 ?+- nvme4 rdma traddr=12.212.99.85 trsvcid=4420 live
 ?\
 ? +- nvme4c1n1
 ? +- nvme4c1n2
 ? +- nvme4c1n3
 ?+- nvme5 rdma traddr=12.212.99.85 trsvcid=4420 live
 ?\
 ? +- nvme4c2n1
 ? +- nvme4c2n2
 ? +- nvme4c2n3


if this if fine I'll send a V2 with a small fix.

-Max.

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

[Keith Busch]

On Thu, May 23, 2019@01:01:54AM -0700, Max Gurtovoy wrote:
> 
> On 5/23/2019 1:49 AM, Keith Busch wrote:
> > On Wed, May 22, 2019@07:39:53PM +0300, Max Gurtovoy wrote:
> >> Add an association between subsystems/ctrls/namespaces using
> >> "nvme list-subsys" command. Now this command will show the following:
> >>
> >> nvme-subsys4 - NQN=testsubsystem_0
> >> \
> >>   +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420 live
> >>   \
> >>    +- nvme4n1
> >>    +- nvme4n2
> >>    +- nvme4n3
> >>   +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420 live
> >>   \
> >>    +- nvme5n1
> >>    +- nvme5n2
> >>    +- nvme5n3
> >>
> >> Instead of:
> >> ----------------
> >> nvme-subsys4 - NQN=testsubsystem_0
> >> \
> >>   +- nvme4 rdma traddr=12.212.99.85 trsvcid=4420
> >>   +- nvme5 rdma traddr=12.212.99.85 trsvcid=4420
> > This is a terrific start. Your output indicates you are not using
> > nvme native multipathing, though. Could you retry this command with
> > that enabled (I'll try it tomorrow as well)?
> 
> Yup I'll configure the native multipath. But what is expected ?
> 
> should we print nvme4c1n1/nvme4c1n2/nvme4c1n3 for nvme4
> 
> and nvme4c2n1/nvme4c2n2/nvme4c2n3 for nvme5 ?
> 
> if so, I guess the information on the "IO-ble" devices 
> nvme4n1/nvme4n2/nvme4n3 will not be printed.
> 
> something like:
> 
> nvme-subsys4 - NQN=testsubsystem_0
> \
>  ?+- nvme4 rdma traddr=12.212.99.85 trsvcid=4420 live
>  ?\
>  ? +- nvme4c1n1
>  ? +- nvme4c1n2
>  ? +- nvme4c1n3
>  ?+- nvme5 rdma traddr=12.212.99.85 trsvcid=4420 live
>  ?\
>  ? +- nvme4c2n1
>  ? +- nvme4c2n2
>  ? +- nvme4c2n3
> 
> 
> if this if fine I'll send a V2 with a small fix.

I think we'd actually prefer not seeing the hidden block devices, but
instead use the device name from the "head": nvme4n1, nvme4n2, etc...

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

[hch]

On Thu, May 23, 2019@07:39:46AM -0600, Keith Busch wrote:
> > if this if fine I'll send a V2 with a small fix.
> 
> I think we'd actually prefer not seeing the hidden block devices, but
> instead use the device name from the "head": nvme4n1, nvme4n2, etc...

Agreed.  The hidden devices are an artefact we should expose to the
user as little as possible.

[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 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