13111: Propagate errors in Child().

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go
index 4d22c1e82c6cc807d34c9e4c98bb8a7b68f731c2..369b4bb9420b779846304f69662e4596e4cb6d0a 100644 (file)
--- a/sdk/go/arvados/fs_base.go
+++ b/sdk/go/arvados/fs_base.go
@@ -27,6 +27,7 @@ var (
        ErrWriteOnlyMode     = errors.New("file is O_WRONLY")
        ErrSyncNotSupported  = errors.New("O_SYNC flag is not supported")
        ErrIsDirectory       = errors.New("cannot rename file to overwrite existing directory")
+       ErrNotADirectory     = errors.New("not a directory")
        ErrPermission        = os.ErrPermission
 )
 
@@ -128,7 +129,7 @@ type inode interface {
        // a child was added or changed, the new child is returned.
        //
        // Caller must have lock (or rlock if replace is nil).
-       Child(name string, replace func(inode) inode) inode
+       Child(name string, replace func(inode) (inode, error)) (inode, error)
 
        sync.Locker
        RLock()
@@ -202,8 +203,8 @@ func (*nullnode) Readdir() ([]os.FileInfo, error) {
        return nil, ErrInvalidOperation
 }
 
-func (*nullnode) Child(name string, replace func(inode) inode) inode {
-       return nil
+func (*nullnode) Child(name string, replace func(inode) (inode, error)) (inode, error) {
+       return nil, ErrNotADirectory
 }
 
 type treenode struct {
@@ -236,18 +237,25 @@ func (n *treenode) IsDir() bool {
        return true
 }
 
-func (n *treenode) Child(name string, replace func(inode) inode) (child inode) {
-       // TODO: special treatment for "", ".", ".."
+func (n *treenode) Child(name string, replace func(inode) (inode, error)) (child inode, err error) {
        child = n.inodes[name]
-       if replace != nil {
-               newchild := replace(child)
-               if newchild == nil {
-                       delete(n.inodes, name)
-               } else if newchild != child {
-                       n.inodes[name] = newchild
-                       n.fileinfo.modTime = time.Now()
-                       child = newchild
-               }
+       if name == "" || name == "." || name == ".." {
+               err = ErrInvalidArgument
+               return
+       }
+       if replace == nil {
+               return
+       }
+       newchild, err := replace(child)
+       if err != nil {
+               return
+       }
+       if newchild == nil {
+               delete(n.inodes, name)
+       } else if newchild != child {
+               n.inodes[name] = newchild
+               n.fileinfo.modTime = time.Now()
+               child = newchild
        }
        return
 }
@@ -297,9 +305,9 @@ func (fs *fileSystem) openFile(name string, flag int, perm os.FileMode) (*fileha
                return nil, ErrSyncNotSupported
        }
        dirname, name := path.Split(name)
-       parent := rlookup(fs.root, dirname)
-       if parent == nil {
-               return nil, os.ErrNotExist
+       parent, err := rlookup(fs.root, dirname)
+       if err != nil {
+               return nil, err
        }
        var readable, writable bool
        switch flag & (os.O_RDWR | os.O_RDONLY | os.O_WRONLY) {
@@ -331,22 +339,26 @@ func (fs *fileSystem) openFile(name string, flag int, perm os.FileMode) (*fileha
                parent.RLock()
                defer parent.RUnlock()
        }
-       n := parent.Child(name, nil)
-       if n == nil {
+       n, err := parent.Child(name, nil)
+       if err != nil {
+               return nil, err
+       } else if n == nil {
                if !createMode {
                        return nil, os.ErrNotExist
                }
-               var err error
-               n = parent.Child(name, func(inode) inode {
-                       n, err = parent.FS().newNode(name, perm|0755, time.Now())
-                       n.SetParent(parent, name)
-                       return n
+               n, err = parent.Child(name, func(inode) (repl inode, err error) {
+                       repl, err = parent.FS().newNode(name, perm|0755, time.Now())
+                       if err != nil {
+                               return
+                       }
+                       repl.SetParent(parent, name)
+                       return
                })
                if err != nil {
                        return nil, err
                } else if n == nil {
-                       // parent rejected new child
-                       return nil, ErrInvalidOperation
+                       // Parent rejected new child, but returned no error
+                       return nil, ErrInvalidArgument
                }
        } else if flag&os.O_EXCL != 0 {
                return nil, ErrFileExists
@@ -375,38 +387,37 @@ func (fs *fileSystem) Create(name string) (File, error) {
        return fs.OpenFile(name, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0)
 }
 
-func (fs *fileSystem) Mkdir(name string, perm os.FileMode) (err error) {
+func (fs *fileSystem) Mkdir(name string, perm os.FileMode) error {
        dirname, name := path.Split(name)
-       n := rlookup(fs.root, dirname)
-       if n == nil {
-               return os.ErrNotExist
+       n, err := rlookup(fs.root, dirname)
+       if err != nil {
+               return err
        }
        n.Lock()
        defer n.Unlock()
-       if n.Child(name, nil) != nil {
+       if child, err := n.Child(name, nil); err != nil {
+               return err
+       } else if child != nil {
                return os.ErrExist
        }
-       child := n.Child(name, func(inode) (child inode) {
-               child, err = n.FS().newNode(name, perm|os.ModeDir, time.Now())
-               child.SetParent(n, name)
+
+       _, err = n.Child(name, func(inode) (repl inode, err error) {
+               repl, err = n.FS().newNode(name, perm|os.ModeDir, time.Now())
+               if err != nil {
+                       return
+               }
+               repl.SetParent(n, name)
                return
        })
-       if err != nil {
-               return err
-       } else if child == nil {
-               return ErrInvalidArgument
-       }
-       return nil
+       return err
 }
 
-func (fs *fileSystem) Stat(name string) (fi os.FileInfo, err error) {
-       node := rlookup(fs.root, name)
-       if node == nil {
-               err = os.ErrNotExist
-       } else {
-               fi = node.FileInfo()
+func (fs *fileSystem) Stat(name string) (os.FileInfo, error) {
+       node, err := rlookup(fs.root, name)
+       if err != nil {
+               return nil, err
        }
-       return
+       return node.FileInfo(), nil
 }
 
 func (fs *fileSystem) Rename(oldname, newname string) error {
@@ -475,43 +486,31 @@ func (fs *fileSystem) Rename(oldname, newname string) error {
                }
        }
 
-       // Return ErrInvalidOperation if olddirf.inode doesn't even
-       // bother calling our "remove oldname entry" replacer func.
-       err = ErrInvalidArgument
-       olddirf.inode.Child(oldname, func(oldinode inode) inode {
-               err = nil
+       _, err = olddirf.inode.Child(oldname, func(oldinode inode) (inode, error) {
                if oldinode == nil {
-                       err = os.ErrNotExist
-                       return nil
+                       return oldinode, os.ErrNotExist
                }
                if locked[oldinode] {
                        // oldinode cannot become a descendant of itself.
-                       err = ErrInvalidArgument
-                       return oldinode
+                       return oldinode, ErrInvalidArgument
                }
                if oldinode.FS() != cfs && newdirf.inode != olddirf.inode {
                        // moving a mount point to a different parent
                        // is not (yet) supported.
-                       err = ErrInvalidArgument
-                       return oldinode
+                       return oldinode, ErrInvalidArgument
                }
-               accepted := newdirf.inode.Child(newname, func(existing inode) inode {
+               accepted, err := newdirf.inode.Child(newname, func(existing inode) (inode, error) {
                        if existing != nil && existing.IsDir() {
-                               err = ErrIsDirectory
-                               return existing
+                               return existing, ErrIsDirectory
                        }
-                       return oldinode
+                       return oldinode, nil
                })
-               if accepted != oldinode {
-                       if err == nil {
-                               // newdirf didn't accept oldinode.
-                               err = ErrInvalidArgument
-                       }
+               if err != nil {
                        // Leave oldinode in olddir.
-                       return oldinode
+                       return oldinode, err
                }
                accepted.SetParent(newdirf.inode, newname)
-               return nil
+               return nil, nil
        })
        return err
 }
@@ -530,27 +529,25 @@ func (fs *fileSystem) RemoveAll(name string) error {
        return err
 }
 
-func (fs *fileSystem) remove(name string, recursive bool) (err error) {
+func (fs *fileSystem) remove(name string, recursive bool) error {
        dirname, name := path.Split(name)
        if name == "" || name == "." || name == ".." {
                return ErrInvalidArgument
        }
-       dir := rlookup(fs.root, dirname)
-       if dir == nil {
-               return os.ErrNotExist
+       dir, err := rlookup(fs.root, dirname)
+       if err != nil {
+               return err
        }
        dir.Lock()
        defer dir.Unlock()
-       dir.Child(name, func(node inode) inode {
+       _, err = dir.Child(name, func(node inode) (inode, error) {
                if node == nil {
-                       err = os.ErrNotExist
-                       return nil
+                       return nil, os.ErrNotExist
                }
                if !recursive && node.IsDir() && node.Size() > 0 {
-                       err = ErrDirectoryNotEmpty
-                       return node
+                       return node, ErrDirectoryNotEmpty
                }
-               return nil
+               return nil, nil
        })
        return err
 }
@@ -563,12 +560,9 @@ func (fs *fileSystem) Sync() error {
 // rlookup (recursive lookup) returns the inode for the file/directory
 // with the given name (which may contain "/" separators). If no such
 // file/directory exists, the returned node is nil.
-func rlookup(start inode, path string) (node inode) {
+func rlookup(start inode, path string) (node inode, err error) {
        node = start
        for _, name := range strings.Split(path, "/") {
-               if node == nil {
-                       break
-               }
                if node.IsDir() {
                        if name == "." || name == "" {
                                continue
@@ -578,11 +572,17 @@ func rlookup(start inode, path string) (node inode) {
                                continue
                        }
                }
-               node = func() inode {
+               node, err = func() (inode, error) {
                        node.RLock()
                        defer node.RUnlock()
                        return node.Child(name, nil)
                }()
+               if node == nil || err != nil {
+                       break
+               }
+       }
+       if node == nil && err == nil {
+               err = os.ErrNotExist
        }
        return
 }

diff --git a/sdk/go/arvados/fs_collection.go b/sdk/go/arvados/fs_collection.go
index fbd9775b0cde0876e68328fd63a019485eeac4a9..923615ba768c003f957c70623c747dbb459b40e2 100644 (file)
--- a/sdk/go/arvados/fs_collection.go
+++ b/sdk/go/arvados/fs_collection.go
@@ -521,7 +521,7 @@ func (dn *dirnode) FS() FileSystem {
        return dn.fs
 }
 
-func (dn *dirnode) Child(name string, replace func(inode) inode) inode {
+func (dn *dirnode) Child(name string, replace func(inode) (inode, error)) (inode, error) {
        if dn == dn.fs.rootnode() && name == ".arvados#collection" {
                gn := &getternode{Getter: func() ([]byte, error) {
                        var coll Collection
@@ -537,7 +537,7 @@ func (dn *dirnode) Child(name string, replace func(inode) inode) inode {
                        return data, err
                }}
                gn.SetParent(dn, name)
-               return gn
+               return gn, nil
        }
        return dn.treenode.Child(name, replace)
 }
@@ -837,38 +837,41 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
                        node = node.Parent()
                        continue
                }
-               node.Child(name, func(child inode) inode {
+               node, err = node.Child(name, func(child inode) (inode, error) {
                        if child == nil {
-                               child, err = node.FS().newNode(name, 0755|os.ModeDir, node.Parent().FileInfo().ModTime())
+                               child, err := node.FS().newNode(name, 0755|os.ModeDir, node.Parent().FileInfo().ModTime())
+                               if err != nil {
+                                       return nil, err
+                               }
                                child.SetParent(node, name)
-                               node = child
+                               return child, nil
                        } else if !child.IsDir() {
-                               err = ErrFileExists
+                               return child, ErrFileExists
                        } else {
-                               node = child
+                               return child, nil
                        }
-                       return child
                })
                if err != nil {
                        return
                }
        }
-       node.Child(basename, func(child inode) inode {
+       _, err = node.Child(basename, func(child inode) (inode, error) {
                switch child := child.(type) {
                case nil:
                        child, err = node.FS().newNode(basename, 0755, node.FileInfo().ModTime())
+                       if err != nil {
+                               return nil, err
+                       }
                        child.SetParent(node, basename)
                        fn = child.(*filenode)
-                       return child
+                       return child, nil
                case *filenode:
                        fn = child
-                       return child
+                       return child, nil
                case *dirnode:
-                       err = ErrIsDirectory
-                       return child
+                       return child, ErrIsDirectory
                default:
-                       err = ErrInvalidArgument
-                       return child
+                       return child, ErrInvalidArgument
                }
        })
        return

diff --git a/sdk/go/arvados/fs_collection_test.go b/sdk/go/arvados/fs_collection_test.go
index 023226ff128880a186b94c15e76ec337cd606e6f..8d32eb2473fcfc4945a3ceffc7ea5a99fd721981 100644 (file)
--- a/sdk/go/arvados/fs_collection_test.go
+++ b/sdk/go/arvados/fs_collection_test.go
@@ -688,13 +688,13 @@ func (s *CollectionFSSuite) TestRename(c *check.C) {
                                err = fs.Rename(
                                        fmt.Sprintf("dir%d/file%d/patherror", i, j),
                                        fmt.Sprintf("dir%d/irrelevant", i))
-                               c.Check(err, check.ErrorMatches, `.*does not exist`)
+                               c.Check(err, check.ErrorMatches, `.*not a directory`)
 
                                // newname parent dir is a file
                                err = fs.Rename(
                                        fmt.Sprintf("dir%d/dir%d/file%d", i, j, j),
                                        fmt.Sprintf("dir%d/file%d/patherror", i, inner-j-1))
-                               c.Check(err, check.ErrorMatches, `.*does not exist`)
+                               c.Check(err, check.ErrorMatches, `.*not a directory`)
                        }(i, j)
                }
        }

diff --git a/sdk/go/arvados/fs_deferred.go b/sdk/go/arvados/fs_deferred.go
index 97fe68b28c77942ff1570f58fb9b035ebf942231..a84f64fe7e254334a00a42226d0003f682d53684 100644 (file)
--- a/sdk/go/arvados/fs_deferred.go
+++ b/sdk/go/arvados/fs_deferred.go
@@ -85,7 +85,7 @@ func (dn *deferrednode) Write(p []byte, pos filenodePtr) (int, filenodePtr, erro
        return dn.realinode().Write(p, pos)
 }
 
-func (dn *deferrednode) Child(name string, replace func(inode) inode) inode {
+func (dn *deferrednode) Child(name string, replace func(inode) (inode, error)) (inode, error) {
        return dn.realinode().Child(name, replace)
 }
 

diff --git a/sdk/go/arvados/fs_getternode.go b/sdk/go/arvados/fs_getternode.go
index c9ffb387d346d377e371bca8b003f4ab2a975bec..966fe9d5c04187829ebab9f065ea765355386fff 100644 (file)
--- a/sdk/go/arvados/fs_getternode.go
+++ b/sdk/go/arvados/fs_getternode.go
@@ -23,8 +23,8 @@ func (*getternode) IsDir() bool {
        return false
 }
 
-func (*getternode) Child(string, func(inode) inode) inode {
-       return nil
+func (*getternode) Child(string, func(inode) (inode, error)) (inode, error) {
+       return nil, ErrInvalidArgument
 }
 
 func (gn *getternode) get() error {

diff --git a/sdk/go/arvados/fs_project.go b/sdk/go/arvados/fs_project.go
index f9cb7997023a28d97fbc3b94035f113a4bc3edd2..a5e4710633b9952b748d3535b8feb36a7594c1da 100644 (file)
--- a/sdk/go/arvados/fs_project.go
+++ b/sdk/go/arvados/fs_project.go
@@ -5,7 +5,6 @@
 package arvados
 
 import (
-       "log"
        "os"
        "sync"
 )
@@ -48,8 +47,8 @@ func (pn *projectnode) setup() {
                        if coll.Name == "" {
                                continue
                        }
-                       pn.inode.Child(coll.Name, func(inode) inode {
-                               return deferredCollectionFS(fs, pn, coll)
+                       pn.inode.Child(coll.Name, func(inode) (inode, error) {
+                               return deferredCollectionFS(fs, pn, coll), nil
                        })
                }
                params.Filters = append(filters, Filter{"uuid", ">", resp.Items[len(resp.Items)-1].UUID})
@@ -71,8 +70,8 @@ func (pn *projectnode) setup() {
                        if group.Name == "" || group.Name == "." || group.Name == ".." {
                                continue
                        }
-                       pn.inode.Child(group.Name, func(inode) inode {
-                               return fs.newProjectNode(pn, group.Name, group.UUID)
+                       pn.inode.Child(group.Name, func(inode) (inode, error) {
+                               return fs.newProjectNode(pn, group.Name, group.UUID), nil
                        })
                }
                params.Filters = append(filters, Filter{"uuid", ">", resp.Items[len(resp.Items)-1].UUID})
@@ -87,30 +86,35 @@ func (pn *projectnode) Readdir() ([]os.FileInfo, error) {
        return pn.inode.Readdir()
 }
 
-func (pn *projectnode) Child(name string, replace func(inode) inode) inode {
+func (pn *projectnode) Child(name string, replace func(inode) (inode, error)) (inode, error) {
        pn.setupOnce.Do(pn.setup)
        if pn.err != nil {
-               log.Printf("BUG: not propagating error setting up %T %v: %s", pn, pn, pn.err)
-               // TODO: propagate error, instead of just being empty
-               return nil
+               return nil, pn.err
        }
        if replace == nil {
                // lookup
                return pn.inode.Child(name, nil)
        }
-       return pn.inode.Child(name, func(existing inode) inode {
-               if repl := replace(existing); repl == nil {
-                       // delete
+       return pn.inode.Child(name, func(existing inode) (inode, error) {
+               if repl, err := replace(existing); err != nil {
+                       return existing, err
+               } else if repl == nil {
+                       if existing == nil {
+                               return nil, nil
+                       }
+                       // rmdir
                        // (TODO)
-                       return pn.Child(name, nil) // not implemented
+                       return existing, ErrInvalidArgument
+               } else if existing != nil {
+                       // clobber
+                       return existing, ErrInvalidArgument
                } else if repl.FileInfo().IsDir() {
                        // mkdir
                        // TODO: repl.SetParent(pn, name), etc.
-                       return pn.Child(name, nil) // not implemented
+                       return existing, ErrInvalidArgument
                } else {
                        // create file
-                       // TODO: repl.SetParent(pn, name), etc.
-                       return pn.Child(name, nil) // not implemented
+                       return existing, ErrInvalidArgument
                }
        })
 }

diff --git a/sdk/go/arvados/fs_site.go b/sdk/go/arvados/fs_site.go
index c8d7360ae420bfdd46b8325c5a6abc7eb9c058b3..701711e21777b5645f923e4258b2e4b2cdf1dce4 100644 (file)
--- a/sdk/go/arvados/fs_site.go
+++ b/sdk/go/arvados/fs_site.go
@@ -37,7 +37,7 @@ func (c *Client) SiteFileSystem(kc keepClient) FileSystem {
                },
                inodes: make(map[string]inode),
        }
-       root.inode.Child("by_id", func(inode) inode {
+       root.inode.Child("by_id", func(inode) (inode, error) {
                return &vdirnode{
                        inode: &treenode{
                                fs:     fs,
@@ -50,10 +50,10 @@ func (c *Client) SiteFileSystem(kc keepClient) FileSystem {
                                },
                        },
                        create: fs.mountCollection,
-               }
+               }, nil
        })
-       root.inode.Child("home", func(inode) inode {
-               return fs.newProjectNode(fs.root, "home", "")
+       root.inode.Child("home", func(inode) (inode, error) {
+               return fs.newProjectNode(fs.root, "home", ""), nil
        })
        return fs
 }
@@ -104,18 +104,23 @@ type vdirnode struct {
        create func(parent inode, name string) inode
 }
 
-func (vn *vdirnode) Child(name string, _ func(inode) inode) inode {
-       return vn.inode.Child(name, func(existing inode) inode {
-               if existing != nil {
-                       return existing
-               } else if vn.create == nil {
-                       return nil
+func (vn *vdirnode) Child(name string, replace func(inode) (inode, error)) (inode, error) {
+       return vn.inode.Child(name, func(existing inode) (inode, error) {
+               if existing == nil && vn.create != nil {
+                       existing = vn.create(vn, name)
+                       if existing != nil {
+                               existing.SetParent(vn, name)
+                               vn.inode.(*treenode).fileinfo.modTime = time.Now()
+                       }
                }
-               n := vn.create(vn, name)
-               if n != nil {
-                       n.SetParent(vn, name)
-                       vn.inode.(*treenode).fileinfo.modTime = time.Now()
+               if replace == nil {
+                       return existing, nil
+               } else if tryRepl, err := replace(existing); err != nil {
+                       return existing, err
+               } else if tryRepl != existing {
+                       return existing, ErrInvalidArgument
+               } else {
+                       return existing, nil
                }
-               return n
        })
 }