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 error code on __zpl_ioctl_setflags() #11791

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

lumigch
Copy link
Contributor

@lumigch lumigch commented Mar 24, 2021

Other (all?) Linux filesystems seem to return -EPERM instead of -EACCESS when
trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the CAP_LINUX_IMMUTABLE
capability. This was detected by generic/545 test in the fstest suite.

Signed-off-by: Luis Henriques henrix@camandro.org

Motivation and Context

It normalizes error code across other filesystems.

Description

As mentioned in the commit changelog: simply return -EPERM instead of -EACCESS when trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the CAP_LINUX_IMMUTABLE capability.

How Has This Been Tested?

Executed a (modified) fstest suite, specifically generic/545

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:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 24, 2021
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.

Thanks for catching this slight difference in behavior! Can you just address the style warning and then this should be good. You can run make checkstyle locally before updating the PR.

If you encounter any other unexpected discrepancies while testing with xfstests by all means let us know. Patches are even better! It's something we've wanted to add to our usual testing for a while now but haven't yet automated since it takes a little tweaking to run it against ZFS as I'm sure you know.

Other (all?) Linux filesystems seem to return -EPERM instead of -EACCESS
when trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the
CAP_LINUX_IMMUTABLE capability.  This was detected by generic/545 test
in the fstest suite.

Signed-off-by: Luis Henriques <henrix@camandro.org>
@lumigch
Copy link
Contributor Author

lumigch commented Mar 24, 2021

Thanks for catching this slight difference in behavior! Can you just address the style warning and then this should be good. You can run make checkstyle locally before updating the PR.

Thanks for the review! I should have read more carefully the CONTRIBUTING.md before submitting my first fix. I've pushed a version with that style correction (TBH I assumed that change was making that if statement more readable; oh well...).

If you encounter any other unexpected discrepancies while testing with xfstests by all means let us know. Patches are even better! It's something we've wanted to add to our usual testing for a while now but haven't yet automated since it takes a little tweaking to run it against ZFS as I'm sure you know.

Yeah, it's tricky to run fstests against ZFS. I had to hack a few things and I'm sure there were a lot of tests skipped because some more hacks were missing. Is there a public tree with these tweaks available? (Obviously, I it would be nice to try to push these changes upstream.)

@behlendorf
Copy link
Contributor

Is there a public tree with these tweaks available?

Nothing current that I'm aware of. It would be ideal if whatever changes are needed to support ZFS could be pushed upstream to xfstests. We could maintain our own repo with the changes, but being able to use an unmodified upstream version would be great. That would make it easier for us to integrate in to our CI workflow.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 26, 2021
@behlendorf behlendorf merged commit 2037edb into openzfs:master Mar 26, 2021
adamdmoss pushed a commit to adamdmoss/zfs that referenced this pull request Apr 10, 2021
Other (all?) Linux filesystems seem to return -EPERM instead of -EACCESS
when trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the
CAP_LINUX_IMMUTABLE capability.  This was detected by generic/545 test
in the fstest suite.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Luis Henriques <henrix@camandro.org>
Closes openzfs#11791
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Other (all?) Linux filesystems seem to return -EPERM instead of -EACCESS
when trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the
CAP_LINUX_IMMUTABLE capability.  This was detected by generic/545 test
in the fstest suite.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Luis Henriques <henrix@camandro.org>
Closes openzfs#11791
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jun 4, 2021
Other (all?) Linux filesystems seem to return -EPERM instead of -EACCESS
when trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the
CAP_LINUX_IMMUTABLE capability.  This was detected by generic/545 test
in the fstest suite.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Luis Henriques <henrix@camandro.org>
Closes openzfs#11791
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
Other (all?) Linux filesystems seem to return -EPERM instead of -EACCESS
when trying to set FS_APPEND_FL or FS_IMMUTABLE_FL without the
CAP_LINUX_IMMUTABLE capability.  This was detected by generic/545 test
in the fstest suite.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Luis Henriques <henrix@camandro.org>
Closes #11791
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.

2 participants