Add -gLp to zpool subcommands for alt vdev names #4343
Conversation
|
@ryao @ilovezfs I took the opportunity to build on the work you've both done adding options to control the output of vdevs names show by various zpool subcommands. This is a refactoring of those changes with an additional options, -p, and extending it to more of the subcommands. It would be great if I could get your feedback. This has been lightly tested and works as expected. |
|
@behlendorf I see that you are burning the midnight oil on this. :) The thing that kept this from being accepted in the past was that people wanted to see nvlists used and I never made time to refactor it, although an There is nothing that jumps out at me as being bad in this patch. |
| .ad | ||
| .RS 12n | ||
| .rt | ||
| Display vdev GUIDs instead of the normal short device names. These GUIDs can be used in-place of device names for the zpool detach/offline/remove/replace commands. |
There was a problem hiding this comment.
"in place" not "in-place"
|
@behlendorf ryao is referring to I'm not sure I ever actually really "agreed" with that, but it's the reason I used an nvlist. I think I actually prefer the flag instead, but it would be good if you could explain why you think it's a good idea to use a flag and not an nvlist as @wca had been requesting. |
|
"detach/offline/remove/replace" Does it work with |
| extern char *zpool_vdev_name(libzfs_handle_t *, zpool_handle_t *, nvlist_t *, | ||
| boolean_t verbose); | ||
| int name_flags); |
There was a problem hiding this comment.
@ryao Is libzfs.h sufficiently "internal" to consider this an "internal interface"?
behlendorf
force-pushed
the
issue-2012
branch
from
f6ddc7a
to
9ffd414
Compare
The following options have been added to the zpool add, iostat,
list, status, and split subcommands. The default behavior was
not modified, from zfs(8).
-g Display vdev GUIDs instead of the normal short
device names. These GUIDs can be used in-place of
device names for the zpool detach/off‐
line/remove/replace commands.
-L Display real paths for vdevs resolving all symbolic
links. This can be used to lookup the current block
device name regardless of the /dev/disk/ path used
to open it.
-p Display full paths for vdevs instead of only the
last component of the path. This can be used in
conjunction with the -L flag.
This behavior may also be enabled using the following environment
variables.
ZPOOL_VDEV_NAME_GUID
ZPOOL_VDEV_NAME_FOLLOW_LINKS
ZPOOL_VDEV_NAME_PATH
This change is based on worked originally started by Richard Yao
to add a -g option. Then extended by @ilovezfs to add a -L option
for openzfsonosx. Those changes have been merged, re-factored,
a -p option added and extended to all relevant zpool subcommands.
Original-patch-by: Richard Yao <ryao@gentoo.org>
Extended-by: ilovezfs <ilovezfs@icloud.com>
Extended-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#2011
Issue openzfs#4341
behlendorf
force-pushed
the
issue-2012
branch
from
9ffd414
to
ad1616a
Compare
Actually, I was under the exact opposite impression. One of the reasons it wasn't accepted was because it did introduce this nvlist. At least personally I felt it was an unnecessary complication for very little gain. I should have been clearer about that in the original PR. Let's debate the point though if you feel strongly about it.
No it doesn't. It was added to the expected status/iostat/list commands and other command which supported a dryrun option. Although it looks like I missed |
|
@behlendorf Personally I prefer the flag. I just wanted to make sure everyone's on the same page about what the requirements should or shouldn't be.
I don't think I was clear. I was actually quoting the new man page entries regarding the use of the printed GUIDs:
That references specific commands that accept GUID but I don't think it's a complete list.
The "it" in question is the ability to accept GUIDs in place of device names. Is "detach/offline/remove/replace" actually the full list? |
I suspect you're right. Most command should accept guids but I'm not sure if they all do. |
|
Yeah, the only outstanding concern I have is that each additional format if more are added will consume another option in each command that uses this. Would it be better to have this be a -o format=%g type thing? |
|
That would certainly be more flexible. I flirted with a similar idea but eventually decided against it because I felt it would be a somewhat awkward interface. While we are using a few extra flags, which isn't ideal, I personally think it's a tradeoff worth making in this case. |
|
@behlendorf OK, that's fine. If someone else wants to complain about that, they can make some other pull request changing the CLI accordingly. I have no further objections. |
|
Merged as: d2f3e29 Add -gLp to zpool subcommands for alt vdev names |
|
There is a documentation error in this pull request. invalid option 'p' |
|
@timemaster67 this was addressed by commit a77f29f. |
The following options have been added to the zpool add, iostat,
list, status, and split subcommands. The default behavior was
not modified, from zfs(8).
-g Display vdev GUIDs instead of the normal short
device names. These GUIDs can be used in-place of
device names for the zpool detach/off‐
line/remove/replace commands.
-L Display real paths for vdevs resolving all symbolic
links. This can be used to lookup the current block
device name regardless of the /dev/disk/ path used
to open it.
-p Display full paths for vdevs instead of only the
last component of the path. This can be used in
conjunction with the -L flag.
This change is based on worked originally started by Richard Yao
to add a -g option. Then extended by @ilovezfs to add a -L option
for openzfsonosx. Those changes have been merged, re-factored,
a -p option added and extended to all relevant zpool subcommands.
Original-patch-by: Richard Yao ryao@gentoo.org
Extended-by: ilovezfs ilovezfs@icloud.com
Extended-by: Brian Behlendorf behlendorf1@llnl.gov
Issue #2011
Issue #4341