Skip to content

Conversation

Shreshth3
Copy link
Contributor

@Shreshth3 Shreshth3 commented Oct 11, 2025

Fixes #11040.

Right now, the -v and -o options for zpool list work independently, but when paired, the -v "wins out" and the -o effect is lost. This commit fixes that problem.

Before my change:
(master) ~/zfs$ ./zpool list -v -o name,health mypool
NAME          HEALTH
mypool        ONLINE
  mirror-0   960M   110K   960M        -         -     2%  0.01%      -    ONLINE
    loop17     1G      -      -        -         -      -      -      -    ONLINE
    loop18     1G      -      -        -         -      -      -      -    ONLINE
After my change:
(v-o-option-conflict) ~/zfs$ ./zpool list -v -o name,health mypool
NAME          HEALTH
mypool        ONLINE
  mirror-0    ONLINE
    loop17    ONLINE
    loop18    ONLINE

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Fixes openzfs#11040.

Right now, the -v and -o options for `zpool list`
work independently, but when paired, the -v
"wins out" and the -o effect is lost. This
commit fixes that problem.

Signed-off-by: Shreshth Srivastava <[email protected]>
@github-actions github-actions bot added the Status: Work in Progress Not yet ready for general review label Oct 11, 2025
@Shreshth3 Shreshth3 marked this pull request as ready for review October 11, 2025 07:29
@github-actions github-actions bot added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Oct 11, 2025
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice straightforward fix, good job!

Comment on lines +7093 to +7096
cap =
(vs->vs_space == 0)
? 0
: (vs->vs_alloc * 10000 / vs->vs_space);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we make it more compact here and in other places? We do have 80 columns for last 30+ years. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any advice for a good setup to handle code formatting? I spent quite a bit of time trying to get a good setup, but haven't found anything that works well. I was going for format-on-save based a local .clang-format file. But with that setup, when I make my changes and save, it's re-formatting lines throughout the entire file.

Would be curious what you or any other maintainers do for code formatting. Thank you in advance!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately none of the code formatters around appear able to express the exact style, though some come very close. You should run make cstyle (or the script under it, scripts/cstyle.pl) as often as you can, which will definitely tell you when you've broken the "rules", such as they are.

For things like continuation line layout (the topic here), cstyle doesn't have a lot to say except that you get within 80 chars, and that your leading tabs and spaces look right. It won't care too much about where the lines are split and how many. Most people seem to just compress them into as few lines as possible; I usually try a few variations to balance concision with readability, but it can get pretty difficult sometimes.

This one, I would probably do:

			case ZPOOL_PROP_CAPACITY:
				cap = (vs->vs_space == 0) ?
				    0 : (vs->vs_alloc * 10000 / vs->vs_space);

I personally prefer to finish lines with an operator if I have the choice, and this is arguably readable as the possible outcomes are grouped together on the same line. If the computation had gone longer than 80, I probably would tried to at least avoid breaking the parens, and so made it more like:

			case ZPOOL_PROP_CAPACITY:
				cap = (vs->vs_space == 0) ? 0 :
				    (vs->vs_alloc * 10000 / vs->vs_space);

Sorry that's so vague; it's just what we've inherited. I'd would personally love to develop a (say) clang-format file that expresses most of what we have, and then we just take the defaults on the rest (in fact, I have developed most of this config a few times). The last bits though, that it doesn't support, needs a little thinking through and then someone to make the arguments and get the feedback and consensus and push it over the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Code Review Needed Ready for review and testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zpool list -v disregards other options

3 participants