Allow appending to default props for zfs and zpool list.

[Shreshth3]

This PR allows users to add specific properties in addition to the defaults when using zfs list -o or zpool list -o. For example, zpool list -o +guid will run zpool list showing the default properties in addition to guid.

Resolves #17112.

One note: with the current implementation, if a user tries to append a property that is already in the defaults (e.g. name), they will see that column two times. If this is not acceptable, I can adjust the implementation accordingly.

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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[intelfx]

One note: with the current implementation, if a user tries to append a property that is already in the defaults (e.g. name), they will see that column two times. If this is not acceptable, I can adjust the implementation accordingly.

It would be nice to support this case by removing such a column from the "default" part of the list and inserting it into the "user" part of the list at the specified position. E.g. I might want to keep the mountpoint column last, and so I'd write zfs list -o +lused,lrefer,mountpoint.

Additionally, perhaps some care is needed to support (or cleanly refuse) repeated -o +... options. As written, it looks like only one -o +... option is ever expected by the code. This is a non-intuitive restriction and might create problems when scripting around it.

[tonyhutter]

Additionally, perhaps some care is needed to support (or cleanly refuse) repeated -o +... options. As written, it looks like only one -o +... option is ever expected by the code. This is a non-intuitive restriction and might create problems when scripting around it.

It's a good point to bring up. Personally, I like the simplicity of -o +... and I like that it follows the lsblk conventions. It's something I would use in my day-to-day work. That simplicity is a bigger plus to me than the possible conflict with "multiple -o args" in the future. And even then it may not be a conflict - you can just say the + literally means "put the defaults here". So zpool list -o guid -o +compression would have columns in the order of: guid, <the default columns>, compression.

[tonyhutter]

Could you add a simple test case?

[Shreshth3]

Additionally, perhaps some care is needed to support (or cleanly refuse) repeated -o +... options. As written, it looks like only one -o +... option is ever expected by the code. This is a non-intuitive restriction and might create problems when scripting around it.

It looks to me like the normal -o in ZFS already has this issue. For example,

zfs master λ ./zpool list -o name -o guid                   
                GUID
13145110569708569494

and

zfs master λ ./zfs list -o name -o guid
                GUID
17425442327516357480

Given this, it doesn't feel to me like this PR is the right place to address this issue. It seems worthwhile to have a separate thread discussing how ZFS should handle multiple -o options, and then implement changes accordingly.

[Shreshth3]

Could you add a simple test case?

Added.

[tonyhutter]

Could you add a simple test case?

Added.

I started reviewing your test cases yesterday and ended up tweaking them a little 😄 . The new version gets rid of the temporary files, adds stricter checks for column order, and verifies the JSON (diff below):

diff

diff --git a/tests/zfs-tests/tests/functional/cli_user/zfs_list/zfs_list_009_pos.ksh b/tests/zfs-tests/tests/functional/cli_user/zfs_list/zfs_list_009_pos.ksh
index 5a499275f..6eae533cb 100755
--- a/tests/zfs-tests/tests/functional/cli_user/zfs_list/zfs_list_009_pos.ksh
+++ b/tests/zfs-tests/tests/functional/cli_user/zfs_list/zfs_list_009_pos.ksh
@@ -37,26 +37,23 @@
 #      Verify 'zfs list -o+' allows users to append a column to the defaults
 #
 # STRATEGY:
-# 1. Execute `zfs list -o+guid`.
-# 2. Confirm that the "name" and "guid" columns both appear in the output.
-#
-
-function cleanup
-{
-       if [[ -f $tmpfile ]]; then
-               rm -f $tmpfile
-       fi
-}
+# 1. Add a user comment to dataset
+# 2. Execute `zfs list -o +comment:`.
+# 3. Verify the first column of the defaults gets printed.
+# 4. Verify the user comment appears as the last column.
+# 5. Verify we see both one of the default entries and the user comment in JSON.
 
 verify_runnable "both"
-log_onexit cleanup
 
 log_assert "Verify 'zfs list -o+<...>' appends columns to the defaults."
 
-tmpfile=$TEST_BASE_DIR/zfslist.out.$$
+log_must zfs set comment:=helloworld $TESTPOOL
 
-zfs list -o+guid > $tmpfile
-log_must grep -qi "name" $tmpfile
-log_must grep -qi "guid" $tmpfile
+log_must eval zfs list $TESTPOOL -o +comment: | grep -E '^NAME.+COMMENT:$'
+log_must eval zfs list $TESTPOOL -o +comment: | grep -E "^$TESTPOOL.+helloworld$"
+val=$(zfs list -j -o +comment: $TESTPOOL | jq -r '.datasets.'$TESTPOOL'.properties."comment:".value')
+log_must test $val == "helloworld"
+val=$(zfs list -j -o +comment: $TESTPOOL | jq -r ".datasets.$TESTPOOL.properties.mountpoint.value")
+log_must test $val == "/$TESTPOOL"
 
 log_pass "'zfs list -o+<...>' successfully added columns to the defaults."
diff --git a/tests/zfs-tests/tests/functional/cli_user/zpool_list/zpool_list_003_pos.ksh b/tests/zfs-tests/tests/functional/cli_user/zpool_list/zpool_list_003_pos.ksh
index 000c8b72d..18d831525 100755
--- a/tests/zfs-tests/tests/functional/cli_user/zpool_list/zpool_list_003_pos.ksh
+++ b/tests/zfs-tests/tests/functional/cli_user/zpool_list/zpool_list_003_pos.ksh
@@ -37,16 +37,11 @@
 # Verify that 'zpool list -o+' allows users to append a column to the defaults.
 #
 # STRATEGY:
-# 1. Execute `zpool list -o+guid`
-# 2. Confirm that the "name" and "guid" columns both appear in the output.
-#
-
-function cleanup
-{
-       if [[ -f $tmpfile ]]; then
-               rm -f $tmpfile
-       fi
-}
+# 1. Add a user comment to the pool
+# 2. Execute `zfs list -o +comment`.
+# 3. Verify the first column of the defaults gets printed.
+# 4. Verify the user comment appears as the last column.
+# 5. Verify we see both one of the default entries and the user comment in JSON.
 
 verify_runnable "both"
 log_onexit cleanup
@@ -57,10 +52,16 @@ fi
 
 log_assert "Verify 'zpool list -o+<...>' appends columns to the defaults."
 
-tmpfile=$TEST_BASE_DIR/zpoollist.out.$$
+log_must zpool set comment="helloworld" $TESTPOOL
+
+# Verify the first and last columns are correct
+log_must eval zpool list -o +comment | grep -Eq '^NAME.+COMMENT$'
+log_must eval zpool list -o +comment | grep -Eq "^$TESTPOOL.+helloworld$"
 
-zpool list -o+guid > $tmpfile
-log_must grep -qi "name" $tmpfile
-log_must grep -qi "guid" $tmpfile
+# Verify we see both a default value and our added value in the JSON
+val=$(zpool list -j -o +comment | jq -r ".pools.$TESTPOOL.properties.comment.value")
+log_must test "$val" == "helloworld"
+val=$(zpool list -j -o +comment | jq -r ".pools.$TESTPOOL.properties.health.value")
+log_must test "$val" == "ONLINE"
 
 log_pass "'zpool list -o+<...>' successfully added columns to the defaults."

[Shreshth3]

Integrated your changes, thanks for making them! I ended up hand-copying the diff (ran into an issue trying to apply the patch), so worth double checking that things look the way you intended. I also edited a couple small things (removing log_onexit cleanup, and making it so that both greps are grep -Eq).