-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zpool: Fix column alignment with long zpool names #6786
Conversation
Looks good, just fix the commitcheck error:
|
And please rebase this branch against the latest master. |
cmd/zpool/zpool_main.c
Outdated
|
||
free(name); | ||
/* | ||
* vdev_name is "root-0" for the root vdev on my machine, but it's the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this comment mean? my machine
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my machine all root vdev names are root-0
(for all zpools).
I wasn't sure if it was possible for them to be something else (e.g. root-1
, or other-type-10
), but from a brief look at the codebase I now think root vdev names will always be root-0
.
Code updated. If my understanding is incorrect, please let me know.
d3264ed
to
1e06922
Compare
@tonyhutter The only lines exceeding 72 characters are the ones showing command output verbatim:
and
Should I truncate the lines or leave them as they are? I have rebased the branch and have also added an |
4a3c178
to
a248220
Compare
@gg7 for the sake of readability I think we can ignore the usual line length limit in the commit message. |
a248220
to
affabef
Compare
cmd/zpool/zpool_main.c
Outdated
* name that will be displayed to the user. | ||
*/ | ||
if (max <= 0) | ||
max = MAX(max, strlen(zpool_get_name(zhp))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible for a NULL zhp
to be passed to max_width
which results in segmentation fault. This is why you're seeing all the failures from the automated testing. At a minimum you'll need to add a NULL check here. I think it would also be worthwhile to move this logic down in to zpool_vdev_name()
for printing the pool name instead of root-0
. This would then allow you to properly handle the case where the pool guid is printed instead of its name. If it's needed we could add another vdev_name_t
to the enum to optionally control this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. I will redo my patch soon.
cmd/zpool/zpool_main.c
Outdated
/* | ||
* vdev_name will be "root-0" for the root vdev, but it is the zpool | ||
* name that will be displayed to the user. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this up in to the block comment above the function and clearly describe that callers are expected to pass 0
for max
.
affabef
to
9c01615
Compare
@behlendorf I have moved the "get zpool name" logic inside of Could you please tell me how to get the zpool name if
I tried doing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reworking this.
You can run the ZFS Test Suite locally to more quickly iterate on specific test cases.
For example, if you rebase on master you can quickly run the zpool import
test cases which will trigger this by running the following in-tree.
./scripts/zfs.sh
./scripts/zfs-tests.sh -v -T zpool_import
lib/libzfs/libzfs_pool.c
Outdated
* vdev_name will be "root"/"root-0" for the root vdev, but it is the | ||
* zpool name that will be displayed to the user. | ||
* | ||
* TODO: figure out a way to get the zpool name when zhp is NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please tell me how to get the zpool name if zhp is null?
This will happen when using the zpool import -D
to import a previously destroyed pool. Since this is such an uncommon case I don't think it's critical that we handle it and we can drop this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@behlendorf As mentioned above, the pool could have been exported (and not destroyed), which is a more common scenario:
george@george:~$ sudo zpool export dummy-very-very-long-zpool-name
george@george:~$ sudo zpool import -d /tmp
pool: dummy-very-very-long-zpool-name
id: 15691137324734536224
state: ONLINE
action: The pool can be imported using its name or numeric identifier.
config:
dummy-very-very-long-zpool-name ONLINE
mirror-0 ONLINE
/tmp/dummy-1.bin ONLINE
/tmp/dummy-2.bin ONLINE
mirror-1 ONLINE
/tmp/dummy-3.bin ONLINE
/tmp/dummy-4.bin ONLINE
This is annoying me, but if you think it's not worth fixing then I will remove the TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is annoying, for the show_import()
case how about something like this to address it.
diff --git a/cmd/zpool/zpool_main.c b/cmd/zpool/zpool_main.c
index b6b6f0c..8493f28 100644
--- a/cmd/zpool/zpool_main.c
+++ b/cmd/zpool/zpool_main.c
@@ -2216,7 +2216,8 @@ show_import(nvlist_t *config)
(void) printf(gettext(" config:\n\n"));
- cb.cb_namewidth = max_width(NULL, nvroot, 0, 0, VDEV_NAME_TYPE_ID);
+ cb.cb_namewidth = max_width(NULL, nvroot, 0, strlen(name),
+ VDEV_NAME_TYPE_ID);
if (cb.cb_namewidth < 10)
cb.cb_namewidth = 10;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, it works! Thank you.
lib/libzfs/libzfs_pool.c
Outdated
* TODO: figure out a way to get the zpool name when zhp is NULL | ||
*/ | ||
verify(nvlist_lookup_string(nv, ZPOOL_CONFIG_TYPE, &type) == 0); | ||
if (strcmp(type, "root") == 0 && zhp != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you invert this so the NULL check comes before the strcmp().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, fixed.
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io>
9c01615
to
25f327f
Compare
Thank you for the suggestion, but my I have implemented your suggestions and have also removed an extra call to |
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes openzfs#6786
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes openzfs#6786 Requires-spl: refs/pull/669/head
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes openzfs#6786 Requires-spl: refs/pull/669/head
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes #6786
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes openzfs#6786
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes openzfs#6786
`zpool status` normally aligns NAME/STATE/etc columns: NAME STATE READ WRITE CKSUM dummy ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-long-1.bin ONLINE 0 0 0 /tmp/dummy-long-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-long-3.bin ONLINE 0 0 0 /tmp/dummy-long-4.bin ONLINE 0 0 0 However, if the zpool name is longer than the zvol names, alignment issues arise: NAME STATE READ WRITE CKSUM dummy-very-very-long-zpool-name ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 /tmp/dummy-1.bin ONLINE 0 0 0 /tmp/dummy-2.bin ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 /tmp/dummy-3.bin ONLINE 0 0 0 /tmp/dummy-4.bin ONLINE 0 0 0 `zpool iostat` and `zpool import` are also affected: capacity operations bandwidth pool alloc free read write read write ---------- ----- ----- ----- ----- ----- ----- dummy 104K 1.97G 0 0 152 9.84K dummy-very-very-long-zpool-name 152K 1.97G 0 1 144 13.1K ---------- ----- ----- ----- ----- ----- ----- dummy-very-very-long-zpool-name ONLINE mirror-0 ONLINE /tmp/dummy-1.bin ONLINE /tmp/dummy-2.bin ONLINE mirror-1 ONLINE /tmp/dummy-3.bin ONLINE /tmp/dummy-4.bin ONLINE Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: George Gaydarov <git@gg7.io> Closes openzfs#6786
Description
zpool status
normally alignsNAME
/STATE
/etc columns:However, if the zpool name is longer than the zvol names, alignment issues
arise:
zpool iostat
is also affected:My patch fixes column alignment:
How Has This Been Tested?
Manually. You can reproduce the zpool setup like this:
Types of changes
Checklist:
Signed-off-by
.