Fix nits from #2312 #2328
Fix nits from #2312 #2328
Conversation
pkg/chunked/compression_linux.go
Outdated
| @@ -160,9 +160,9 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, | |||
| } | |||
|
|
|||
| func openTmpFile(tmpDir string) (*os.File, error) { | |||
| file, err := os.OpenFile(tmpDir, unix.O_TMPFILE|unix.O_RDWR|unix.O_CLOEXEC|unix.O_EXCL, 0o600) | |||
| fd, err := unix.Open(tmpDir, unix.O_TMPFILE|unix.O_RDWR|unix.O_CLOEXEC|unix.O_EXCL, 0o600) | |||
There was a problem hiding this comment.
Back when reviewing the original commit I checked that
- os.OpenFile flags argument type is
int(i.e. a generic one); - it is passed almost as is onto syscall.Open ("almost" since O_LARGEFILE and O_CLOEXEC are added);
os.O_*,syscall.O_*, andunix.O_*flag values should be equal (although there is no guarantee);- a lot of existing code uses
unix.O_*flags withos.OpenFile;
and so I guesses it's OK as it is.
With this change, though
- we lose the ability to retry on EINTR;
- as far as I understand,
os.OpenFileadds the file to the internal poller, while the new code doesn't (which probably doesn't matter here).
All in all, I slightly prefer the old way.
There was a problem hiding this comment.
we lose the ability to retry on EINTR;
I don’t know why that should happen on a local filesystem operation
as far as I understand,
os.OpenFileadds the file to the internal poller
The Linux implementation would try to use epoll, and at least on XFS that’s not possible and fails.
There was a problem hiding this comment.
The Linux implementation would try to use epoll, and at least on XFS that’s not possible and fails.
Yes it will always fail so it shouldn't matter either way.
I routinely use strace to debug things and every time I just smile at how the Go runtime always tries and fails to add a local file to epoll, even in the case where it knows it's a regular file it just opened. It has never worked, and almost certainly never will. The modern way to do async I/O that works for local files and network sockets is io_uring. But, not relevant here. The "try and fail to add to epoll every time" though is a very Go thing to do.
I continue to think the mixing is incorrect, but there are several other pre-existing instances in the repo, and there is a unit test, so, fair enough. Dropped. |
|
Rebased, PTAL. |
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This ultimately amounts to the same thing (and os.File.Seek is a tiny bit more costly, with some atomic operations) - using os.File.Seek is more consistent with the surrounding code relying on uses through os.File, as well as a tiny bit shorter. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The callers don't really need to know, and this way we don't need to get the details of the syntax correct. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
AFAICS this does not apply to that line of code. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, kolyshkin, mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Addresses most of #2312 (review) . See individual commit messages for details.