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

libindex: add fallback path for O_TMPFILE not being supported #1292

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

hdonnay
Copy link
Member

@hdonnay hdonnay commented Mar 18, 2024

No description provided.

@hdonnay hdonnay force-pushed the hack/o_tmpfile branch 3 times, most recently from e493183 to ff437ae Compare March 19, 2024 19:35
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 55.87%. Comparing base (bc8d3a3) to head (fedb9d3).

Files Patch % Lines
libindex/tempfile_linux.go 75.00% 6 Missing and 1 partial ⚠️
libindex/tempdir_unix.go 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1292      +/-   ##
==========================================
+ Coverage   55.82%   55.87%   +0.04%     
==========================================
  Files         265      266       +1     
  Lines       16597    16627      +30     
==========================================
+ Hits         9266     9290      +24     
- Misses       6367     6374       +7     
+ Partials      964      963       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hdonnay hdonnay force-pushed the hack/o_tmpfile branch 3 times, most recently from 899da53 to 13cb984 Compare March 20, 2024 18:42
@hdonnay hdonnay marked this pull request as ready for review March 20, 2024 19:03
@hdonnay hdonnay requested a review from a team as a code owner March 20, 2024 19:03
@hdonnay hdonnay requested review from crozzy and removed request for a team March 20, 2024 19:03
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

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

Nice, just a couple of clarifying comments

libindex/tempdir_unix.go Outdated Show resolved Hide resolved
libindex/tempfile_linux.go Outdated Show resolved Hide resolved
@hdonnay hdonnay force-pushed the hack/o_tmpfile branch 2 times, most recently from 15d0212 to 313f0ed Compare March 21, 2024 00:28
@crozzy crozzy self-requested a review March 21, 2024 20:08
@crozzy crozzy added the needs-changelog Label for PRs that need a changelog note. label Mar 21, 2024
Comment on lines +67 to +68
// On OSX, temporary files are not unlinked from the filesystem upon creation,
// because an equivalent to Linux's "/proc/self/fd" doesn't seem to exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hdonnay, do you mind elaborating on this or rephrasing the comment? I'm having a hard time grokking it.

... temporary files are not unlinked from the filesystem upon creation,
because an equivalent to Linux's "/proc/self/fd" doesn't seem to exist.

I couldn't find a reason "/proc/self/fd" was required for "unlinking" temporary files. I'm also not sure what unlinking in this context means; my assumption based on the word choice would be that the implementation does some sort of symlinking and we need to unlink them at some point, but I don't think that's the case.

temporary files are not unlinked from the filesystem upon creation

I'm particularly confused by this. If we're on Linux, do other temporary files get unlinked when we create our temporary files?


fwiw, I'm not sure if others do this, but I know Ross and I both run the ACS Scanner on macOS devices, so sorting this out would help us out a lot (in other words, I'm not trying to be pedantic with this comment 🙂)

Copy link
Contributor

Choose a reason for hiding this comment

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

(also happy to move this discussion to another PR or elsewhere if we want to unblock merging this)

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries.

"Unlink" in this context is a filesystem/syscall term-of-art: it means making a name no longer visible. This isn't exactly deleting a file because the semantics change based on the the type of object, the underlying filesystem, and outstanding file descriptions. Out of a combination of battle scars and habit, I tend to say "unlink" when I'm specifically thinking of unlink(2).

With regards to /proc/self/fd, that's a directory containing magic symlinks (actual terminology, see symlink(7)) representing open file descriptors. Opening one of them creates a new file descriptor backed by a new file description (see the NOTES section of open(2)). This means that unlike dup(2), the uses of the two file descriptors are completely independent (e.g. seeking one will not change the offset on the other).

The Linux code uses this to open files that aren't linked into the filesystem (meaning they'll never outlast the process, because only the process has them open) but can still be reopened as many times as needed to create independent file descriptors.

AFAICT, there's not a way to do this on OSX. Looking at FreeBSD man pages, fdescfs(5) acts the same way but only if mounted with nodup, which I don't think is the default. There is mention on that manpage about a O_EMPTY_PATH to openat(2), but I can't find any information on whether that exists on OSX and don't have a machine to test with. To work around this, the code simply doesn't unlink the file and re-opens it by name as needed. The downside is that the files are only removed as long as the program shuts down cleanly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the detailed explanation! I need to dive deeper into all this info, so please feel free to consider this discussion non-blocking for the PR

@crozzy crozzy self-requested a review March 25, 2024 17:54
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

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

LGTM passing baton to @BradLugo

@crozzy crozzy requested review from crozzy and removed request for crozzy March 25, 2024 17:55
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
Copy link
Member Author

hdonnay commented Mar 25, 2024

will merge with green CI

@hdonnay
Copy link
Member Author

hdonnay commented Mar 25, 2024

/fast-forward

@github-actions github-actions bot merged commit fedb9d3 into quay:main Mar 25, 2024
8 checks passed
@hdonnay hdonnay deleted the hack/o_tmpfile branch March 25, 2024 21:43
@crozzy crozzy added needs-changelog Label for PRs that need a changelog note. and removed needs-changelog Label for PRs that need a changelog note. labels Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

O_TMPFILE on Linux with non-supporting kernel ("/tmp: Operation not supported")
3 participants