Skip to content

Remove list command.#42

Closed
pmmccorm wants to merge 1 commit intolinux-nvme:masterfrom
pmmccorm:master
Closed

Remove list command.#42
pmmccorm wants to merge 1 commit intolinux-nvme:masterfrom
pmmccorm:master

Conversation

@pmmccorm
Copy link
Contributor

@pmmccorm pmmccorm commented Dec 9, 2015

Completely remove libudev dependency from build, remove documentation,
and move register reading into common code.

lsnvme handles this better and it keeps nvme-cli ioctly only.

Signed-off-by: Patrick McCormick patrick.m.mccormick@intel.com

Completely remove libudev dependency from build, remove documentation,
and move register reading into common code.

lsnvme handles this better and it keeps nvme-cli ioctly only.

Signed-off-by: Patrick McCormick <patrick.m.mccormick@intel.com>
@samiWaheed
Copy link
Contributor

What is the reason for removing the list command? I suggest keeping it and moving any future enhancements to the lsnvme utility.
I do have some infrastructure and automaton built around the list command. I am sure others do as well.

@sbates130272
Copy link
Contributor

Patrick

Agree with Sami. We should have both nvme list ans lsnvme. A lot of people are already using nvme list so removing it will be very disruptive. We can offline discuss how to handle the case of fabric attached controllers in this command.

Stephen

@keithbusch
Copy link
Contributor

I think it'd be better to route "list" to leverage "lsnvme" instead of removing the sub-command.

Closing based on comments.

@keithbusch keithbusch closed this Dec 9, 2015
@sbates130272
Copy link
Contributor

I concur. Patrick can you do another PR based on this. Need to ensure the output of nvme list is not altered as people are parsing that in their test environments.

Stephen

@pmmccorm
Copy link
Contributor Author

pmmccorm commented Dec 9, 2015

So the idea is/was that it is easier to leave lsnvme handle listing devices with libudev vs. factor out common code and share between nvme-cli and lsnvme.

Also: "nvme list" is the only command that does not use the nvme ioctl interface. I think that nvme-cli should just be a tool to test and inspect a device's through its ioctl interface.

As far as disrupting existing tests: I know it is hard, but now is the time to make a change like this. Even if lsnvme and nvme-cli share libudev code the output will change (since right now nvme-cli is not displaying some things correctly: displaying namespace properties as character device properties).

My idea is that when lsnvme/nvme-cli eventually merge as "nvme-utils" there will be:

lsnvme - list nvme controllers, block devices, and properties (libudev + hwdb + sysfs)
nvme-cli - low level testing and ioctl interface access only
...more

@keithbusch
Copy link
Contributor

Not entirely true. 'nvme show-regs' does not use ioctls either. It uses sysfs and mmap.

I don't think its a big deal to change how a sub-command displays it's data. Most of the commands are built to pipe it's data to other programs for parsing anyway, and the existing show routines are merely a convenience. I wouldn't rely on the output being consistent, though we should be careful to change/remove commands and their options.

To remove the duplication responsibly, we can make "nvme list" an alias to "lsnvme". From there we can either deprecate the sub-command, or add all the options that lsnvme supports.

I generally agree the ioctl component should be separated into a library, kind of like how the "show" routines are in progressing. I've a branch that splits ioctl handling from the command line parsing, and just need a little more free time to clean it up before submitting.

@pmmccorm
Copy link
Contributor Author

pmmccorm commented Dec 9, 2015

@keithbusch Yes about show-regs, and that is why I moved it into common.c because it looked useful to lsnvme (and the only way to determine version pre 1.2?).

I can see "nvme list" just exec-ing lsnvme. What we lose is the unix-like separation of tools: all the nvme tools are doing a little bit of everything vs each tool specializing in something and doing it well (like my example above). However if that is the desired structure I think we should organize these like git:

$ nvme show-regs # built in command, calls "/usr/bin/nvme show-regs" as expected
$ nvme list  # calls /usr/bin/nvme/nvme-list
# /usr/bin/nvme/nvme-list is "lsnvme"
$ man nvme-list

Just a thought.

Its good you mentioned you were factoring out the ioctl commands, I was about to do that..

ikegami-t pushed a commit to ikegami-t/nvme-cli that referenced this pull request Aug 18, 2023
jimmunn pushed a commit to Micron-TPG-OSS/nvme-cli that referenced this pull request Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants