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

Update tmpfile() existence detection #12087

Merged
merged 1 commit into from May 20, 2021
Merged

Conversation

rincebrain
Copy link
Contributor

@rincebrain rincebrain commented May 20, 2021

Motivation and Context

Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure, leading to
#12060 .

Description

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate based on the new detection.

How Has This Been Tested?

  • Verified that O_TMPFILE failed with EOPNOTSUPP on ZFS on Debian bullseye, kernel 5.12.0 with vanilla git and passed with this PR.
  • Verified that O_TMPFILE still works as expected on the aforementioned system running a 5.10.0 kernel
  • Verified same on Ubuntu 18.04 box running 4.15.0-143-generic
  • Verified that all the above systems pass all the tmpfile-related tests in ZTS

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:

Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Closes: openzfs#12060

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@dankamongmen
Copy link
Contributor

Hey there. I filed the original bug (#12060), after running into it as part of dankamongmen/notcurses#1653. Just wanted to say that I tested a rebuilt zfs-2.1.99-1, and I'm no longer drawing ENOTSUPP. The regression appears remedied. Thanks for running with it, @rincebrain!

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.

Whoops. This looks good, it seems we missed this case when updating the rest of the VFS hooks in #11712. Thanks for sorting it out.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label May 20, 2021
@rincebrain
Copy link
Contributor Author

I suppose it would be a fair bit of work to refactor many of the kernel version checks to say "if ([target kernel version] >= [expected version for this check to start passing] && [target kernel version] < [version where the API changed again] && [this test returned false]), hard fail the configure", even if people were somehow okay with making Python a hard dependency and I stole the version comparison from #12073, to reduce the chances of something like this silently happening again.

@behlendorf
Copy link
Contributor

Thankfully this kind of thing doesn't happen all that often, usually it's caught by the CI. In this particular case it slipped through because the tmpfile() ZTS tests were written in such a way that they automatically disable themselves when the kernel support for O_TMPFILE is missing (it was added in the 3.11 kernel). See tests/zfs-tests/tests/functional/tmpfile/setup.ksh, we could make that check smarter potentially for this specific case. Though linking any of these checks to a specific kernel version can be dodgy too since distributions have been known to change kernel interfaces and backport patches from upstream. This is mainly why we rely on autoconf to check the actual devel headers to determine what is and isn't available.

@rincebrain
Copy link
Contributor Author

Sure, I understand why this particular case didn't get caught, but that's the point - my thought was "distinguishing between "feature absent and kernel too old" and "feature absent when kernel should have it"".

So if we're checking for "newer than 3.0 and the test fails", and someone backported to 2.6.20, that's fine, this hypothetical test couldn't catch it failing unexpectedly, but (almost) nothing short of the Linux kernel versioning its interfaces would let you know that "oh, it's broken because it's too new, not because it's too old" in those cases. (I guess you could try to enforce always having a test case for every variant of a feature you support, e.g. in this case adding a test after the other tmpfile checks that verifies the error that gets spit back for trying to define .tmpfile is "no such member" not "incorrect argument types" (or however the compilers would phrase it), but down that road madness and terrible parsing lies.)

Though that would involve parsing version numbers...again, but with no eventual Python packaging escape hatches unless it was made a hard dep {for this hypothetical check, for everything}, which I'm not a fan of as an idea. (Though if the checks only applied in the case that it was present, maybe that'd work...)

It was all a hypothetical anyway. I'm unlikely to go down a rabbit hole trying to build this unless I notice it breaking like this again.

rincebrain added a commit to rincebrain/zfs that referenced this pull request May 20, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Wrap zpl_set_acl with zpl_set_acl2, and add a new configure test for
the new version.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
rincebrain added a commit to rincebrain/zfs that referenced this pull request May 20, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Wrap zpl_set_acl with zpl_set_acl2, and add a new configure test for
the new version.

Closes: openzfs#12076

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@behlendorf behlendorf merged commit 0b1b66b into openzfs:master May 20, 2021
rincebrain added a commit to rincebrain/zfs that referenced this pull request May 22, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Closes: openzfs#12076

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
rincebrain added a commit to rincebrain/zfs that referenced this pull request May 22, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Closes: openzfs#12076

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
rincebrain added a commit to rincebrain/zfs that referenced this pull request May 26, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Closes: openzfs#12076

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
behlendorf pushed a commit that referenced this pull request May 27, 2021
Just like #12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused #12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes #12076
Closes #12093
BtbN pushed a commit to BtbN/zfs that referenced this pull request May 27, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12060
Closes: openzfs#12087
BtbN pushed a commit to BtbN/zfs that referenced this pull request May 27, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12076
Closes openzfs#12093
BtbN pushed a commit to BtbN/zfs that referenced this pull request May 27, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12060
Closes: openzfs#12087
BtbN pushed a commit to BtbN/zfs that referenced this pull request May 27, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12076
Closes openzfs#12093
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12060
Closes: openzfs#12087
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12076
Closes openzfs#12093
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12060
Closes: openzfs#12087
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12076
Closes openzfs#12093
tonyhutter pushed a commit that referenced this pull request Jun 1, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: #12060
Closes: #12087
tonyhutter pushed a commit that referenced this pull request Jun 1, 2021
Just like #12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused #12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes #12076
Closes #12093
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jun 2, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12060
Closes: openzfs#12087
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jun 2, 2021
Just like openzfs#12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused openzfs#12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12076
Closes openzfs#12093
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
Linux changed the tmpfile() signature again in torvalds/linux@6521f89,
which in turn broke our HAVE_TMPFILE detection in configure.

Update that macro to include the new case, and change the signature of
zpl_tmpfile as appropriate.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: #12060
Closes: #12087
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
Just like #12087, the set_acl signature changed with all the bolted-on
*userns parameters, which disabled set_acl usage, and caused #12076.

Turn zpl_set_acl into zpl_set_acl and zpl_set_acl_impl, and add a
new configure test for the new version.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes #12076
Closes #12093
@rincebrain rincebrain deleted the tmpfile_5x branch October 23, 2021 05:54
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.

None yet

3 participants