Skip to contentSkip to conversation

nvme-cli: Fix to the logic of the nvme list-ns <device> -a command.

PATCH New Chaitanya Kulkarni 5 participants 5 messages Jul 9, 2017 – Jul 10, 2017 NVMe

lore
Share & tools
lore

Chaitanya Kulkarni

author
#0 lore raw

PATCH 1/1 nvme-cli: Fix to the logic of the nvme list-ns <device> -a command.

This fix will reverse the logic of the -a parm in the nvme list-ns command. Currently, the -a option only shows the attached namespaces and not all the namespaces. By removing the extra ! it will show attached inactive namespaces with -a option as mentioned in the help text.

Example : Controller has 3 namespaces, two of them are not attached.

Default exection with ?!!? :-

$ nvme list-ns  /dev/nvme0 -a
[   0]:0x1

Execution with ?!? :-
nvme list-ns /dev/nvme0 -a

[   0]:0x1
[   1]:0x2
[   2]:0x3
Signed-off-by Chaitanya Kulkarni <chaitanya.kulkarni at hgst.com>
---
 nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
nvme.c +1 -1
index d64d042..a3abff0 100644
@@ -560,7 +560,7 @@ 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, !cfg.all, ns_list);
 	if (!err) {
 		for (i = 0; i < 1024; i++)
 			if (ns_list[i])
Signature · 1 line
2.7.4
maintainer
#1 lore raw

PATCH 1/1 nvme-cli: Fix to the logic of the nvme list-ns <device> -a command.

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
#2 lore raw

PATCH 1/1 nvme-cli: Fix to the logic of the nvme list-ns <device> -a command.

In reply to Chaitanya Kulkarni

Hi,Chaitanya
Thanks for your effort.

Quoted 5 lines · Chaitanya Kulkarni

This fix will reverse the logic of the -a parm in the nvme list-ns command. Currently, the -a option only shows the attached namespaces and not all the namespaces. By removing the extra ! it will show attached inactive namespaces with -a option as mentioned in the help text.

Actually, the approach of reversing the logic of the -a option doesn't keep the consistence of literal input parameter __bool__ __all__ of the nvme_identify_ns_list function.

By the way ,I had sent a patch to fix this two month ago but Keith missed it. A new updated patch of this was sent a minute ago. Hope Keith would catch it. (The origin link is: http://lists.infradead.org/pipermail/linux-nvme/2017-April/009805.html
)

Thanks
.

#3 lore raw

PATCH 1/1 nvme-cli: Fix to the logic of the nvme list-ns <device> -a command.

With this approach
(http://lists.infradead.org/pipermail/linux-nvme/2017-April/009805.html)
all the future caller's including any plug-ins will require writing a code with "!!".

I wanted to get rid of "!!" approach which makes the code simpler for everyone at first glance, only if everyone agrees with that change.

Quoted 26 lines · Guan Junxiong

Hi,Chaitanya
Thanks for your effort.

Quoted 5 lines · Chaitanya Kulkarni

This fix will reverse the logic of the -a parm in the nvme list-ns command. Currently, the -a option only shows the attached namespaces and not all the namespaces. By removing the extra ! it will show attached inactive namespaces with -a option as mentioned in the help text.

Actually, the approach of reversing the logic of the -a option doesn't keep the consistence of literal input parameter __bool__ __all__ of the nvme_identify_ns_list function.

By the way ,I had sent a patch to fix this two month ago but Keith missed it. A new updated patch of this was sent a minute ago. Hope Keith would catch it. (The origin link is: http://lists.infradead.org/pipermail/linux-nvme/2017-April/009805.html
)

Thanks
.

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

#4 lore raw

PATCH 1/1 nvme-cli: Fix to the logic of the nvme list-ns <device> -a command.

Quoted 4 lines · royy walls

With this approach
(http://lists.infradead.org/pipermail/linux-nvme/2017-April/009805.html)
all the future caller's including any plug-ins will require writing a code with "!!".

The use of '!!' was to explicitly coerce the 'int' type required for the arg parsing to a 'true/false' value for the bool type. It's not really necessary, though.

Quoted 21 lines

I wanted to get rid of "!!" approach which makes the code simpler for everyone at first glance, only if everyone agrees with that change.

Quoted 17 lines · Guan Junxiong

Hi,Chaitanya
Thanks for your effort.

Quoted 5 lines · Chaitanya Kulkarni

This fix will reverse the logic of the -a parm in the nvme list-ns command. Currently, the -a option only shows the attached namespaces and not all the namespaces. By removing the extra ! it will show attached inactive namespaces with -a option as mentioned in the help text.

Actually, the approach of reversing the logic of the -a option doesn't keep the consistence of literal input parameter __bool__ __all__ of the nvme_identify_ns_list function.

By the way ,I had sent a patch to fix this two month ago but Keith missed it. A new updated patch of this was sent a minute ago. Hope Keith would catch it. (The origin link is: http://lists.infradead.org/pipermail/linux-nvme/2017-April/009805.html
)

Sorry I missed that patch. That one looks correct to me, and thank you for reposting the v2. I think we'll use that one.