From 044aa10a412d00d54f633784b9a32bfa4476c2ac Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:12:16 +1000 Subject: [PATCH 01/30] Declared test names. --- samples/memfs/memfs_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 0513e49..226d94d 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1271,3 +1271,19 @@ func (t *MemFSTest) MkdirInParallel() { func (t *MemFSTest) SymlinkInParallel() { fusetesting.RunSymlinkInParallelTest(t.Ctx, t.Dir) } + +func (t *MemFSTest) RenameWithinDir_File() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameWithinDir_Directory() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameAcrossDirs_File() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameAcrossDirs_Directory() { + AssertTrue(false, "TODO") +} From 60383de7121b3a5e62bb815540d9351ebb66e7fb Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:27:07 +1000 Subject: [PATCH 02/30] MemFSTest.RenameWithinDir_File --- samples/memfs/memfs_test.go | 43 ++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 226d94d..be2ec6a 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1273,7 +1273,48 @@ func (t *MemFSTest) SymlinkInParallel() { } func (t *MemFSTest) RenameWithinDir_File() { - AssertTrue(false, "TODO") + var err error + + // Create a parent directory. + parentPath := path.Join(t.Dir, "parent") + + err = os.Mkdir(parentPath, 0700) + AssertEq(nil, err) + + // And a file within it. + oldPath := path.Join(parentPath, "foo") + + err = ioutil.WriteFile(oldPath, []byte("taco"), 0644) + AssertEq(nil, err) + + // Rename it. + newPath := path.Join(parentPath, "bar") + + err = os.Rename(oldPath, newPath) + AssertEq(nil, err) + + // The old name shouldn't work. + _, err = os.Stat(oldPath) + ExpectTrue(os.IsNotExist(err), "err: %v", err) + + _, err = ioutil.ReadFile(oldPath) + ExpectTrue(os.IsNotExist(err), "err: %v", err) + + // The new name should. + fi, err := os.Stat(newPath) + AssertEq(nil, err) + ExpectEq(len("taco"), fi.Size()) + + contents, err := ioutil.ReadFile(newPath) + AssertEq(nil, err) + ExpectEq("taco", string(contents)) + + // There should only be the new entry in the directory. + entries, err := fusetesting.ReadDirPicky(parentPath) + AssertEq(nil, err) + + AssertEq(1, len(entries)) + ExpectEq(path.Base(newPath), entries[0].Name()) } func (t *MemFSTest) RenameWithinDir_Directory() { From 3440c23dd2eed41a2ac1f15ccf08c79af862982d Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:28:43 +1000 Subject: [PATCH 03/30] MemFSTest.RenameWithinDir_Directory --- samples/memfs/memfs_test.go | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index be2ec6a..45897c2 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1284,7 +1284,7 @@ func (t *MemFSTest) RenameWithinDir_File() { // And a file within it. oldPath := path.Join(parentPath, "foo") - err = ioutil.WriteFile(oldPath, []byte("taco"), 0644) + err = ioutil.WriteFile(oldPath, []byte("taco"), 0400) AssertEq(nil, err) // Rename it. @@ -1312,13 +1312,51 @@ func (t *MemFSTest) RenameWithinDir_File() { // There should only be the new entry in the directory. entries, err := fusetesting.ReadDirPicky(parentPath) AssertEq(nil, err) - AssertEq(1, len(entries)) - ExpectEq(path.Base(newPath), entries[0].Name()) + fi = entries[0] + + ExpectEq(path.Base(newPath), fi.Name()) } func (t *MemFSTest) RenameWithinDir_Directory() { - AssertTrue(false, "TODO") + var err error + + // Create a parent directory. + parentPath := path.Join(t.Dir, "parent") + + err = os.Mkdir(parentPath, 0700) + AssertEq(nil, err) + + // And a directory within it. + oldPath := path.Join(parentPath, "foo") + + err = os.Mkdir(oldPath, 0700) + AssertEq(nil, err) + + // Rename it. + newPath := path.Join(parentPath, "bar") + + err = os.Rename(oldPath, newPath) + AssertEq(nil, err) + + // The old name shouldn't work. + _, err = os.Stat(oldPath) + ExpectTrue(os.IsNotExist(err), "err: %v", err) + + // The new name should. + fi, err := os.Stat(newPath) + AssertEq(nil, err) + ExpectEq(len("taco"), fi.Size()) + ExpectTrue(fi.IsDir()) + + // There should only be the new entry in the directory. + entries, err := fusetesting.ReadDirPicky(parentPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi = entries[0] + + ExpectEq(path.Base(newPath), fi.Name()) + ExpectTrue(fi.IsDir()) } func (t *MemFSTest) RenameAcrossDirs_File() { From 45ab98457f98736ff000956527b7280a40c024ab Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:33:13 +1000 Subject: [PATCH 04/30] Improved rename tests. --- samples/memfs/memfs_test.go | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 45897c2..9615f57 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1327,10 +1327,10 @@ func (t *MemFSTest) RenameWithinDir_Directory() { err = os.Mkdir(parentPath, 0700) AssertEq(nil, err) - // And a directory within it. + // And a non-empty directory within it. oldPath := path.Join(parentPath, "foo") - err = os.Mkdir(oldPath, 0700) + err = os.MkdirAll(path.Join(oldPath, "child"), 0700) AssertEq(nil, err) // Rename it. @@ -1349,7 +1349,7 @@ func (t *MemFSTest) RenameWithinDir_Directory() { ExpectEq(len("taco"), fi.Size()) ExpectTrue(fi.IsDir()) - // There should only be the new entry in the directory. + // There should only be the new entry in the parent. entries, err := fusetesting.ReadDirPicky(parentPath) AssertEq(nil, err) AssertEq(1, len(entries)) @@ -1357,6 +1357,15 @@ func (t *MemFSTest) RenameWithinDir_Directory() { ExpectEq(path.Base(newPath), fi.Name()) ExpectTrue(fi.IsDir()) + + // And the child should still be present. + entries, err = fusetesting.ReadDirPicky(newPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi = entries[0] + + ExpectEq("child", fi.Name()) + ExpectTrue(fi.IsDir()) } func (t *MemFSTest) RenameAcrossDirs_File() { @@ -1366,3 +1375,19 @@ func (t *MemFSTest) RenameAcrossDirs_File() { func (t *MemFSTest) RenameAcrossDirs_Directory() { AssertTrue(false, "TODO") } + +func (t *MemFSTest) RenameOutOfFileSystem_File() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameOutOfFileSystem_Directory() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameIntoFileSystem_File() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameIntoFileSystem_Directory() { + AssertTrue(false, "TODO") +} From feb41dddfa2c79098df5ae83fb89f89dac2fb9da Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:35:26 +1000 Subject: [PATCH 05/30] Defined RenameOp. --- fuseops/ops.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/fuseops/ops.go b/fuseops/ops.go index 7e7a9d3..e7c3c5c 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -358,6 +358,24 @@ func (o *CreateSymlinkOp) toBazilfuseResponse() (bfResp interface{}) { // Unlinking //////////////////////////////////////////////////////////////////////// +// TODO(jacobsa): Comments for struct and fields, in particular covering +// renames across mount points. +type RenameOp struct { + commonOp + + // TODO(jacobsa): Comments. + OldParent InodeID + OldName string + + // TODO(jacobsa): Comments. + NewParent InodeID + NewName string +} + +func (o *RenameOp) toBazilfuseResponse() (bfResp interface{}) { + return +} + // Unlink a directory from its parent. Because directories cannot have a link // count above one, this means the directory inode should be deleted as well // once the kernel sends ForgetInodeOp. From b121025b48fe0a059d0f602075e0ac9fa4158d55 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:37:04 +1000 Subject: [PATCH 06/30] Added Convert support. --- fuseops/convert.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fuseops/convert.go b/fuseops/convert.go index 960ca2a..0bb8544 100644 --- a/fuseops/convert.go +++ b/fuseops/convert.go @@ -117,6 +117,16 @@ func Convert( io = to co = &to.commonOp + case *bazilfuse.RenameRequest: + to := &RenameOp{ + OldParent: InodeID(typed.Header.Node), + OldName: typed.OldName, + NewParent: InodeID(typed.NewDir), + NewName: typed.OldName, + } + io = to + co = &to.commonOp + case *bazilfuse.RemoveRequest: if typed.Dir { to := &RmDirOp{ From 723fcb1d3e1e5369c8b0d9a25067fdbfa580f1e2 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:37:37 +1000 Subject: [PATCH 07/30] Added FileSystem support. --- fuseutil/file_system.go | 1 + fuseutil/not_implemented_file_system.go | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/fuseutil/file_system.go b/fuseutil/file_system.go index aff879b..c2abcdc 100644 --- a/fuseutil/file_system.go +++ b/fuseutil/file_system.go @@ -47,6 +47,7 @@ type FileSystem interface { MkDir(*fuseops.MkDirOp) error CreateFile(*fuseops.CreateFileOp) error CreateSymlink(*fuseops.CreateSymlinkOp) error + Rename(*fuseops.RenameOp) error RmDir(*fuseops.RmDirOp) error Unlink(*fuseops.UnlinkOp) error OpenDir(*fuseops.OpenDirOp) error diff --git a/fuseutil/not_implemented_file_system.go b/fuseutil/not_implemented_file_system.go index d3a5301..a395c16 100644 --- a/fuseutil/not_implemented_file_system.go +++ b/fuseutil/not_implemented_file_system.go @@ -70,6 +70,12 @@ func (fs *NotImplementedFileSystem) CreateSymlink( return } +func (fs *NotImplementedFileSystem) Rename( + op *fuseops.RenameOp) (err error) { + err = fuse.ENOSYS + return +} + func (fs *NotImplementedFileSystem) RmDir( op *fuseops.RmDirOp) (err error) { err = fuse.ENOSYS From f31cd5eb0af5b874db23cc957902be987c5076a4 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:40:03 +1000 Subject: [PATCH 08/30] Added a snooping implementation. --- samples/memfs/memfs.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/samples/memfs/memfs.go b/samples/memfs/memfs.go index ef511d5..7cafb91 100644 --- a/samples/memfs/memfs.go +++ b/samples/memfs/memfs.go @@ -15,8 +15,10 @@ package memfs import ( + "errors" "fmt" "io" + "log" "os" "time" @@ -396,6 +398,19 @@ func (fs *memFS) CreateSymlink( return } +func (fs *memFS) Rename( + op *fuseops.RenameOp) (err error) { + log.Printf( + "Received: %d %d %s %s", + op.OldParent, + op.NewParent, + op.OldName, + op.NewName) + + err = errors.New("foobar") + return +} + func (fs *memFS) RmDir( op *fuseops.RmDirOp) (err error) { fs.mu.Lock() From 8ba8c0bf24f1bccb91d5800b2b55e779e84f13d0 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:40:29 +1000 Subject: [PATCH 09/30] Oops, forgot fileSystemServer.handleOp. --- fuseutil/file_system.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fuseutil/file_system.go b/fuseutil/file_system.go index c2abcdc..7951ffa 100644 --- a/fuseutil/file_system.go +++ b/fuseutil/file_system.go @@ -149,6 +149,9 @@ func (s *fileSystemServer) handleOp(op fuseops.Op) { case *fuseops.CreateSymlinkOp: err = s.fs.CreateSymlink(typed) + case *fuseops.RenameOp: + err = s.fs.Rename(typed) + case *fuseops.RmDirOp: err = s.fs.RmDir(typed) From eeb412d4066e702f1b6b99ae34f61c610933b93b Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:41:08 +1000 Subject: [PATCH 10/30] Fixed a bug. --- fuseops/convert.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuseops/convert.go b/fuseops/convert.go index 0bb8544..ab280e2 100644 --- a/fuseops/convert.go +++ b/fuseops/convert.go @@ -122,7 +122,7 @@ func Convert( OldParent: InodeID(typed.Header.Node), OldName: typed.OldName, NewParent: InodeID(typed.NewDir), - NewName: typed.OldName, + NewName: typed.NewName, } io = to co = &to.commonOp From 9406c5bc29596056500cd0c48422ece8ac2b011d Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:44:23 +1000 Subject: [PATCH 11/30] Added more test names and TODOs. --- fuseops/ops.go | 3 ++- samples/memfs/memfs_test.go | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/fuseops/ops.go b/fuseops/ops.go index e7c3c5c..efa2d9a 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -359,7 +359,8 @@ func (o *CreateSymlinkOp) toBazilfuseResponse() (bfResp interface{}) { //////////////////////////////////////////////////////////////////////// // TODO(jacobsa): Comments for struct and fields, in particular covering -// renames across mount points. +// renames across mount points. Mention that you'll still get a forget, like +// RmDirOp. type RenameOp struct { commonOp diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 9615f57..76f3ea1 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1391,3 +1391,11 @@ func (t *MemFSTest) RenameIntoFileSystem_File() { func (t *MemFSTest) RenameIntoFileSystem_Directory() { AssertTrue(false, "TODO") } + +func (t *MemFSTest) RenameOverExistingFile() { + AssertTrue(false, "TODO") +} + +func (t *MemFSTest) RenameOverExistingDirectory() { + AssertTrue(false, "TODO") +} From 4008a44fb6e61295ff68e9bb5a7b5ab7ea49c2e9 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 15:45:12 +1000 Subject: [PATCH 12/30] Added another TODO. --- fuseops/ops.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuseops/ops.go b/fuseops/ops.go index efa2d9a..9652d3a 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -360,7 +360,7 @@ func (o *CreateSymlinkOp) toBazilfuseResponse() (bfResp interface{}) { // TODO(jacobsa): Comments for struct and fields, in particular covering // renames across mount points. Mention that you'll still get a forget, like -// RmDirOp. +// RmDirOp. Also that an existing destination name should be atomically replaced. type RenameOp struct { commonOp From 1d764bcde04b285bb359c49c2b844c93854e6c4b Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 16:52:28 +1000 Subject: [PATCH 13/30] Mostly implemented Rename. --- samples/memfs/memfs.go | 44 +++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/samples/memfs/memfs.go b/samples/memfs/memfs.go index 7cafb91..b242513 100644 --- a/samples/memfs/memfs.go +++ b/samples/memfs/memfs.go @@ -15,10 +15,8 @@ package memfs import ( - "errors" "fmt" "io" - "log" "os" "time" @@ -400,14 +398,42 @@ func (fs *memFS) CreateSymlink( func (fs *memFS) Rename( op *fuseops.RenameOp) (err error) { - log.Printf( - "Received: %d %d %s %s", - op.OldParent, - op.NewParent, - op.OldName, - op.NewName) + fs.mu.Lock() + defer fs.mu.Unlock() + + // Ask the old parent for the child's inode ID. Unlock because we need to + // lock the new parent. This should be safe without risk of the referrent of + // the name changing, because the kernel needs to hold a lock on each of the + // parents. + oldParent := fs.getInodeOrDie(op.OldParent) + childID, ok := oldParent.LookUpChild(op.OldName) + oldParent.mu.Unlock() + + if !ok { + err = fuse.ENOENT + return + } + + // If the new name exists in the new parent, delete it first. Then link in + // the child. + newParent := fs.getInodeOrDie(op.NewParent) + _, ok = newParent.LookUpChild(op.NewName) + if ok { + newParent.RemoveChild(op.NewName) + } + + newParent.AddChild( + childID, + op.NewName, + TODO_Type) + + newParent.Unlock() + + // Finally, remove the old name from the old parent. + oldParent.Lock() + oldParent.RemoveChild(op.OldName) + oldParent.Unlock() - err = errors.New("foobar") return } From 5532e21b56fcd0ba53109ab46fe19d8f06273386 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 16:54:49 +1000 Subject: [PATCH 14/30] Fixed build errors. --- samples/memfs/inode.go | 6 +++++- samples/memfs/memfs.go | 24 ++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/samples/memfs/inode.go b/samples/memfs/inode.go index 587fd3e..4a0a16e 100644 --- a/samples/memfs/inode.go +++ b/samples/memfs/inode.go @@ -202,10 +202,14 @@ func (in *inode) Len() (n int) { // Find an entry for the given child name and return its inode ID. // // REQUIRES: in.isDir() -func (in *inode) LookUpChild(name string) (id fuseops.InodeID, ok bool) { +func (in *inode) LookUpChild(name string) ( + id fuseops.InodeID, + typ fuseutil.DirentType, + ok bool) { index, ok := in.findChild(name) if ok { id = in.entries[index].Inode + typ = in.entries[index].Type } return diff --git a/samples/memfs/memfs.go b/samples/memfs/memfs.go index b242513..7d7dd8b 100644 --- a/samples/memfs/memfs.go +++ b/samples/memfs/memfs.go @@ -192,7 +192,7 @@ func (fs *memFS) LookUpInode( inode := fs.getInodeOrDie(op.Parent) // Does the directory have an entry with the given name? - childID, ok := inode.LookUpChild(op.Name) + childID, _, ok := inode.LookUpChild(op.Name) if !ok { err = fuse.ENOENT return @@ -262,7 +262,7 @@ func (fs *memFS) MkDir( // Ensure that the name doesn't already exist, so we don't wind up with a // duplicate. - _, exists := parent.LookUpChild(op.Name) + _, _, exists := parent.LookUpChild(op.Name) if exists { err = fuse.EEXIST return @@ -305,7 +305,7 @@ func (fs *memFS) CreateFile( // Ensure that the name doesn't already exist, so we don't wind up with a // duplicate. - _, exists := parent.LookUpChild(op.Name) + _, _, exists := parent.LookUpChild(op.Name) if exists { err = fuse.EEXIST return @@ -355,7 +355,7 @@ func (fs *memFS) CreateSymlink( // Ensure that the name doesn't already exist, so we don't wind up with a // duplicate. - _, exists := parent.LookUpChild(op.Name) + _, _, exists := parent.LookUpChild(op.Name) if exists { err = fuse.EEXIST return @@ -406,7 +406,7 @@ func (fs *memFS) Rename( // the name changing, because the kernel needs to hold a lock on each of the // parents. oldParent := fs.getInodeOrDie(op.OldParent) - childID, ok := oldParent.LookUpChild(op.OldName) + childID, childType, ok := oldParent.LookUpChild(op.OldName) oldParent.mu.Unlock() if !ok { @@ -417,7 +417,7 @@ func (fs *memFS) Rename( // If the new name exists in the new parent, delete it first. Then link in // the child. newParent := fs.getInodeOrDie(op.NewParent) - _, ok = newParent.LookUpChild(op.NewName) + _, _, ok = newParent.LookUpChild(op.NewName) if ok { newParent.RemoveChild(op.NewName) } @@ -425,14 +425,14 @@ func (fs *memFS) Rename( newParent.AddChild( childID, op.NewName, - TODO_Type) + childType) - newParent.Unlock() + newParent.mu.Unlock() // Finally, remove the old name from the old parent. - oldParent.Lock() + oldParent.mu.Lock() oldParent.RemoveChild(op.OldName) - oldParent.Unlock() + oldParent.mu.Unlock() return } @@ -446,7 +446,7 @@ func (fs *memFS) RmDir( parent := fs.getInodeOrDie(op.Parent) // Find the child within the parent. - childID, ok := parent.LookUpChild(op.Name) + childID, _, ok := parent.LookUpChild(op.Name) if !ok { err = fuse.ENOENT return @@ -479,7 +479,7 @@ func (fs *memFS) Unlink( parent := fs.getInodeOrDie(op.Parent) // Find the child within the parent. - childID, ok := parent.LookUpChild(op.Name) + childID, _, ok := parent.LookUpChild(op.Name) if !ok { err = fuse.ENOENT return From 7c97a95067f11839c5752bcd32691db717b7750c Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 16:56:16 +1000 Subject: [PATCH 15/30] Strengthened some tests. --- samples/memfs/memfs_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 76f3ea1..a21c143 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1304,6 +1304,7 @@ func (t *MemFSTest) RenameWithinDir_File() { fi, err := os.Stat(newPath) AssertEq(nil, err) ExpectEq(len("taco"), fi.Size()) + ExpectEq(os.FileMode(0400), fi.Mode()) contents, err := ioutil.ReadFile(newPath) AssertEq(nil, err) @@ -1316,6 +1317,7 @@ func (t *MemFSTest) RenameWithinDir_File() { fi = entries[0] ExpectEq(path.Base(newPath), fi.Name()) + ExpectEq(os.FileMode(0400), fi.Mode()) } func (t *MemFSTest) RenameWithinDir_Directory() { @@ -1347,7 +1349,7 @@ func (t *MemFSTest) RenameWithinDir_Directory() { fi, err := os.Stat(newPath) AssertEq(nil, err) ExpectEq(len("taco"), fi.Size()) - ExpectTrue(fi.IsDir()) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) // There should only be the new entry in the parent. entries, err := fusetesting.ReadDirPicky(parentPath) @@ -1356,7 +1358,7 @@ func (t *MemFSTest) RenameWithinDir_Directory() { fi = entries[0] ExpectEq(path.Base(newPath), fi.Name()) - ExpectTrue(fi.IsDir()) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) // And the child should still be present. entries, err = fusetesting.ReadDirPicky(newPath) @@ -1365,7 +1367,7 @@ func (t *MemFSTest) RenameWithinDir_Directory() { fi = entries[0] ExpectEq("child", fi.Name()) - ExpectTrue(fi.IsDir()) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) } func (t *MemFSTest) RenameAcrossDirs_File() { From 99080585cefd0dcdfacba648879372e3b4f0b040 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 16:56:26 +1000 Subject: [PATCH 16/30] Fixed a test bug. --- samples/memfs/memfs_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index a21c143..7a13531 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1348,7 +1348,6 @@ func (t *MemFSTest) RenameWithinDir_Directory() { // The new name should. fi, err := os.Stat(newPath) AssertEq(nil, err) - ExpectEq(len("taco"), fi.Size()) ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) // There should only be the new entry in the parent. From f7f7ac8ff698c0df42d63aa4deae9e27a91d7a9a Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:40:42 +1000 Subject: [PATCH 17/30] Fixed build errors. --- samples/memfs/memfs.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/samples/memfs/memfs.go b/samples/memfs/memfs.go index 7d7dd8b..36008de 100644 --- a/samples/memfs/memfs.go +++ b/samples/memfs/memfs.go @@ -401,13 +401,9 @@ func (fs *memFS) Rename( fs.mu.Lock() defer fs.mu.Unlock() - // Ask the old parent for the child's inode ID. Unlock because we need to - // lock the new parent. This should be safe without risk of the referrent of - // the name changing, because the kernel needs to hold a lock on each of the - // parents. + // Ask the old parent for the child's inode ID and type. oldParent := fs.getInodeOrDie(op.OldParent) childID, childType, ok := oldParent.LookUpChild(op.OldName) - oldParent.mu.Unlock() if !ok { err = fuse.ENOENT @@ -427,12 +423,8 @@ func (fs *memFS) Rename( op.NewName, childType) - newParent.mu.Unlock() - // Finally, remove the old name from the old parent. - oldParent.mu.Lock() oldParent.RemoveChild(op.OldName) - oldParent.mu.Unlock() return } From 7b0dfea7ddc054bf92312e97dbc720536f9b06f4 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 16:57:50 +1000 Subject: [PATCH 18/30] Added a test stub. --- samples/memfs/memfs_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 7a13531..83bd914 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1369,6 +1369,11 @@ func (t *MemFSTest) RenameWithinDir_Directory() { ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) } +func (t *MemFSTest) RenameWithinDir_SameName() { + // TODO(jacobsa): Make sure to check what a real file system does here. + AssertTrue(false, "TODO") +} + func (t *MemFSTest) RenameAcrossDirs_File() { AssertTrue(false, "TODO") } From 6ec382c95209492e51e5a9a07852059e04400de9 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:29:40 +1000 Subject: [PATCH 19/30] Added another test name. --- samples/memfs/memfs_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 83bd914..9e8d2da 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1405,3 +1405,7 @@ func (t *MemFSTest) RenameOverExistingFile() { func (t *MemFSTest) RenameOverExistingDirectory() { AssertTrue(false, "TODO") } + +func (t *MemFSTest) RenameNonExistentFile() { + AssertTrue(false, "TODO") +} From bde0d1be294593f516a80ddfb33837d783a6090a Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:30:58 +1000 Subject: [PATCH 20/30] MemFSTest.RenameWithinDir_SameName --- samples/memfs/memfs_test.go | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 9e8d2da..49287c6 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1370,8 +1370,37 @@ func (t *MemFSTest) RenameWithinDir_Directory() { } func (t *MemFSTest) RenameWithinDir_SameName() { - // TODO(jacobsa): Make sure to check what a real file system does here. - AssertTrue(false, "TODO") + var err error + + // Create a parent directory. + parentPath := path.Join(t.Dir, "parent") + + err = os.Mkdir(parentPath, 0700) + AssertEq(nil, err) + + // And a file within it. + filePath := path.Join(parentPath, "foo") + + err = ioutil.WriteFile(filePath, []byte("taco"), 0400) + AssertEq(nil, err) + + // Attempt to rename it. + err = os.Rename(filePath, filePath) + AssertEq(nil, err) + + // The file should still exist. + contents, err := ioutil.ReadFile(filePath) + AssertEq(nil, err) + ExpectEq("taco", string(contents)) + + // There should only be the one entry in the directory. + entries, err := fusetesting.ReadDirPicky(parentPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi := entries[0] + + ExpectEq(path.Base(filePath), fi.Name()) + ExpectEq(os.FileMode(0400), fi.Mode()) } func (t *MemFSTest) RenameAcrossDirs_File() { From 361e9e61f60f47804ed813bed223c35acb82956f Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:43:01 +1000 Subject: [PATCH 21/30] MemFSTest.RenameAcrossDirs_File --- samples/memfs/memfs_test.go | 55 ++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 49287c6..fb58092 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1404,7 +1404,60 @@ func (t *MemFSTest) RenameWithinDir_SameName() { } func (t *MemFSTest) RenameAcrossDirs_File() { - AssertTrue(false, "TODO") + var err error + + // Create two parent directories. + oldParentPath := path.Join(t.Dir, "old") + newParentPath := path.Join(t.Dir, "new") + + err = os.Mkdir(oldParentPath, 0700) + AssertEq(nil, err) + + err = os.Mkdir(newParentPath, 0700) + AssertEq(nil, err) + + // And a file within the first. + oldPath := path.Join(oldParentPath, "foo") + + err = ioutil.WriteFile(oldPath, []byte("taco"), 0400) + AssertEq(nil, err) + + // Rename it. + newPath := path.Join(newParentPath, "bar") + + err = os.Rename(oldPath, newPath) + AssertEq(nil, err) + + // The old name shouldn't work. + _, err = os.Stat(oldPath) + ExpectTrue(os.IsNotExist(err), "err: %v", err) + + _, err = ioutil.ReadFile(oldPath) + ExpectTrue(os.IsNotExist(err), "err: %v", err) + + // The new name should. + fi, err := os.Stat(newPath) + AssertEq(nil, err) + ExpectEq(len("taco"), fi.Size()) + ExpectEq(os.FileMode(0400), fi.Mode()) + + contents, err := ioutil.ReadFile(newPath) + AssertEq(nil, err) + ExpectEq("taco", string(contents)) + + // Check the old parent. + entries, err := fusetesting.ReadDirPicky(oldParentPath) + AssertEq(nil, err) + AssertEq(0, len(entries)) + + // And the new one. + entries, err = fusetesting.ReadDirPicky(newParentPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi = entries[0] + + ExpectEq(path.Base(newPath), fi.Name()) + ExpectEq(os.FileMode(0400), fi.Mode()) } func (t *MemFSTest) RenameAcrossDirs_Directory() { From af38b8f8e5777283cb5ac6b5fefcaaec2e16af9c Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:45:12 +1000 Subject: [PATCH 22/30] MemFSTest.RenameAcrossDirs_Directory --- samples/memfs/memfs_test.go | 56 ++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index fb58092..83438b6 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1461,7 +1461,61 @@ func (t *MemFSTest) RenameAcrossDirs_File() { } func (t *MemFSTest) RenameAcrossDirs_Directory() { - AssertTrue(false, "TODO") + var err error + + // Create two parent directories. + oldParentPath := path.Join(t.Dir, "old") + newParentPath := path.Join(t.Dir, "new") + + err = os.Mkdir(oldParentPath, 0700) + AssertEq(nil, err) + + err = os.Mkdir(newParentPath, 0700) + AssertEq(nil, err) + + // And a non-empty directory within the first. + oldPath := path.Join(oldParentPath, "foo") + + err = os.MkdirAll(path.Join(oldPath, "child"), 0700) + AssertEq(nil, err) + + // Rename it. + newPath := path.Join(newParentPath, "bar") + + err = os.Rename(oldPath, newPath) + AssertEq(nil, err) + + // The old name shouldn't work. + _, err = os.Stat(oldPath) + ExpectTrue(os.IsNotExist(err), "err: %v", err) + + // The new name should. + fi, err := os.Stat(newPath) + AssertEq(nil, err) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) + + // And the child should still be present. + entries, err := fusetesting.ReadDirPicky(newPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi = entries[0] + + ExpectEq("child", fi.Name()) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) + + // Check the old parent. + entries, err = fusetesting.ReadDirPicky(oldParentPath) + AssertEq(nil, err) + AssertEq(0, len(entries)) + + // And the new one. + entries, err = fusetesting.ReadDirPicky(newParentPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi = entries[0] + + ExpectEq(path.Base(newPath), fi.Name()) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) } func (t *MemFSTest) RenameOutOfFileSystem_File() { From a69cbc2139d5099d1db9d77f3c596465dca2844d Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:47:31 +1000 Subject: [PATCH 23/30] MemFSTest.RenameOutOfFileSystem --- samples/memfs/memfs_test.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 83438b6..3996c45 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1518,12 +1518,22 @@ func (t *MemFSTest) RenameAcrossDirs_Directory() { ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) } -func (t *MemFSTest) RenameOutOfFileSystem_File() { - AssertTrue(false, "TODO") -} +func (t *MemFSTest) RenameOutOfFileSystem() { + var err error -func (t *MemFSTest) RenameOutOfFileSystem_Directory() { - AssertTrue(false, "TODO") + // Create a file. + oldPath := path.Join(t.Dir, "foo") + + err = ioutil.WriteFile(oldPath, []byte("taco"), 0400) + AssertEq(nil, err) + + // Attempt to move it out of the file system. + tempDir, err := ioutil.TempDir("", "memfs_test") + AssertEq(nil, err) + defer os.RemoveAll(tempDir) + + err = os.Rename(oldPath, path.Join(tempDir, "bar")) + ExpectThat(err, Error(HasSubstr("cross-device"))) } func (t *MemFSTest) RenameIntoFileSystem_File() { From 542984f5bd1413d728d1147a23c67077f0aaf8ee Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:48:45 +1000 Subject: [PATCH 24/30] MemFSTest.RenameIntoFileSystem --- samples/memfs/memfs_test.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 3996c45..6d1b80c 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1536,12 +1536,20 @@ func (t *MemFSTest) RenameOutOfFileSystem() { ExpectThat(err, Error(HasSubstr("cross-device"))) } -func (t *MemFSTest) RenameIntoFileSystem_File() { - AssertTrue(false, "TODO") -} +func (t *MemFSTest) RenameIntoFileSystem() { + var err error -func (t *MemFSTest) RenameIntoFileSystem_Directory() { - AssertTrue(false, "TODO") + // Create a file outside of our file system. + f, err := ioutil.TempFile("", "memfs_test") + AssertEq(nil, err) + defer f.Close() + + oldPath := f.Name() + defer os.Remove(oldPath) + + // Attempt to move it into the file system. + err = os.Rename(oldPath, path.Join(t.Dir, "bar")) + ExpectThat(err, Error(HasSubstr("cross-device"))) } func (t *MemFSTest) RenameOverExistingFile() { From 76b311bdc2135dc82fdb47cda3fc4e9c87739786 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:51:36 +1000 Subject: [PATCH 25/30] MemFSTest.RenameOverExistingFile --- samples/memfs/memfs_test.go | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 6d1b80c..c42c3ab 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1553,13 +1553,45 @@ func (t *MemFSTest) RenameIntoFileSystem() { } func (t *MemFSTest) RenameOverExistingFile() { - AssertTrue(false, "TODO") + var err error + + // Create two files. + oldPath := path.Join(t.Dir, "foo") + err = ioutil.WriteFile(oldPath, []byte("taco"), 0400) + AssertEq(nil, err) + + newPath := path.Join(t.Dir, "bar") + err = ioutil.WriteFile(newPath, []byte("burrito"), 0600) + AssertEq(nil, err) + + // Rename one over the other. + err = os.Rename(oldPath, newPath) + AssertEq(nil, err) + + // Check the file contents. + contents, err := ioutil.ReadFile(newPath) + AssertEq(nil, err) + ExpectEq("taco", string(contents)) + + // And the parent listing. + entries, err := fusetesting.ReadDirPicky(t.Dir) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi := entries[0] + + ExpectEq(path.Base(newPath), fi.Name()) + ExpectEq(os.FileMode(0400), fi.Mode()) + ExpectEq(len("taco"), fi.Size()) } func (t *MemFSTest) RenameOverExistingDirectory() { AssertTrue(false, "TODO") } +func (t *MemFSTest) RenameOverExisting_WrongType() { + AssertTrue(false, "TODO") +} + func (t *MemFSTest) RenameNonExistentFile() { AssertTrue(false, "TODO") } From a28c3afdfd3ceb9bc72dc539e7a2629ad9d0e477 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:54:38 +1000 Subject: [PATCH 26/30] MemFSTest.RenameOverExistingDirectory --- samples/memfs/memfs_test.go | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index c42c3ab..136d429 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1585,7 +1585,41 @@ func (t *MemFSTest) RenameOverExistingFile() { } func (t *MemFSTest) RenameOverExistingDirectory() { - AssertTrue(false, "TODO") + var err error + + // Create two directories, the first non-empty. + oldPath := path.Join(t.Dir, "foo") + err = os.MkdirAll(path.Join(oldPath, "child"), 0700) + AssertEq(nil, err) + + newPath := path.Join(t.Dir, "bar") + err = os.Mkdir(newPath, 0600) + AssertEq(nil, err) + + // Renaming over the non-empty one shouldn't work. + err = os.Rename(newPath, oldPath) + ExpectThat(err, Error(HasSubstr("TODO"))) + + // But the other way around should. + err = os.Rename(oldPath, newPath) + AssertEq(nil, err) + + // Check the parent listing. + entries, err := fusetesting.ReadDirPicky(t.Dir) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi := entries[0] + + ExpectEq(path.Base(newPath), fi.Name()) + ExpectEq(os.FileMode(0700)|os.ModeDir, fi.Mode()) + + // And the directory itself. + entries, err = fusetesting.ReadDirPicky(newPath) + AssertEq(nil, err) + AssertEq(1, len(entries)) + fi = entries[0] + + ExpectEq("child", fi.Name()) } func (t *MemFSTest) RenameOverExisting_WrongType() { From 7fb4f4d34fc16c285e10af76740f3993d30e8a32 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:57:17 +1000 Subject: [PATCH 27/30] MemFSTest.RenameOverExisting_WrongType --- samples/memfs/memfs_test.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 136d429..8a49bdd 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1623,7 +1623,23 @@ func (t *MemFSTest) RenameOverExistingDirectory() { } func (t *MemFSTest) RenameOverExisting_WrongType() { - AssertTrue(false, "TODO") + var err error + + // Create a file and a directory. + filePath := path.Join(t.Dir, "foo") + err = ioutil.WriteFile(filePath, []byte("taco"), 0400) + AssertEq(nil, err) + + dirPath := path.Join(t.Dir, "bar") + err = os.Mkdir(dirPath, 0700) + AssertEq(nil, err) + + // Renaming one over the other shouldn't work. + err = os.Rename(filePath, dirPath) + ExpectThat(err, Error(HasSubstr("is a directory"))) + + err = os.Rename(dirPath, filePath) + ExpectThat(err, Error(HasSubstr("not a directory"))) } func (t *MemFSTest) RenameNonExistentFile() { From 1878a138500fc8998c2cd7c0340daf15bae0b56d Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 21:58:10 +1000 Subject: [PATCH 28/30] MemFSTest.RenameNonExistentFile --- samples/memfs/memfs_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 8a49bdd..61a08b5 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1643,5 +1643,8 @@ func (t *MemFSTest) RenameOverExisting_WrongType() { } func (t *MemFSTest) RenameNonExistentFile() { - AssertTrue(false, "TODO") + var err error + + err = os.Rename(path.Join(t.Dir, "foo"), path.Join(t.Dir, "bar")) + ExpectThat(err, Error(HasSubstr("no such file"))) } From 515cdb41a879a8c5c6a9edee6fa8c3f5ab5cda0c Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 22:01:56 +1000 Subject: [PATCH 29/30] Fixed a bug in memfs renaming. --- fuseops/ops.go | 3 ++- samples/memfs/inode.go | 2 +- samples/memfs/memfs.go | 19 +++++++++++-------- samples/memfs/memfs_test.go | 2 +- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/fuseops/ops.go b/fuseops/ops.go index 9652d3a..4cfc790 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -360,7 +360,8 @@ func (o *CreateSymlinkOp) toBazilfuseResponse() (bfResp interface{}) { // TODO(jacobsa): Comments for struct and fields, in particular covering // renames across mount points. Mention that you'll still get a forget, like -// RmDirOp. Also that an existing destination name should be atomically replaced. +// RmDirOp. Also that an existing destination name should be atomically +// replaced. Also that the new directory must be empty if it exists. type RenameOp struct { commonOp diff --git a/samples/memfs/inode.go b/samples/memfs/inode.go index 4a0a16e..78eca0b 100644 --- a/samples/memfs/inode.go +++ b/samples/memfs/inode.go @@ -278,7 +278,7 @@ func (in *inode) RemoveChild(name string) { // Serve a ReadDir request. // // REQUIRES: in.isDir() -func (in *inode) ReadDir(offset int, size int) (data []byte, err error) { +func (in *inode) ReadDir(offset int, size int) (data []byte) { if !in.isDir() { panic("ReadDir called on non-directory.") } diff --git a/samples/memfs/memfs.go b/samples/memfs/memfs.go index 36008de..beabb57 100644 --- a/samples/memfs/memfs.go +++ b/samples/memfs/memfs.go @@ -410,14 +410,21 @@ func (fs *memFS) Rename( return } - // If the new name exists in the new parent, delete it first. Then link in - // the child. + // If the new name exists already in the new parent, make sure it's not a + // non-empty directory, then delete it. newParent := fs.getInodeOrDie(op.NewParent) - _, _, ok = newParent.LookUpChild(op.NewName) + existingID, _, ok := newParent.LookUpChild(op.NewName) if ok { + existing := fs.getInodeOrDie(existingID) + if existing.isDir() && len(existing.ReadDir(0, 1024)) > 0 { + err = fuse.ENOTEMPTY + return + } + newParent.RemoveChild(op.NewName) } + // Link the new name. newParent.AddChild( childID, op.NewName, @@ -515,11 +522,7 @@ func (fs *memFS) ReadDir( inode := fs.getInodeOrDie(op.Inode) // Serve the request. - op.Data, err = inode.ReadDir(int(op.Offset), op.Size) - if err != nil { - err = fmt.Errorf("inode.ReadDir: %v", err) - return - } + op.Data = inode.ReadDir(int(op.Offset), op.Size) return } diff --git a/samples/memfs/memfs_test.go b/samples/memfs/memfs_test.go index 61a08b5..10c3f36 100644 --- a/samples/memfs/memfs_test.go +++ b/samples/memfs/memfs_test.go @@ -1598,7 +1598,7 @@ func (t *MemFSTest) RenameOverExistingDirectory() { // Renaming over the non-empty one shouldn't work. err = os.Rename(newPath, oldPath) - ExpectThat(err, Error(HasSubstr("TODO"))) + ExpectThat(err, Error(HasSubstr("not empty"))) // But the other way around should. err = os.Rename(oldPath, newPath) From b6588a6d4102b16a59dc299f5bb261254066e836 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Thu, 25 Jun 2015 22:13:45 +1000 Subject: [PATCH 30/30] Added documentation for RenameOp. --- fuseops/ops.go | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/fuseops/ops.go b/fuseops/ops.go index 4cfc790..22696b8 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -358,18 +358,50 @@ func (o *CreateSymlinkOp) toBazilfuseResponse() (bfResp interface{}) { // Unlinking //////////////////////////////////////////////////////////////////////// -// TODO(jacobsa): Comments for struct and fields, in particular covering -// renames across mount points. Mention that you'll still get a forget, like -// RmDirOp. Also that an existing destination name should be atomically -// replaced. Also that the new directory must be empty if it exists. +// Rename a file or directory, given the IDs of the original parent directory +// and the new one (which may be the same). +// +// In Linux, this is called by vfs_rename (https://goo.gl/eERItT), which is +// called by sys_renameat2 (https://goo.gl/fCC9qC). +// +// The kernel takes care of ensuring that the source and destination are not +// identical (in which case it does nothing), that the rename is not across +// file system boundaries, and that the destination doesn't already exist with +// the wrong type. Some subtleties that the file system must care about: +// +// * If the new name is an existing directory, the file system must ensure it +// is empty before replacing it, returning ENOTEMPTY otherwise. (This is +// per the posix spec: http://goo.gl/4XtT79) +// +// * The rename must be atomic from the point of view of an observer of the +// new name. That is, if the new name already exists, there must be no +// point at which it doesn't exist. +// +// * It is okay for the new name to be modified before the old name is +// removed; these need not be atomic. In fact, the Linux man page +// explicitly says this is likely (cf. https://goo.gl/Y1wVZc). +// +// * Linux bends over backwards (https://goo.gl/pLDn3r) to ensure that +// neither the old nor the new parent can be concurrently modified. But +// it's not clear whether OS X does this, and in any case it doesn't matter +// for file systems that may be modified remotely. Therefore a careful file +// system implementor should probably ensure if possible that the unlink +// step in the "link new name, unlink old name" process doesn't unlink a +// different inode than the one that was linked to the new name. Still, +// posix and the man pages are imprecise about the actual semantics of a +// rename if it's not atomic, so it is probably not disastrous to be loose +// about this. +// type RenameOp struct { commonOp - // TODO(jacobsa): Comments. + // The old parent directory, and the name of the entry within it to be + // relocated. OldParent InodeID OldName string - // TODO(jacobsa): Comments. + // The new parent directory, and the name of the entry to be created or + // overwritten within it. NewParent InodeID NewName string }