From 1df3fa69977477092efa65a8de407bd6f0f88db4 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Mon, 1 Apr 2019 17:28:54 -0700 Subject: Automated rollback of changelist 240657604 PiperOrigin-RevId: 241434161 Change-Id: I9ec734e50cef5b39203e8bf37de2d91d24943f1e --- runsc/fsgofer/fsgofer.go | 218 +++++++++++++++++++++++------------------- runsc/fsgofer/fsgofer_test.go | 168 ++++++++++++++++++-------------- 2 files changed, 212 insertions(+), 174 deletions(-) diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index 1d64458e5..45b455430 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -176,23 +176,21 @@ func (a *attachPoint) makeQID(stat syscall.Stat_t) p9.QID { } // localFile implements p9.File wrapping a local file. The underlying file -// is opened during Walk() and stored in 'controlFile' to be used with other -// operations. The control file is opened as readonly, unless it's a symlink -// which requires O_PATH. 'controlFile' is dup'ed when Walk(nil) is called -// to clone the file. This reduces the number of walks that need to be done by -// the host file system when files are reused. +// is opened during Walk() and stored in 'file' to be used with other +// operations. The file is opened as readonly, unless it's a symlink or there is +// no read access, which requires O_PATH. 'file' is dup'ed when Walk(nil) is +// called to clone the file. This reduces the number of walks that need to be +// done by the host file system when files are reused. // -// 'openedFile' is assigned when Open() is called. If requested open mode is -// a subset of controlFile's mode, it's possible to use the same file. If mode -// is not a subset, then another file is opened. Consequently, 'openedFile' -// could have a mode wider than requested and must be verified before read/write -// operations. Before the file is opened and after it's closed, 'mode' is set to -// an invalid value to prevent an unopened file from being used. +// The file may be reopened if the requested mode in Open() is not a subset of +// current mode. Consequently, 'file' could have a mode wider than requested and +// must be verified before read/write operations. Before the file is opened and +// after it's closed, 'mode' is set to an invalid value to prevent an unopened +// file from being used. // -// The reason that the control file is never opened as read-write is for better +// The reason that the file is not opened initially as read-write is for better // performance with 'overlay2' storage driver. overlay2 eagerly copies the // entire file up when it's opened in write mode, and would perform badly when -// multiple files are being opened for read-only (esp. startup). type localFile struct { p9.DefaultWalkGetAttr @@ -202,12 +200,9 @@ type localFile struct { // hostPath will be safely updated by the Renamed hook. hostPath string - // controlFile is opened when localFile is created and it's never nil. - controlFile *os.File - - // openedFile is nil until localFile is opened. It may point to controlFile - // or be a new file struct. See struct comment for more details. - openedFile *os.File + // file is opened when localFile is created and it's never nil. It may be + // reopened... + file *os.File // mode is the mode in which the file was opened. Set to invalidMode // if localFile isn't opened. @@ -220,7 +215,22 @@ type localFile struct { readDirMu sync.Mutex } -func openAnyFile(parent *localFile, name string) (*os.File, string, error) { +func openAnyFileFromParent(parent *localFile, name string) (*os.File, string, error) { + path := path.Join(parent.hostPath, name) + f, err := openAnyFile(path, func(mode int) (*os.File, error) { + fd, err := syscall.Openat(parent.fd(), name, openFlags|mode, 0) + if err != nil { + return nil, err + } + return os.NewFile(uintptr(fd), path), nil + }) + return f, path, err +} + +// openAnyFile attempts to open the file in O_RDONLY and if it fails fallsback +// to O_PATH. 'path' is used for logging messages only. 'fn' is what does the +// actual file open and is customizable by the caller. +func openAnyFile(path string, fn func(mode int) (*os.File, error)) (*os.File, error) { // Attempt to open file in the following mode in order: // 1. RDONLY | NONBLOCK: for all files, works for directories and ro mounts too. // Use non-blocking to prevent getting stuck inside open(2) for FIFOs. This option @@ -229,9 +239,9 @@ func openAnyFile(parent *localFile, name string) (*os.File, string, error) { modes := []int{syscall.O_RDONLY | syscall.O_NONBLOCK, unix.O_PATH} var err error - var fd int + var file *os.File for i, mode := range modes { - fd, err = syscall.Openat(parent.controlFD(), name, openFlags|mode, 0) + file, err = fn(mode) if err == nil { // openat succeeded, we're done. break @@ -239,20 +249,19 @@ func openAnyFile(parent *localFile, name string) (*os.File, string, error) { switch e := extractErrno(err); e { case syscall.ENOENT: // File doesn't exist, no point in retrying. - return nil, "", e + return nil, e } // openat failed. Try again with next mode, preserving 'err' in case this // was the last attempt. - log.Debugf("Attempt %d to open file failed, mode: %#x, path: %s/%s, err: %v", i, openFlags|mode, parent.controlFile.Name(), name, err) + log.Debugf("Attempt %d to open file failed, mode: %#x, path: %q, err: %v", i, openFlags|mode, path, err) } if err != nil { // All attempts to open file have failed, return the last error. - log.Debugf("Failed to open file, path: %s/%s, err: %v", parent.controlFile.Name(), name, err) - return nil, "", extractErrno(err) + log.Debugf("Failed to open file, path: %q, err: %v", path, err) + return nil, extractErrno(err) } - newPath := path.Join(parent.hostPath, name) - return os.NewFile(uintptr(fd), newPath), newPath, nil + return file, nil } func getSupportedFileType(stat syscall.Stat_t) (fileType, error) { @@ -279,7 +288,7 @@ func newLocalFile(a *attachPoint, file *os.File, path string, stat syscall.Stat_ return &localFile{ attachPoint: a, hostPath: path, - controlFile: file, + file: file, mode: invalidMode, ft: ft, }, nil @@ -314,33 +323,26 @@ func fchown(fd int, uid p9.UID, gid p9.GID) error { return syscall.Fchownat(fd, "", int(uid), int(gid), linux.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW) } -func (l *localFile) controlFD() int { - return int(l.controlFile.Fd()) -} - -func (l *localFile) openedFD() int { - if l.openedFile == nil { - panic(fmt.Sprintf("trying to use an unopened file: %q", l.controlFile.Name())) - } - return int(l.openedFile.Fd()) +func (l *localFile) fd() int { + return int(l.file.Fd()) } // Open implements p9.File. func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) { - if l.openedFile != nil { - panic(fmt.Sprintf("attempting to open already opened file: %q", l.controlFile.Name())) + if l.isOpen() { + panic(fmt.Sprintf("attempting to open already opened file: %q", l.file.Name())) } // Check if control file can be used or if a new open must be created. var newFile *os.File if mode == p9.ReadOnly { - log.Debugf("Open reusing control file, mode: %v, %q", mode, l.controlFile.Name()) - newFile = l.controlFile + log.Debugf("Open reusing control file, mode: %v, %q", mode, l.file.Name()) + newFile = l.file } else { // Ideally reopen would call name_to_handle_at (with empty name) and // open_by_handle_at to reopen the file without using 'hostPath'. However, // name_to_handle_at and open_by_handle_at aren't supported by overlay2. - log.Debugf("Open reopening file, mode: %v, %q", mode, l.controlFile.Name()) + log.Debugf("Open reopening file, mode: %v, %q", mode, l.file.Name()) var err error newFile, err = os.OpenFile(l.hostPath, openFlags|mode.OSFlags(), 0) @@ -351,7 +353,9 @@ func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) { stat, err := stat(int(newFile.Fd())) if err != nil { - newFile.Close() + if newFile != l.file { + newFile.Close() + } return nil, p9.QID{}, 0, extractErrno(err) } @@ -361,8 +365,13 @@ func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) { fd = newFDMaybe(newFile) } - // Set fields on success - l.openedFile = newFile + // Close old file in case a new one was created. + if newFile != l.file { + if err := l.file.Close(); err != nil { + log.Warningf("Error closing file %q: %v", l.file.Name(), err) + } + l.file = newFile + } l.mode = mode return fd, l.attachPoint.makeQID(stat), 0, nil } @@ -377,10 +386,9 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid return nil, nil, p9.QID{}, 0, syscall.EBADF } - // Use a single file for both 'controlFile' and 'openedFile'. Mode must - // include read for control and whichever else was requested by caller. Note - // that resulting file might have a wider mode than needed for each particular - // case. + // 'file' may be used for other operations (e.g. Walk), so read access is + // always added to flags. Note that resulting file might have a wider mode + // than needed for each particular case. flags := openFlags | syscall.O_CREAT | syscall.O_EXCL if mode == p9.WriteOnly { flags |= syscall.O_RDWR @@ -388,14 +396,14 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid flags |= mode.OSFlags() } - fd, err := syscall.Openat(l.controlFD(), name, flags, uint32(perm.Permissions())) + fd, err := syscall.Openat(l.fd(), name, flags, uint32(perm.Permissions())) if err != nil { return nil, nil, p9.QID{}, 0, extractErrno(err) } cu := specutils.MakeCleanup(func() { syscall.Close(fd) // Best effort attempt to remove the file in case of failure. - if err := syscall.Unlinkat(l.controlFD(), name); err != nil { + if err := syscall.Unlinkat(l.fd(), name); err != nil { log.Warningf("error unlinking file %q after failure: %v", path.Join(l.hostPath, name), err) } }) @@ -414,13 +422,12 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid c := &localFile{ attachPoint: l.attachPoint, hostPath: cPath, - controlFile: f, - openedFile: f, + file: f, mode: mode, } cu.Release() - return newFDMaybe(c.openedFile), c, l.attachPoint.makeQID(stat), 0, nil + return newFDMaybe(c.file), c, l.attachPoint.makeQID(stat), 0, nil } // Mkdir implements p9.File. @@ -433,12 +440,12 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID) return p9.QID{}, syscall.EBADF } - if err := syscall.Mkdirat(l.controlFD(), name, uint32(perm.Permissions())); err != nil { + if err := syscall.Mkdirat(l.fd(), name, uint32(perm.Permissions())); err != nil { return p9.QID{}, extractErrno(err) } cu := specutils.MakeCleanup(func() { // Best effort attempt to remove the dir in case of failure. - if err := unix.Unlinkat(l.controlFD(), name, unix.AT_REMOVEDIR); err != nil { + if err := unix.Unlinkat(l.fd(), name, unix.AT_REMOVEDIR); err != nil { log.Warningf("error unlinking dir %q after failure: %v", path.Join(l.hostPath, name), err) } }) @@ -446,7 +453,7 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID) // Open directory to change ownership and stat it. flags := syscall.O_DIRECTORY | syscall.O_RDONLY | openFlags - fd, err := syscall.Openat(l.controlFD(), name, flags, 0) + fd, err := syscall.Openat(l.fd(), name, flags, 0) if err != nil { return p9.QID{}, extractErrno(err) } @@ -468,20 +475,34 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID) func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) { // Duplicate current file if 'names' is empty. if len(names) == 0 { - newFd, err := syscall.Dup(l.controlFD()) - if err != nil { - return nil, nil, extractErrno(err) + var newFile *os.File + if l.isOpen() { + // File mode may have changed when it was opened, so open a new one. + var err error + newFile, err = openAnyFile(l.hostPath, func(mode int) (*os.File, error) { + return os.OpenFile(l.hostPath, openFlags|mode, 0) + }) + if err != nil { + return nil, nil, extractErrno(err) + } + } else { + newFd, err := syscall.Dup(l.fd()) + if err != nil { + return nil, nil, extractErrno(err) + } + newFile = os.NewFile(uintptr(newFd), l.hostPath) } - stat, err := stat(newFd) + + stat, err := stat(int(newFile.Fd())) if err != nil { - syscall.Close(newFd) + newFile.Close() return nil, nil, extractErrno(err) } c := &localFile{ attachPoint: l.attachPoint, hostPath: l.hostPath, - controlFile: os.NewFile(uintptr(newFd), l.hostPath), + file: newFile, mode: invalidMode, } return []p9.QID{l.attachPoint.makeQID(stat)}, c, nil @@ -490,7 +511,7 @@ func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) { var qids []p9.QID last := l for _, name := range names { - f, path, err := openAnyFile(last, name) + f, path, err := openAnyFileFromParent(last, name) if err != nil { return nil, nil, extractErrno(err) } @@ -514,7 +535,7 @@ func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) { // StatFS implements p9.File. func (l *localFile) StatFS() (p9.FSStat, error) { var s syscall.Statfs_t - if err := syscall.Fstatfs(l.controlFD(), &s); err != nil { + if err := syscall.Fstatfs(l.fd(), &s); err != nil { return p9.FSStat{}, extractErrno(err) } @@ -533,10 +554,10 @@ func (l *localFile) StatFS() (p9.FSStat, error) { // FSync implements p9.File. func (l *localFile) FSync() error { - if l.openedFile == nil { + if !l.isOpen() { return syscall.EBADF } - if err := l.openedFile.Sync(); err != nil { + if err := l.file.Sync(); err != nil { return extractErrno(err) } return nil @@ -544,7 +565,7 @@ func (l *localFile) FSync() error { // GetAttr implements p9.File. func (l *localFile) GetAttr(_ p9.AttrMask) (p9.QID, p9.AttrMask, p9.Attr, error) { - stat, err := stat(l.controlFD()) + stat, err := stat(l.fd()) if err != nil { return p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err) } @@ -612,14 +633,14 @@ func (l *localFile) SetAttr(valid p9.SetAttrMask, attr p9.SetAttr) error { // Handle all the sanity checks up front so that the client gets a // consistent result that is not attribute dependent. if !valid.IsSubsetOf(allowed) { - log.Warningf("SetAttr() failed for %q, mask: %v", l.controlFile.Name(), valid) + log.Warningf("SetAttr() failed for %q, mask: %v", l.file.Name(), valid) return syscall.EPERM } - fd := l.controlFD() - if l.ft == regular { - // Regular files are opened in RO mode, thus it needs to be reopened here - // for write. + // Check if it's possible to use cached file, or if another one needs to be + // opened for write. + fd := l.fd() + if l.ft == regular && l.mode != p9.WriteOnly && l.mode != p9.ReadWrite { f, err := os.OpenFile(l.hostPath, openFlags|os.O_WRONLY, 0) if err != nil { return extractErrno(err) @@ -733,7 +754,7 @@ func (l *localFile) RenameAt(oldName string, directory p9.File, newName string) } newParent := directory.(*localFile) - if err := renameat(l.controlFD(), oldName, newParent.controlFD(), newName); err != nil { + if err := renameat(l.fd(), oldName, newParent.fd(), newName); err != nil { return extractErrno(err) } return nil @@ -744,11 +765,11 @@ func (l *localFile) ReadAt(p []byte, offset uint64) (int, error) { if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite { return 0, syscall.EBADF } - if l.openedFile == nil { + if !l.isOpen() { return 0, syscall.EBADF } - r, err := l.openedFile.ReadAt(p, int64(offset)) + r, err := l.file.ReadAt(p, int64(offset)) switch err { case nil, io.EOF: return r, nil @@ -762,11 +783,11 @@ func (l *localFile) WriteAt(p []byte, offset uint64) (int, error) { if l.mode != p9.WriteOnly && l.mode != p9.ReadWrite { return 0, syscall.EBADF } - if l.openedFile == nil { + if !l.isOpen() { return 0, syscall.EBADF } - w, err := l.openedFile.WriteAt(p, int64(offset)) + w, err := l.file.WriteAt(p, int64(offset)) if err != nil { return w, extractErrno(err) } @@ -783,19 +804,19 @@ func (l *localFile) Symlink(target, newName string, uid p9.UID, gid p9.GID) (p9. return p9.QID{}, syscall.EBADF } - if err := unix.Symlinkat(target, l.controlFD(), newName); err != nil { + if err := unix.Symlinkat(target, l.fd(), newName); err != nil { return p9.QID{}, extractErrno(err) } cu := specutils.MakeCleanup(func() { // Best effort attempt to remove the symlink in case of failure. - if err := syscall.Unlinkat(l.controlFD(), newName); err != nil { + if err := syscall.Unlinkat(l.fd(), newName); err != nil { log.Warningf("error unlinking file %q after failure: %v", path.Join(l.hostPath, newName), err) } }) defer cu.Clean() // Open symlink to change ownership and stat it. - fd, err := syscall.Openat(l.controlFD(), newName, unix.O_PATH|openFlags, 0) + fd, err := syscall.Openat(l.fd(), newName, unix.O_PATH|openFlags, 0) if err != nil { return p9.QID{}, extractErrno(err) } @@ -824,7 +845,7 @@ func (l *localFile) Link(target p9.File, newName string) error { } targetFile := target.(*localFile) - if err := unix.Linkat(targetFile.controlFD(), "", l.controlFD(), newName, linux.AT_EMPTY_PATH); err != nil { + if err := unix.Linkat(targetFile.fd(), "", l.fd(), newName, linux.AT_EMPTY_PATH); err != nil { return extractErrno(err) } return nil @@ -847,7 +868,7 @@ func (l *localFile) UnlinkAt(name string, flags uint32) error { return syscall.EBADF } - if err := unix.Unlinkat(l.controlFD(), name, int(flags)); err != nil { + if err := unix.Unlinkat(l.fd(), name, int(flags)); err != nil { return extractErrno(err) } return nil @@ -858,7 +879,7 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite { return nil, syscall.EBADF } - if l.openedFile == nil { + if !l.isOpen() { return nil, syscall.EBADF } @@ -866,11 +887,11 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { // reading all directory contents. Take a lock because this operation is // stateful. l.readDirMu.Lock() - if _, err := l.openedFile.Seek(0, 0); err != nil { + if _, err := l.file.Seek(0, 0); err != nil { l.readDirMu.Unlock() return nil, extractErrno(err) } - names, err := l.openedFile.Readdirnames(-1) + names, err := l.file.Readdirnames(-1) if err != nil { l.readDirMu.Unlock() return nil, extractErrno(err) @@ -879,7 +900,7 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { var dirents []p9.Dirent for i := int(offset); i >= 0 && i < len(names); i++ { - stat, err := statAt(l.openedFD(), names[i]) + stat, err := statAt(l.fd(), names[i]) if err != nil { continue } @@ -897,9 +918,10 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { // Readlink implements p9.File. func (l *localFile) Readlink() (string, error) { // Shamelessly stolen from os.Readlink (added upper bound limit to buffer). - for len := 128; len < 1024*1024; len *= 2 { + const limit = 1024 * 1024 + for len := 128; len < limit; len *= 2 { b := make([]byte, len) - n, err := unix.Readlinkat(l.controlFD(), "", b) + n, err := unix.Readlinkat(l.fd(), "", b) if err != nil { return "", extractErrno(err) } @@ -922,20 +944,16 @@ func (l *localFile) Connect(p9.ConnectFlags) (*fd.FD, error) { // Close implements p9.File. func (l *localFile) Close() error { - err := l.controlFile.Close() - - // Close only once in case opened and control files point to - // the same os.File struct. - if l.openedFile != nil && l.openedFile != l.controlFile { - err = l.openedFile.Close() - } - - l.openedFile = nil - l.controlFile = nil l.mode = invalidMode + err := l.file.Close() + l.file = nil return err } +func (l *localFile) isOpen() bool { + return l.mode != invalidMode +} + // Renamed implements p9.Renamed. func (l *localFile) Renamed(newDir p9.File, newName string) { l.hostPath = path.Join(newDir.(*localFile).hostPath, newName) diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index 47b5380dc..e74df7ede 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -42,6 +42,48 @@ func assertPanic(t *testing.T, f func()) { f() } +func testReadWrite(f p9.File, flags p9.OpenFlags, content []byte) error { + want := make([]byte, len(content)) + copy(want, content) + + b := []byte("test-1-2-3") + w, err := f.WriteAt(b, uint64(len(content))) + if flags == p9.WriteOnly || flags == p9.ReadWrite { + if err != nil { + return fmt.Errorf("WriteAt(): %v", err) + } + if w != len(b) { + return fmt.Errorf("WriteAt() was partial, got: %d, want: %d", w, len(b)) + } + want = append(want, b...) + } else { + if e, ok := err.(syscall.Errno); !ok || e != syscall.EBADF { + return fmt.Errorf("WriteAt() should have failed, got: %d, want: EBADFD", err) + } + } + + rBuf := make([]byte, len(want)) + r, err := f.ReadAt(rBuf, 0) + if flags == p9.ReadOnly || flags == p9.ReadWrite { + if err != nil { + return fmt.Errorf("ReadAt(): %v", err) + } + if r != len(rBuf) { + return fmt.Errorf("ReadAt() was partial, got: %d, want: %d", r, len(rBuf)) + } + if string(rBuf) != string(want) { + return fmt.Errorf("ReadAt() wrong data, got: %s, want: %s", string(rBuf), want) + } + } else { + if e, ok := err.(syscall.Errno); !ok || e != syscall.EBADF { + return fmt.Errorf("ReadAt() should have failed, got: %d, want: EBADFD", err) + } + } + return nil +} + +var allOpenFlags = []p9.OpenFlags{p9.ReadOnly, p9.WriteOnly, p9.ReadWrite} + var ( allTypes = []fileType{regular, directory, symlink} @@ -160,61 +202,24 @@ func TestReadWrite(t *testing.T) { t.Fatalf("%v: createFile() failed, err: %v", s, err) } defer child.Close() - b := []byte("foobar") - w, err := child.WriteAt(b, 0) + want := []byte("foobar") + w, err := child.WriteAt(want, 0) if err != nil { t.Fatalf("%v: Write() failed, err: %v", s, err) } - if w != len(b) { - t.Fatalf("%v: Write() was partial, got: %d, expected: %d", s, w, len(b)) - } - for _, test := range []struct { - flags p9.OpenFlags - read bool - write bool - }{ - {flags: p9.ReadOnly, read: true, write: false}, - {flags: p9.WriteOnly, read: false, write: true}, - {flags: p9.ReadWrite, read: true, write: true}, - } { + if w != len(want) { + t.Fatalf("%v: Write() was partial, got: %d, expected: %d", s, w, len(want)) + } + for _, flags := range allOpenFlags { _, l, err := s.file.Walk([]string{"test"}) if err != nil { t.Fatalf("%v: Walk(%s) failed, err: %v", s, "test", err) } - if _, _, _, err := l.Open(test.flags); err != nil { - t.Fatalf("%v: Open(%v) failed, err: %v", s, test.flags, err) - } - - w, err = l.WriteAt(b, 0) - if test.write { - if err != nil { - t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, test.flags, err) - } - if w != len(b) { - t.Fatalf("%v, %v: WriteAt() was partial, got: %d, expected: %d", s, test.flags, w, len(b)) - } - } else { - if err == nil { - t.Fatalf("%v, %v: WriteAt() should have failed", s, test.flags) - } + if _, _, _, err := l.Open(flags); err != nil { + t.Fatalf("%v: Open(%v) failed, err: %v", s, flags, err) } - - rBuf := make([]byte, len(b)) - r, err := l.ReadAt(rBuf, 0) - if test.read { - if err != nil { - t.Fatalf("%v, %v: ReadAt() failed, err: %v", s, test.flags, err) - } - if r != len(rBuf) { - t.Fatalf("%v, %v: ReadAt() was partial, got: %d, expected: %d", s, test.flags, r, len(rBuf)) - } - if string(rBuf) != "foobar" { - t.Fatalf("%v, %v: ReadAt() wrong data, got: %s, expected: %s", s, test.flags, string(rBuf), "foobar") - } - } else { - if err == nil { - t.Fatalf("%v, %v: ReadAt() should have failed", s, test.flags) - } + if err := testReadWrite(l, flags, want); err != nil { + t.Fatalf("%v: testReadWrite(%v) failed: %v", s, flags, err) } } }) @@ -222,42 +227,57 @@ func TestReadWrite(t *testing.T) { func TestCreate(t *testing.T) { runCustom(t, []fileType{directory}, rwConfs, func(t *testing.T, s state) { - for i, test := range []struct { - flags p9.OpenFlags - read bool - }{ - {flags: p9.WriteOnly, read: false}, - {flags: p9.ReadWrite, read: true}, - } { - _, l, _, _, err := s.file.Create(fmt.Sprintf("test-%d", i), test.flags, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) + for i, flags := range allOpenFlags { + _, l, _, _, err := s.file.Create(fmt.Sprintf("test-%d", i), flags, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) if err != nil { - t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, test.flags, err) + t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, flags, err) } - b := []byte("foobar") - w, err := l.WriteAt(b, 0) + if err := testReadWrite(l, flags, []byte{}); err != nil { + t.Fatalf("%v: testReadWrite(%v) failed: %v", s, flags, err) + } + } + }) +} + +// TestReadWriteDup tests that a file opened in any mode can be dup'ed and +// reopened in any other mode. +func TestReadWriteDup(t *testing.T) { + runCustom(t, []fileType{directory}, rwConfs, func(t *testing.T, s state) { + child, err := createFile(s.file, "test") + if err != nil { + t.Fatalf("%v: createFile() failed, err: %v", s, err) + } + defer child.Close() + want := []byte("foobar") + w, err := child.WriteAt(want, 0) + if err != nil { + t.Fatalf("%v: Write() failed, err: %v", s, err) + } + if w != len(want) { + t.Fatalf("%v: Write() was partial, got: %d, expected: %d", s, w, len(want)) + } + for _, flags := range allOpenFlags { + _, l, err := s.file.Walk([]string{"test"}) if err != nil { - t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, test.flags, err) + t.Fatalf("%v: Walk(%s) failed, err: %v", s, "test", err) } - if w != len(b) { - t.Fatalf("%v, %v: WriteAt() was partial, got: %d, expected: %d", s, test.flags, w, len(b)) + defer l.Close() + if _, _, _, err := l.Open(flags); err != nil { + t.Fatalf("%v: Open(%v) failed, err: %v", s, flags, err) } - - rBuf := make([]byte, len(b)) - r, err := l.ReadAt(rBuf, 0) - if test.read { + for _, dupFlags := range allOpenFlags { + t.Logf("Original flags: %v, dup flags: %v", flags, dupFlags) + _, dup, err := l.Walk([]string{}) if err != nil { - t.Fatalf("%v, %v: ReadAt() failed, err: %v", s, test.flags, err) - } - if r != len(rBuf) { - t.Fatalf("%v, %v: ReadAt() was partial, got: %d, expected: %d", s, test.flags, r, len(rBuf)) + t.Fatalf("%v: Walk() failed: %v", s, err) } - if string(rBuf) != "foobar" { - t.Fatalf("%v, %v: ReadAt() wrong data, got: %s, expected: %s", s, test.flags, string(rBuf), "foobar") + defer dup.Close() + if _, _, _, err := dup.Open(dupFlags); err != nil { + t.Fatalf("%v: Open(%v) failed: %v", s, flags, err) } - } else { - if err == nil { - t.Fatalf("%v, %v: ReadAt() should have failed", s, test.flags) + if err := testReadWrite(dup, dupFlags, want); err != nil { + t.Fatalf("%v: testReadWrite(%v) failed: %v", s, dupFlags, err) } } } -- cgit v1.2.3