Fix 'zpool list -v' alignment #8147
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #8147 +/- ##
==========================================
+ Coverage 78.45% 78.61% +0.16%
==========================================
Files 378 378
Lines 114765 114786 +21
==========================================
+ Hits 90035 90244 +209
+ Misses 24730 24542 -188
Continue to review full report at Codecov.
|
behlendorf
added
Status: Revision Needed
|
This may be outside the scope of this fixup, but you might want to consider fixing the |
tonyhutter
reviewed
Nov 26, 2018
cmd/zpool/zpool_main.c
Outdated
| int | ||
| get_namewidth(zpool_handle_t *zhp, void *data) | ||
| static int | ||
| get_namewidth(zpool_handle_t *zhp, int *width, int flags, boolean_t verbose) |
There was a problem hiding this comment.
- What about having
get_namewidth()return the width instead of having a separate*width? - Can you add a quick comment above the function deceleration:
/* Return the length of the pool/vdev name column. */
There was a problem hiding this comment.
Sure, after reworking things a little more I think that'll work.
tonyhutter
reviewed
Nov 26, 2018
behlendorf
force-pushed
the
issue-7308
branch
from
f7b518c to
e2a573b
Compare
|
Refreshed. and feedback addressed.
I'd rather leave this alone to make sure we don't break anything parsing this output. The documentation clearly states the output will print the columns separated by a single tab. I've also re-added the ALTROOT line to the output. Several helper functions in the ZTS look for this column and its absence resulted in test failures. The |
tonyhutter
approved these changes
Nov 27, 2018
behlendorf
added
Status: Code Review Needed
tcaputi
approved these changes
Nov 29, 2018
The verbose output of 'zpool list' was not correctly aligned due to differences in the vdev name lengths. Minimally update the code the correct the alignment using the same strategy employed by 'zpool status'. Missing dashes were added for the empty defaults columns, and the vdev state is now printed for all vdevs. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
behlendorf
force-pushed
the
issue-7308
branch
from
e2a573b to
3fe2e50
Compare
behlendorf
added
Status: Accepted
GregorKopka
pushed a commit
to GregorKopka/zfs
that referenced
this pull request
Jan 7, 2019
The verbose output of 'zpool list' was not correctly aligned due to differences in the vdev name lengths. Minimally update the code the correct the alignment using the same strategy employed by 'zpool status'. Missing dashes were added for the empty defaults columns, and the vdev state is now printed for all vdevs. Reviewed-by: Tom Caputi <tcaputi@datto.com> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#7308 Closes openzfs#8147
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Issue #7308. The current output of
zpool list -vdoes a terriblejob properly aligning its output. We want to correct that so it's
easy to read at a glance. This change only tackles the major
cosmetic issues and intentionally avoids any extensive refactoring.
There's quite a bit more which could be improved.
Description
The verbose output of 'zpool list' was not correctly aligned due
to differences in the vdev name lengths. Minimally update the
code the correct the alignment using the same strategy employed
by 'zpool status'.
Missing dashes were added for the empty defaults columns, and
the vdev state is now printed for all vdevs.
The ALTROOT properly was removed from the default output to get
close to staying within an 80 character width. At least for
short vdev names.
How Has This Been Tested?
Manually inspected the output for a moderating interesting pool configuration.
BEFORE:
AFTER:
Types of changes
Checklist:
Signed-off-by.