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

Fix optional "force" arg handing in zfs_ioc_pool_sync() #11284

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Issue #11281

Description

When checking if the "force" argument is set it is sufficient
to use nvlist_exists(). fnvlist_lookup_boolean_value() should
not be used in this case since it's never allowed to fail which
can happen when the provided nvlist does not set NV_UNIQUE_NAME.
This is consistent with the boolean option handling for the
other ioctls.

How Has This Been Tested?

Using the test case provided in #11281.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

nvpair_type DATA_TYPE_BOOLEAN != DATA_TYPE_BOOLEAN_VALUE, you have to check its value because it can be false.

zfs/lib/libzpool/util.c

Lines 222 to 240 in dcbf847

/*
* Use ZFS_IOC_POOL_SYNC to confirm if a pool is active
*/
fd = open(ZFS_DEV, O_RDWR);
if (fd < 0)
return (-1);
zcp = umem_zalloc(sizeof (zfs_cmd_t), UMEM_NOFAIL);
innvl = fnvlist_alloc();
fnvlist_add_boolean_value(innvl, "force", B_FALSE);
(void) strlcpy(zcp->zc_name, name, sizeof (zcp->zc_name));
packed = fnvlist_pack(innvl, &size);
zcp->zc_nvlist_src = (uint64_t)(uintptr_t)packed;
zcp->zc_nvlist_src_size = size;
ret = zfs_ioctl_fd(fd, ZFS_IOC_POOL_SYNC, zcp);

The fnvlist_lookup_boolean_value() function should not be used
to check the force argument since it's optional.  It may not be
provided or may have been created with the wrong flags.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#11281
@behlendorf
Copy link
Contributor Author

It does appear we set it this way. Updated.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 9, 2020
@behlendorf behlendorf merged commit edb20ff into openzfs:master Dec 9, 2020
ghost pushed a commit to zfsonfreebsd/ZoF that referenced this pull request Dec 23, 2020
The fnvlist_lookup_boolean_value() function should not be used
to check the force argument since it's optional.  It may not be
provided or may have been created with the wrong flags.

Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#11281
Closes openzfs#11284
behlendorf added a commit that referenced this pull request Dec 23, 2020
The fnvlist_lookup_boolean_value() function should not be used
to check the force argument since it's optional.  It may not be
provided or may have been created with the wrong flags.

Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #11281
Closes #11284
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The fnvlist_lookup_boolean_value() function should not be used
to check the force argument since it's optional.  It may not be
provided or may have been created with the wrong flags.

Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#11281
Closes openzfs#11284
@behlendorf behlendorf deleted the issue-11281 branch April 19, 2021 19:21
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
The fnvlist_lookup_boolean_value() function should not be used
to check the force argument since it's optional.  It may not be
provided or may have been created with the wrong flags.

Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#11281
Closes openzfs#11284
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.

1 participant