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.

Sign up for GitHub

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

Jump to bottom

Fix dsl_props_set_sync_impl to work with nested nvlist #5497

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Fix dsl_props_set_sync_impl to work with nested nvlist #5497

merged 1 commit into from
Dec 21, 2016

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Dec 17, 2016 •
edited
Loading

Description

When we iterate over the input (nested) nvlist props in dsl_props_set_sync_impl() we don't preserve the nvpair name before looking up ZPROP_VALUE, so when we later go to process it nvpair_name() is always "value" and not the actual property name.

Motivation and Context

This issue was discovered while trying to implement zfs receive override (-o|-x) properties, it manifests when we fail to receive a send stream causing previously received properties to be wiped clean (instead of being restored). Other codepaths may be affected.
Also, i previously assumed Illumos was not affected because i had no way to manually inject an error in zfs_ioc_recv() via zfs_ioc_recv_inject_err (i used systemtap for ZoL) but now i found a way to reproduce the same behaviour on a semi-new release of smartos (20160527T033529Z) ... so it seems Illumos is affected too.

I was going to push this change with the feature PR but given that it's taking more time than expected and this is a small change might as well push it for review as a separate PR.

EDIT: test case added.

How Has This Been Tested?

Shell script here: https://gist.github.com/loli10K/3828245287eb6405bfc661c17a458166 (requires systemtap and a DEBUG-compiled ZFS)

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)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Change has been approved by a ZFS on Linux member.

@mention-bot
Copy link

@loli10K, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ahrens, @behlendorf and @scsiguy to be potential reviewers.

@loli10K loli10K force-pushed the issue-dsl_props_set_sync_impl branch from 1e3854a to 3746b84 Compare December 18, 2016 07:20
behlendorf
behlendorf requested changes Dec 19, 2016
View reviewed changes
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.

Nice find and test case. It took me a while to wrap my head around exactly why this is needed. Only after examining dsl_prop_get_all_impl() as referenced in dsl_props_set_sync_impl was it clear.

My only suggestion would be to clarify that comment to make it clear why exactly this is needed. A brief description of the expected format of the props variable would have been extremely helpful.

@loli10K
Copy link
Contributor Author

loli10K commented Dec 19, 2016

@behlendorf

A brief description of the expected format of the props variable would have been extremely helpful.

Something like this https://github.com/zfsonlinux/zfs/blob/zfs-0.6.5.8/module/nvpair/nvpair.c#L56 would suffice?

@behlendorf
Copy link
Contributor

behlendorf commented Dec 19, 2016 •
edited
Loading

@loli10K sorry I wasn't clear. What was confusing to me initially was why the props variable could contain a nested nvlist at all. A brief description of under what circumstances this can happen would have been helpful.

loli10K
loli10K commented Dec 20, 2016
View reviewed changes
module/zfs/dsl_prop.c
* returned by the counterpart dsl_prop_get_all_impl().
* For instance we do this to restore the original
* received properties when an error occurs in the
* zfs_ioc_recv() codepath.
*/
Copy link
Contributor Author

@loli10K loli10K Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@behlendorf
I don't know if this is what you had in mind ... i'm not good at explaining things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that'll work. Thanks.

@loli10K
Fix dsl_props_set_sync_impl to work with nested nvlist
834d4e9 
When iterating over the input nvlist in dsl_props_set_sync_impl() when we don't
preserve the nvpair name before looking up ZPROP_VALUE, so when we later go to
process it nvpair_name() is always "value" and not the actual property name.

This fixes a couple of bugs in zfs_ioc_recv():
* Received properties were not restored correctly when failing to receive an
incremental send stream
* Received properties were not completely replaced by the new ones when
successfully receiving an incremental send stream

Signed-off-by: loli10K <[email protected]>
@loli10K loli10K force-pushed the issue-dsl_props_set_sync_impl branch from 3607c96 to 834d4e9 Compare December 20, 2016 21:13
@behlendorf behlendorf merged commit 5f1346c into openzfs:master Dec 21, 2016
@loli10K loli10K deleted the issue-dsl_props_set_sync_impl branch December 21, 2016 06:25
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
@loli10K @wli5
Fix dsl_props_set_sync_impl to work with nested nvlist
0fc2a94 
When iterating over the input nvlist in dsl_props_set_sync_impl() when we don't
preserve the nvpair name before looking up ZPROP_VALUE, so when we later go to
process it nvpair_name() is always "value" and not the actual property name.

This fixes a couple of bugs in zfs_ioc_recv():
* Received properties were not restored correctly when failing to receive an
incremental send stream
* Received properties were not completely replaced by the new ones when
successfully receiving an incremental send stream

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: loli10K <[email protected]>
Closes openzfs#5497
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
@loli10K @wli5
Fix dsl_props_set_sync_impl to work with nested nvlist
332698a 
When iterating over the input nvlist in dsl_props_set_sync_impl() when we don't
preserve the nvpair name before looking up ZPROP_VALUE, so when we later go to
process it nvpair_name() is always "value" and not the actual property name.

This fixes a couple of bugs in zfs_ioc_recv():
* Received properties were not restored correctly when failing to receive an
incremental send stream
* Received properties were not completely replaced by the new ones when
successfully receiving an incremental send stream

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: loli10K <[email protected]>
Closes openzfs#5497
@loli10K loli10K mentioned this pull request Jun 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@loli10K @mention-bot @behlendorf