fileutils: new function to check for path existence by giuseppe · Pull Request #1875 · containers/storage

giuseppe · 2024-04-04T14:33:01Z

On unix it uses the access syscall which is faster than stat for simply checking the existence of a file.

with a file in the cache I see on my machine:

$ go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/containers/storage/pkg/fileutils
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkPathExists-12 1000000 1037 ns/op
BenchmarkStat-12 897534 1330 ns/op

I'd expect even better results if the file is not already in the cache.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

rhatdan · 2024-04-04T18:18:59Z

pkg/fileutils/fileutils_darwin.go

 }
+
+// PathExists checks whether a file or directory exists at the given path.
+func PathExists(path string, followSymlinks bool) error {


Should the name of this function just return an err.

err:=fileutils.Exist(PATH);
err = nil -> True
err == ENOEXIST == False
err == (!nil && not ENOEXIST) == err.

Then perhaps have a fileutils.Lexist(PATH) for not follow symlinks.

I didn't change the type of the returned value because it is easier to replace the existing calls to os.Stat(). Every existing call to _, err := os.Stat(...) simply becomes err := fileutils.PathExists(..., true) and the existing code for handling err is the same.

I don't see the difference, since my function will do the same, except there is two functions, Exists, and Lexists, which is similar to Stat and Lstat.

pushed a new version with Exist and Lexist

rhatdan · 2024-04-05T08:21:30Z

LGTM, can you squash the commits.

giuseppe · 2024-04-05T11:45:15Z

I think keeping them separate helps with the review and potentially the backport. That's what @mtrmac asked me to do last time I've sent a big patch

rhatdan · 2024-04-05T11:52:12Z

Except that you are really only making a single change here. Adding two functions and then using them everywhere. I don't see any difference between the changes, so no need to split them out.

rhatdan · 2024-04-05T11:52:46Z

@mtrmac @nalind @flouthoc @saschagrunert PTAL

pkg/fileutils/fileutils_unix.go

nalind · 2024-04-05T13:53:39Z

pkg/fileutils/fileutils_darwin.go

+
+// Exist checks whether a file or directory exists at the given path.
+func Exist(path string) error {
+	_, err := os.Stat(path)


golang.org/x/sys/unix defines an Faccessat() on Darwin. What's the rationale for not using it?

I've no way to test it (and if it is worth in terms of performance)

FWIW, it works the same (the test just needs the OS check removed), and

BenchmarkExist-10 413298 2890 ns/op BenchmarkStat-10 333810 3559 ns/op

… but also, the tests of that package are now failing on macOS (because /tmp is a symlink), we are clearly not running them.

So, at this moment I’d also lean towards keeping the simplest possible implementation. (Or, alternatively, we could start testing things in CI, but I can’t build a case for doing that based on this PR alone.)

Maybe add a todo here to help future maintainers?

thanks for testing it, I will add it to Darwin too

pkg/fileutils/fileutils_test.go

pkg/fileutils/fileutils_unix.go

mtrmac

Efficiency improvements are always nice
Looking at git grep -C5 '_, .*os.Lstat' and it grep -C5 '_, .*os.Stat', there seem to be more places where this could be used — e.g. drivers/aufs, and other files in pkg/archive. That’s not blocking this PR, of course.
(Aesthetically, the name feels a bit off to me. fileutils.{Exists,Lexists} would be a small improvement, but really I’d expect a function named like that to return a bool, not an error. OTOH the returned error is clearly useful… and I can’t think of a good name. EnsurePathExists is so ugly.)
Do we want this in a ~stable public API of the module? If it is expected to be beneficial in other repos like Podman, sure, let’s make it stable. OTOH making it private would make the naming concerns much less relevant. I don’t really have an opinion here, I just want to make sure this is an intentional decision.

mtrmac · 2024-04-05T14:45:31Z

I think keeping them separate helps with the review and potentially the backport. That's what @mtrmac asked me to do last time I've sent a big patch

My personal view of this is that it is ideal if there is one idea per patch. Introduce one new feature. Fix one bug, or a homogenous class of bugs. Do one refactor. (Ideally the refactors are so trivial that in case of a merge conflict, they could just be thrown away and made again just based on the commit description.)

In that sense, the migration to the new API could well be a single commit. OTOH more commits never really hurt — and they make allocating time for reviews a tiny bit easier, because I know there are natural stopping points and I know before starting to look at that patch that I can make definite progress in 15 minutes, (as opposed to allocating an hour or two to stare at a huge do-12-things commit; that can happen only a few times a day).

Or course, everyone thinks and works a bit differently. Seeing small commits probably makes me more likely to take on the review, and to do it quickly — OTOH there is a definite cost to creating those commits in the first place.

giuseppe · 2024-04-08T10:11:10Z

Looking at git grep -C5 '_, .*os.Lstat' and it grep -C5 '_, .*os.Stat', there seem to be more places where this could be used — e.g. drivers/aufs, and other files in pkg/archive. That’s not blocking this PR, of course.

I've fixed these callers too.

(Aesthetically, the name feels a bit off to me. fileutils.{Exists,Lexists} would be a small improvement, but really I’d expect a function named like that to return a bool, not an error. OTOH the returned error is clearly useful… and I can’t think of a good name. EnsurePathExists is so ugly.)

Renamed the functions to `fileutils.{Exists, Lexists}.

My first version of the API looked like func PathExists(path string) (bool, error) but it made more difficult and error-prone to fix the current callers (if we really wanted to use the found return value). With the current API instead we can mechanically replace the current code with _, err := os.Stat(...) => err := fileutils.Exists(...) and _, err := os.Lstat(...) => => err := fileutils.Lexists(...) without changing any of the conditions in place.

Do we want this in a ~stable public API of the module? If it is expected to be beneficial in other repos like Podman, sure, let’s make it stable. OTOH making it private would make the naming concerns much less relevant. I don’t really have an opinion here, I just want to make sure this is an intentional decision.

Yes, let's make it stable and fix the same pattern in other repos.

mtrmac

LGTM; let’s maybe wait a few hours to confirm consensus on the macOS situation.

pkg/fileutils/fileutils_test.go

mtrmac

LGTM other than that one thing, feel free to merge without another review.

pkg/fileutils/fileutils_windows.go

On unix they use the access syscall which is faster than stat for simply checking the existence of a file. with a file in the cache I see on my machine: $ go test -bench=. goos: linux goarch: amd64 pkg: github.com/containers/storage/pkg/fileutils cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkExists-12 925832 1245 ns/op BenchmarkStat-12 745896 1602 ns/op PASS I'd expect even better results if the file is not already in the cache. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2024-04-08T14:50:49Z

@nalind PTAL

rhatdan · 2024-04-09T20:57:59Z

/approve
/lgtm

openshift-ci · 2024-04-09T20:58:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

giuseppe · 2024-04-09T21:31:42Z

fix for c/common: containers/common#1953

openshift-ci bot added the approved label Apr 4, 2024

giuseppe force-pushed the path-exists branch from a71473f to 4bcdf3f Compare April 4, 2024 14:50

rhatdan reviewed Apr 4, 2024

View reviewed changes

giuseppe force-pushed the path-exists branch from 4bcdf3f to 7405840 Compare April 4, 2024 21:34

nalind reviewed Apr 5, 2024

View reviewed changes

pkg/fileutils/fileutils_unix.go Outdated Show resolved Hide resolved

nalind reviewed Apr 5, 2024

View reviewed changes

mtrmac reviewed Apr 5, 2024

View reviewed changes

pkg/fileutils/fileutils_test.go Outdated Show resolved Hide resolved

pkg/fileutils/fileutils_unix.go Outdated Show resolved Hide resolved

mtrmac reviewed Apr 5, 2024

View reviewed changes

giuseppe force-pushed the path-exists branch 3 times, most recently from 872d522 to b1b8f9a Compare April 8, 2024 10:05

mtrmac reviewed Apr 8, 2024

View reviewed changes

pkg/fileutils/fileutils_test.go Outdated Show resolved Hide resolved

giuseppe force-pushed the path-exists branch from b1b8f9a to 359c1af Compare April 8, 2024 13:25

mtrmac reviewed Apr 8, 2024

View reviewed changes

pkg/fileutils/fileutils_windows.go Outdated Show resolved Hide resolved

giuseppe force-pushed the path-exists branch from 359c1af to 532b18b Compare April 8, 2024 14:01

giuseppe added 9 commits April 8, 2024 16:07

fileutils: use fileutils.{Le,E}xists()

81c2f5f

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

overlay: use fileutils.{Le,E}xists()

a6b7d66

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

idtools: use fileutils.{Le,E}xists()

171191e

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chrootarchive: use fileutils.{Le,E}xists()

9992cd2

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

types: use fileutils.{Le,E}xist()

de0900a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

drivers: use fileutils.{Le,E}xists()

383e8e3

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

archive: use fileutils.{Le,E}xists()

2b26cbf

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

vfs: use fileutils.{Le,E}xists()

1f09fad

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe added 4 commits April 8, 2024 16:07

devmapper: use fileutils.{Le,E}xists()

7bed6ea

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

btrfs: use fileutils.{Le,E}xists()

55c1550

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

aufs: use fileutils.{Le,E}xists()

74f17e7

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

windows: use fileutils.{Le,E}xists()

cbec591

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the path-exists branch from 532b18b to cbec591 Compare April 8, 2024 14:07

openshift-ci bot assigned rhatdan Apr 9, 2024

openshift-ci bot added the lgtm label Apr 9, 2024

openshift-merge-bot bot merged commit 90967d7 into containers:main Apr 9, 2024

giuseppe mentioned this pull request Apr 9, 2024

use fileutils.(Le|E)xists from containers/storage/pkg/fileutils containers/common#1953

Merged

Conversation

giuseppe commented Apr 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 5, 2024

Uh oh!

giuseppe commented Apr 5, 2024

Uh oh!

rhatdan commented Apr 5, 2024

Uh oh!

rhatdan commented Apr 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented Apr 8, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

giuseppe commented Apr 8, 2024

Uh oh!

rhatdan commented Apr 9, 2024

Uh oh!

openshift-ci bot commented Apr 9, 2024

Uh oh!

giuseppe commented Apr 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtrmac commented Apr 5, 2024 •

edited

Loading