chunked: refactor file system ops in a new file and add some tests #1952
Conversation
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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 |
|
@cgwalters @rhatdan PTAL |
giuseppe
force-pushed
the
refactor-chunked-package
branch
from
d47e038
to
8187286
Compare
| @@ -304,7 +304,7 @@ func openFileUnderRoot(dirfd int, name string, flags uint64, mode os.FileMode) ( | |||
| if errors.Is(err, unix.ENOENT) && hasCreate { | |||
| parent := filepath.Dir(name) | |||
| if parent != "" { | |||
| newDirfd, err2 := openOrCreateDirUnderRoot(parent, dirfd, 0) | |||
| newDirfd, err2 := openOrCreateDirUnderRoot(dirfd, parent, 0) | |||
There was a problem hiding this comment.
On a related topic, wouldn't it make sense to create a struct wrapper for the dirfd, and have things be methods on it?
In my Rust code, we extensively use https://docs.rs/cap-std/latest/cap_std/ which is related, and has a Dir struct with a lot of methods, but in implementation it's just:
There was a problem hiding this comment.
I think there are more redundancy/parameter minimization opportunities (isn’t mode often redundant with metadata.Mode? linters report unused parameters) but I think those shouldn’t block the improvements from this PR.
There was a problem hiding this comment.
we need an explicit mode to support ForceMask
pkg/chunked/filesystem_linux_test.go
Outdated
|
|
||
| destDir := createTempDir(t) | ||
| defer os.RemoveAll(destDir) | ||
| destDirFd, err := syscall.Open(destDir, syscall.O_RDONLY, 0) |
There was a problem hiding this comment.
Hello, your friendly O_CLOEXEC linter reporting in here 😄
pkg/chunked/filesystem_linux.go
Outdated
|
|
||
| if err := doChown(); err != nil { | ||
| if !options.IgnoreChownErrors { | ||
| return &fs.PathError{Op: "chown", Path: metadata.Name, Err: err} |
There was a problem hiding this comment.
… and, symmetrically to how os.Stat returns a PathError, I’d expect the PathError creation to happen at a fairly low level.
I don’t know how I feel about a PathError reporting a Path which is not exactly what we used.
Also, the commit says “improve error messages”. but it removes information about non-path parameters to the operation. Is that intentional?
(I didn’t review more parts of the “improve error messages” commit in detail.)
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
follow the same pattern used by other functions. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
follow the same pattern used by other functions Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
refactor-chunked-package
branch
from
8187286
to
4026106
Compare
|
thanks for the comments and suggestions. Addressed them and pushed a new version |
giuseppe
force-pushed
the
refactor-chunked-package
branch
from
4026106
to
8aff803
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
refactor-chunked-package
branch
2 times, most recently
from
36120f0
to
541ad3b
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
refactor-chunked-package
branch
from
541ad3b
to
42801b2
Compare
more details in each commit