From 5a84f72c8b0a3ecae6a83b21252f609b4775fac3 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Thu, 9 Nov 2023 15:10:23 +0200 Subject: [PATCH] Fix OpenFile confusion of flags and permissions OpenFile used to accept `fs.FileMode` twice, as in: ```go func (f *PosixFsys) OpenFile(path string, mode FileMode, perm FileMode) (*PosixFile, error) ``` The correct (or stdlib equivalent) signature is to accept an integer for the status flags and fs.FileMode for the permissions, as in: ```go func (f *PosixFsys) OpenFile(path string, flags int, perm fs.FileMode) (*PosixFile, error) ``` The OpenFile status flags are: File access: - O_RDONLY - open the file read-only. - O_WRONLY - open the file write-only. - O_RDWR - open the file read-write. File creation: - O_APPEND - append data to the file when writing. - O_CREATE - create a new file if none exists. - O_TRUNC - truncate the file if it exists - O_EXCL - used in conjunction with O_CREATE, file must not already exist. There's also O_SYNC but it is not implemented in rig, it's always sync. See https://pkg.go.dev/os#pkg-constants *Usage:* // Explanation of the flags: // - Create a new file if one doesn't exist // - Truncate an existing file if one exist (keeps // existing permissions) // - Open the file for writing fsys.OpenFile("/tmp/example", os.O_CREATE|os.O_TRUNC|os.O_WRONLY, fs.FileMode(0644)) The `rigfs.FileMode` is now retired, you're now expected to use fs.FileMode from `"io/fs"`. Signed-off-by: Kimmo Lehto --- cmd/rigtest/rigtest.go | 4 +- connection.go | 2 +- pkg/rigfs/fileinfo.go | 23 --------- pkg/rigfs/posixfsys.go | 105 +++++++++++++++++++++++++++-------------- pkg/rigfs/types.go | 15 +----- pkg/rigfs/winfsys.go | 25 +++++----- 6 files changed, 88 insertions(+), 86 deletions(-) diff --git a/cmd/rigtest/rigtest.go b/cmd/rigtest/rigtest.go index 478300db..76d387d8 100644 --- a/cmd/rigtest/rigtest.go +++ b/cmd/rigtest/rigtest.go @@ -302,7 +302,7 @@ func main() { shasum := sha256.New() reader := io.TeeReader(origin, shasum) - destf, err := fsys.OpenFile(fn, rigfs.ModeCreate, 0644) + destf, err := fsys.OpenFile(fn, goos.O_CREATE|goos.O_WRONLY, 0644) require.NoError(t, err, "open file") n, err := io.Copy(destf, reader) @@ -320,7 +320,7 @@ func main() { require.Equal(t, fmt.Sprintf("%x", shasum.Sum(nil)), destSum, "sha256 mismatch after io.copy from local to remote") - destf, err = fsys.OpenFile(fn, rigfs.ModeRead, 0644) + destf, err = fsys.OpenFile(fn, goos.O_RDONLY, 0) require.NoError(t, err, "open file for read") readSha := sha256.New() diff --git a/connection.go b/connection.go index b1da63d2..be219637 100644 --- a/connection.go +++ b/connection.go @@ -372,7 +372,7 @@ func (c *Connection) Upload(src, dst string, _ ...exec.Option) error { shasum := sha256.New() fsys := c.Fsys() - remote, err := fsys.OpenFile(dst, rigfs.ModeCreate, rigfs.FileMode(stat.Mode())) + remote, err := fsys.OpenFile(dst, os.O_CREATE|os.O_WRONLY, stat.Mode()) if err != nil { return fmt.Errorf("%w: open remote file %s for writing: %w", ErrInvalidPath, dst, err) } diff --git a/pkg/rigfs/fileinfo.go b/pkg/rigfs/fileinfo.go index fc171caa..2d5fad39 100644 --- a/pkg/rigfs/fileinfo.go +++ b/pkg/rigfs/fileinfo.go @@ -36,29 +36,6 @@ func (f *FileInfo) UnmarshalJSON(b []byte) error { } f.FModTime = time.Unix(f.ModtimeS, 0) f.FName = strings.ReplaceAll(f.FName, "\\", "/") - - var newmode fs.FileMode - - if f.FMode&1 != 0 { // "Readonly" - newmode = 0o444 - } - if f.FMode&16 != 0 { // "Directory" - newmode |= fs.ModeDir | 0o544 - } - if f.FMode&64 != 0 { // "Device" - newmode |= fs.ModeCharDevice - } - if f.FMode&4096 != 0 { // "Offline" - newmode |= fs.ModeIrregular - } - if f.FMode&1024 != 0 { // "ReparsePoint" - newmode |= fs.ModeIrregular - newmode |= fs.ModeSymlink - } - if f.FMode&256 != 0 { // "Temporary" - newmode |= fs.ModeTemporary - } - f.FMode = newmode return nil } diff --git a/pkg/rigfs/posixfsys.go b/pkg/rigfs/posixfsys.go index 74b42aef..1576b729 100644 --- a/pkg/rigfs/posixfsys.go +++ b/pkg/rigfs/posixfsys.go @@ -4,6 +4,7 @@ import ( "bytes" _ "embed" "encoding/json" + "errors" "fmt" "io" "io/fs" @@ -11,7 +12,6 @@ import ( "path/filepath" "strconv" "strings" - "time" "github.com/alessio/shellescape" "github.com/k0sproject/rig/exec" @@ -40,7 +40,10 @@ func NewPosixFsys(conn connection, opts ...exec.Option) *PosixFsys { return &PosixFsys{conn: conn, opts: opts} } -const defaultBlockSize = 4096 +const ( + defaultBlockSize = 4096 + supportedFlags = os.O_RDONLY | os.O_WRONLY | os.O_RDWR | os.O_CREATE | os.O_EXCL | os.O_TRUNC | os.O_APPEND | os.O_SYNC +) // PosixFile implements fs.File for a remote file type PosixFile struct { @@ -50,7 +53,8 @@ type PosixFile struct { isEOF bool pos int64 size int64 - mode FileMode + mode fs.FileMode + flags int blockSize int } @@ -106,11 +110,11 @@ func (f *PosixFile) fsBlockSize() int { } func (f *PosixFile) isReadable() bool { - return f.mode&ModeRead != 0 + return f.isOpen && (f.flags&os.O_WRONLY != os.O_WRONLY || f.flags&os.O_RDWR == os.O_RDWR) } func (f *PosixFile) isWritable() bool { - return f.mode&ModeWrite != 0 + return f.isOpen && f.flags&os.O_WRONLY != 0 } func (f *PosixFile) ddParams(offset int64, numBytes int) (int, int64, int) { @@ -357,50 +361,81 @@ func (fsys *PosixFsys) Sha256(name string) (string, error) { // Open opens the named file for reading. func (fsys *PosixFsys) Open(name string) (fs.File, error) { - info, err := fsys.Stat(name) - if err != nil { - return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrNotExist} + return fsys.OpenFile(name, os.O_RDONLY, 0) +} + +func (fsys *PosixFsys) createFile(name string, perm fs.FileMode) error { + if _, err := fsys.helper("touch", name); err != nil { + return err } - file := PosixFile{fsys: fsys, path: name, isOpen: true, size: info.Size(), mode: ModeRead} - if info.IsDir() { - return &PosixDir{PosixFile: file}, nil + + if _, err := fsys.helper("chmod", name, fmt.Sprintf("%#o", perm)); err != nil { + return err } - return &file, nil + return nil } -// OpenFile is the generalized open call; most users will use Open instead.. The perms are ignored if the file exists. -func (fsys *PosixFsys) OpenFile(name string, mode FileMode, perm FileMode) (File, error) { +// OpenFile is used to open a file with access/creation flags for reading or writing. For info on flags, +// see https://pkg.go.dev/os#pkg-constants +func (fsys *PosixFsys) OpenFile(name string, flags int, perm fs.FileMode) (File, error) { //nolint:cyclop + if flags&^supportedFlags != 0 { + return nil, fmt.Errorf("%w: unsupported flags: %d", ErrCommandFailed, flags) + } + var pos int64 info, err := fsys.Stat(name) - if err != nil { - // file exists - switch { - case mode&ModeRead == ModeRead: + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + fileExists := err == nil + + switch fileExists { + case false: + switch flags & os.O_CREATE { + case 0: return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrNotExist} - case mode&ModeCreate == ModeCreate: - if _, err := fsys.helper("touch", name); err != nil { + default: + if err := fsys.createFile(name, perm); err != nil { return nil, err } - if _, err := fsys.helper("chmod", name, fmt.Sprintf("%#o", perm)); err != nil { + + // re-stat to ensure file is now there and get the correct bits if there's a umask + i, err := fsys.Stat(name) + if err != nil { return nil, err } + info = i } - info = &FileInfo{FName: name, FMode: fs.FileMode(perm), FSize: 0, FIsDir: false, FModTime: time.Now(), fsys: fsys} - } - if info.IsDir() { - return nil, &fs.PathError{Op: "open", Path: name, Err: fmt.Errorf("%w: is a directory", fs.ErrPermission)} - } - switch { - case mode&ModeAppend == ModeAppend: - pos = info.Size() - case mode&ModeCreate == ModeCreate: - if info.Size() > 0 { + case true: + switch { + case info.IsDir(): + return nil, &fs.PathError{Op: "open", Path: name, Err: fmt.Errorf("%w: is a directory", fs.ErrInvalid)} + case flags&(os.O_CREATE|os.O_EXCL) == (os.O_CREATE | os.O_EXCL): + return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrExist} + case flags&os.O_TRUNC != 0: if _, err := fsys.helper("truncate", name, "0"); err != nil { return nil, err } + i, err := fsys.Stat(name) + if err != nil { + return nil, err + } + info = i + case flags&os.O_APPEND != 0: + pos = info.Size() } } - return &PosixFile{fsys: fsys, path: name, isOpen: true, size: info.Size(), pos: pos, mode: mode}, nil + + f := &PosixFile{ + fsys: fsys, + path: name, + isOpen: true, + size: info.Size(), + pos: pos, + mode: info.Mode(), + flags: flags, + } + return f, nil } // ReadDir reads the directory named by dirname and returns a list of directory entries @@ -440,7 +475,7 @@ func (fsys *PosixFsys) RemoveAll(name string) error { // MkDirAll creates a new directory structure with the specified name and permission bits. // If the directory already exists, MkDirAll does nothing and returns nil. -func (fsys *PosixFsys) MkDirAll(name string, perm FileMode) error { +func (fsys *PosixFsys) MkDirAll(name string, perm fs.FileMode) error { dir := shellescape.Quote(name) if existing, err := fsys.Stat(name); err == nil { if existing.IsDir() { @@ -449,11 +484,11 @@ func (fsys *PosixFsys) MkDirAll(name string, perm FileMode) error { return fmt.Errorf("%w: mkdir %s: %w", ErrCommandFailed, name, fs.ErrExist) } - if err := fsys.conn.Exec(fmt.Sprintf("mkdir -p %s", dir), fsys.opts...); err != nil { + if err := fsys.conn.Exec(fmt.Sprintf("mkdir -p %s", shellescape.Quote(dir)), fsys.opts...); err != nil { return fmt.Errorf("%w: mkdir %s: %w", ErrCommandFailed, name, err) } - if err := fsys.conn.Exec(fmt.Sprintf("chmod %#o %s", os.FileMode(perm).Perm(), dir), fsys.opts...); err != nil { + if err := fsys.conn.Exec(fmt.Sprintf("chmod %#o %s", perm, shellescape.Quote(dir)), fsys.opts...); err != nil { return fmt.Errorf("%w: chmod (mkdir) %s: %w", ErrCommandFailed, name, err) } diff --git a/pkg/rigfs/types.go b/pkg/rigfs/types.go index a1ee13e0..975953e0 100644 --- a/pkg/rigfs/types.go +++ b/pkg/rigfs/types.go @@ -35,21 +35,10 @@ type File interface { // Fsys is a filesystem on the remote host type Fsys interface { fs.FS - OpenFile(path string, mode FileMode, perm FileMode) (File, error) + OpenFile(path string, flag int, perm fs.FileMode) (File, error) Sha256(path string) (string, error) Stat(path string) (fs.FileInfo, error) Remove(path string) error RemoveAll(path string) error - MkDirAll(path string, perm FileMode) error + MkDirAll(path string, perm fs.FileMode) error } - -// FileMode is used to set the type of allowed operations when opening remote files -type FileMode int - -const ( - ModeRead FileMode = 1 // ModeRead = Read only - ModeWrite FileMode = 2 // ModeWrite = Write only - ModeReadWrite FileMode = ModeRead | ModeWrite // ModeReadWrite = Read and Write - ModeCreate FileMode = 4 | ModeWrite // ModeCreate = Create a new file or truncate an existing one. Includes write permission. - ModeAppend FileMode = 8 | ModeCreate // ModeAppend = Append to an existing file. Includes create and write permissions. -) diff --git a/pkg/rigfs/winfsys.go b/pkg/rigfs/winfsys.go index 23e40611..6d5d011c 100644 --- a/pkg/rigfs/winfsys.go +++ b/pkg/rigfs/winfsys.go @@ -352,29 +352,30 @@ func (f *winFile) Close() error { // Use OpenFile to get a file that can be written to or if you need any of the methods not // available on fs.File interface without type assertion. func (fsys *WinFsys) Open(name string) (fs.File, error) { - f, err := fsys.OpenFile(name, ModeRead, 0o644) + f, err := fsys.OpenFile(name, os.O_RDONLY, 0) if err != nil { return nil, err } return f, nil } -// OpenFile opens the named remote file with the specified FileMode. Permission bits are ignored on Windows. -func (fsys *WinFsys) OpenFile(name string, mode FileMode, _ FileMode) (File, error) { +// OpenFile opens the named remote file with the specified flags. os.O_EXCL and permission bits are ignored on Windows. +// For a description of the flags, see https://pkg.go.dev/os#pkg-constants +func (fsys *WinFsys) OpenFile(name string, flags int, _ fs.FileMode) (File, error) { var modeStr string - switch mode { - case ModeRead: + switch { + case flags&os.O_RDONLY == os.O_RDONLY: modeStr = "ro" - case ModeWrite: + case flags&os.O_WRONLY == os.O_WRONLY: modeStr = "w" - case ModeReadWrite: + case flags&os.O_RDWR == os.O_RDWR: modeStr = "rw" - case ModeAppend: + case flags&os.O_APPEND == os.O_APPEND: modeStr = "a" - case ModeCreate: + case flags&os.O_CREATE == os.O_CREATE: modeStr = "c" default: - return nil, &fs.PathError{Op: "open", Path: name, Err: fmt.Errorf("%w: invalid mode: %d", ErrRcpCommandFailed, mode)} + return nil, &fs.PathError{Op: "open", Path: name, Err: fmt.Errorf("%w: invalid mode: %d", ErrRcpCommandFailed, flags)} } log.Debugf("opening remote file %s (mode %s)", name, modeStr) @@ -463,8 +464,8 @@ func (fsys *WinFsys) removeDirAll(name string) error { return nil } -// MkDirAll creates a directory named path, along with any necessary parents. The permission bits perm are ignored on Windows. -func (fsys *WinFsys) MkDirAll(name string, _ FileMode) error { +// MkDirAll creates a directory named path, along with any necessary parents. The permission bits are ignored on Windows. +func (fsys *WinFsys) MkDirAll(name string, _ fs.FileMode) error { if err := fsys.conn.Exec(fmt.Sprintf("mkdir -p %s", ps.DoubleQuote(name))); err != nil { return fmt.Errorf("%w: mkdir %s: %w", ErrCommandFailed, name, err) }