Skip to content
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

Add support for zpool user properties #11680

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

allanjude
Copy link
Contributor

zpool set org.freebsd:comment="this is my pool" poolname

Signed-off-by: Allan Jude allan@klarasystems.com

Motivation and Context

Allow users to create and set arbitrary pool properties, with the same syntax as dataset user properties (a user property is identified by the colon separator, and by convention is formatted org.name:propername)

Description

Allows to set and get user properties on the pool (district from dataset properties on the root dataset)

How Has This Been Tested?

Not very much

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)
  • 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:

@AndyLavr
Copy link

AndyLavr commented Mar 3, 2021

Error:

free(): invalid pointer
Aborted

# zpool set org.linux:comment="test pool" test2

# zpool get all test2                          

--- skip

test2  compatibility                  off                            default
test2  org.linux:comment              test pool                      local
test2  feature@async_destroy          enabled                        local

--- skip

test2  feature@draid                  enabled                        local
test2  feature@compress_adaptive      enabled                        local
free(): invalid pointer
Aborted

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 3, 2021
@allanjude
Copy link
Contributor Author

Error:

free(): invalid pointer
Aborted

in zpool_expand_proplist() I had:

entry->pl_user_prop = propname;

But later in zprop_free_list() it would free pl_user_prop, which was the middle of the nvlist allocation. Solved by using zfs_strdup() to keep a separate copy of the property name.

@allanjude
Copy link
Contributor Author

Fixed style

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

The code looks good to me. But I think we should add a couple basic test cases to exercise this new functionality. It looks like in particular you could adapt cli_root/zfs_set/user_property_001_pos.ksh and cli_root/zfs_set/user_property_003_neg.ksh pretty easily and that would provide some coverage.

@allanjude
Copy link
Contributor Author

Mateusz has written the first few tests for this, and cleaned things up.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

You'll want to also update man/man7/zpoolprops.7 to reflect that user properties are supported for the pool. Modeling this after what was done in man/man7/zfsprops.7 seems like a reasonable way to go.

man/man7/zpoolprops.7 Outdated Show resolved Hide resolved
@allanjude allanjude force-pushed the zpool_user_props branch 3 times, most recently from 93de520 to 0da5c12 Compare April 14, 2023 17:03
@allanjude
Copy link
Contributor Author

The new tests are working correctly now

@0mp
Copy link
Contributor

0mp commented Apr 18, 2023

Hmm, I pushed the fixes to the https://github.com/allanjude/zfs/tree/zpool_user_props branch but GitHub does not seem to have propagated the changes to this PR... It should be pointing to allanjude@cf20058 now.

0mp pushed a commit to 0mp/freebsd that referenced this pull request Apr 18, 2023
Usage:

    zpool set org.freebsd:comment="this is my pool" poolname

Tests are based on zfs_set's user property tests.

Also stop truncating property values at MAXNAMELEN, use ZFS_MAXPROPLEN.

Co-authored-by:	Mateusz Piotrowski <0mp@FreeBSD.org>
Obtained from:	OpenZFS PR openzfs/zfs#11680
Sponsored by:	Beckhoff Automation GmbH & Co. KG.
Sponsored by:	Klara Inc.
0mp pushed a commit to 0mp/freebsd that referenced this pull request Apr 18, 2023
Summary:
Usage:

    zpool set org.freebsd:comment="this is my pool" poolname

Tests are based on zfs_set's user property tests.

Also stop truncating property values at MAXNAMELEN, use ZFS_MAXPROPLEN.

Co-authored-by:	Mateusz Piotrowski <0mp@FreeBSD.org>
Obtained from:	OpenZFS PR openzfs/zfs#11680
Sponsored by:	Beckhoff Automation GmbH & Co. KG.
Sponsored by:	Klara Inc.

Reviewers: allanjude

Subscribers: imp, delphij

Differential Revision: https://reviews.freebsd.org/D39657
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 18, 2023
@behlendorf
Copy link
Contributor

@0mp we're resolved the FreeBSD build failure. Can you rebase on the latest commits to master so we can verify this builds cleanly on FreeBSD.

@0mp
Copy link
Contributor

0mp commented Apr 20, 2023

@0mp we're resolved the FreeBSD build failure. Can you rebase on the latest commits to master so we can verify this builds cleanly on FreeBSD.

Rebased!

@behlendorf
Copy link
Contributor

Sorry, it looks like this has some minor conflicts with the recently merged 3e4ed42. One last rebase is needed, then I'll get it merged.

Usage:

    zpool set org.freebsd:comment="this is my pool" poolname

Tests are based on zfs_set's user property tests.

Also stop truncating property values at MAXNAMELEN, use ZFS_MAXPROPLEN.

Sponsored-by: Beckhoff Automation GmbH & Co. KG.
Sponsored-by: Klara Inc.
Co-authored-by: Mateusz Piotrowski <mateusz.piotrowski@klarasystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Signed-off-by: Mateusz Piotrowski <mateusz.piotrowski@klarasystems.com>
@0mp
Copy link
Contributor

0mp commented Apr 21, 2023

Sorry, it looks like this has some minor conflicts with the recently merged 3e4ed42. One last rebase is needed, then I'll get it merged.

Hey Brian, I've resolved the conflict, rebased, and pushed the latest version. :)

@behlendorf behlendorf merged commit 8eae2d2 into openzfs:master Apr 21, 2023
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request May 1, 2023
Usage:

    zpool set org.freebsd:comment="this is my pool" poolname

Tests are based on zfs_set's user property tests.

Also stop truncating property values at MAXNAMELEN, use ZFS_MAXPROPLEN.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Signed-off-by: Mateusz Piotrowski <mateusz.piotrowski@klarasystems.com>
Sponsored-by: Beckhoff Automation GmbH & Co. KG.
Sponsored-by: Klara Inc.
Closes openzfs#11680
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants