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

[PATCH 1/5] 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/5] 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/5] 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 573a6ef..9ed4a56 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1205,5 +1205,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/5] 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/5] 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 1/5] nvme: update list-ns nsid option

[Sagi Grimberg]

Next time please add nvme-cli prefix to the patch set

For the series:
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

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

[Chaitanya Kulkarni]

This patch overall looks good go me, except one nit.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

On 5/30/19 2:43 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 | 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);
nit:- Can we get rid of the new line for the last arg in above call ?
Can be fixed at the commit time.
>  	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;

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

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

On 5/30/19 2:44 AM, Max Gurtovoy wrote:
> 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;
>  	};

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

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
On 5/30/19 2:44 AM, Max Gurtovoy wrote:
> 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 573a6ef..9ed4a56 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -1205,5 +1205,6 @@ int disconnect_all(const char *desc, int argc, char **argv)
>  		}
>  	}
>  out:
> +	free_subsys_list(slist, subcnt);
>  	return ret;
>  }

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

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

On 5/30/19 2:44 AM, Max Gurtovoy wrote:
> 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)

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

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
On 5/30/19 2:44 AM, Max Gurtovoy wrote:
> 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);

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

[Max Gurtovoy]

On 6/3/2019 7:25 PM, Chaitanya Kulkarni wrote:
> This patch overall looks good go me, except one nit.
>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
>
> On 5/30/19 2:43 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 | 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);
> nit:- Can we get rid of the new line for the last arg in above call ?
> Can be fixed at the commit time.

I did it to avoid getting more that 80 char line.

I think the maintainer can do it if it's necessary during commit time.. 
(or I can do it also. both work for me)


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

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

[Chaitanya Kulkarni]

On 6/4/19 1:57 AM, Max Gurtovoy wrote:
> On 6/3/2019 7:25 PM, Chaitanya Kulkarni wrote:
>> This patch overall looks good go me, except one nit.
>>
>> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
>>
>> On 5/30/19 2:43 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 | 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);
>> nit:- Can we get rid of the new line for the last arg in above call ?
>> Can be fixed at the commit time.
> I did it to avoid getting more that 80 char line.
>
> I think the maintainer can do it if it's necessary during commit time.. 
> (or I can do it also. both work for me)
Let's leave it to Keith.
>
>>>   	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;

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

[Keith Busch]

I've applied the entire series. Thanks, Max!

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

[Max Gurtovoy]

On 6/7/2019 7:19 PM, Keith Busch wrote:
> I've applied the entire series. Thanks, Max!

Great.

Now we need to decide how to proceed with the original purpose of 
improving the "nvme list-subsys" cmd..