From 08922d239d3c866ea886d5e6ebec8cb4f9ba94a4 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Fri, 29 Jun 2018 10:57:34 -0300 Subject: [PATCH 1/3] mfs: make `Root` value a `Directory` Make `Root` value explicitly a `Directory` structure instead of the `FSNode` interface (which also allowed the `File` type). This helps to make the code easier to reason about: the root of an MFS layout is always a directory, not a (single) file. Rename `GetValue()` to `GetDirectory()` to also make it more explicit, the renamed function now returns a `Directory` so there is no need for type assertions that were previously done on the `FSNode` interface to check that it was actually a `Directory`. `NewRoot()` now doesn't allow to create `Root` structures from DAG nodes that contain UnixFS files. License: MIT Signed-off-by: Lucas Molas --- core/corerepo/gc.go | 2 +- core/coreunix/add.go | 10 ++++------ fuse/ipns/ipns_unix.go | 9 +-------- mfs/mfs_test.go | 22 +++++++++++----------- mfs/ops.go | 12 +++++------- mfs/system.go | 29 +++++++++++------------------ 6 files changed, 33 insertions(+), 51 deletions(-) diff --git a/core/corerepo/gc.go b/core/corerepo/gc.go index f856d3ace79..389dc971d6e 100644 --- a/core/corerepo/gc.go +++ b/core/corerepo/gc.go @@ -71,7 +71,7 @@ func NewGC(n *core.IpfsNode) (*GC, error) { } func BestEffortRoots(filesRoot *mfs.Root) ([]*cid.Cid, error) { - rootDag, err := filesRoot.GetValue().GetNode() + rootDag, err := filesRoot.GetDirectory().GetNode() if err != nil { return nil, err } diff --git a/core/coreunix/add.go b/core/coreunix/add.go index 09b662dcf3e..f4e9234624c 100644 --- a/core/coreunix/add.go +++ b/core/coreunix/add.go @@ -144,7 +144,7 @@ func (adder *Adder) RootNode() (ipld.Node, error) { if err != nil { return nil, err } - root, err := mr.GetValue().GetNode() + root, err := mr.GetDirectory().GetNode() if err != nil { return nil, err } @@ -199,7 +199,8 @@ func (adder *Adder) Finalize() (ipld.Node, error) { if err != nil { return nil, err } - root := mr.GetValue() + var root mfs.FSNode + root = mr.GetDirectory() err = root.Flush() if err != nil { @@ -224,10 +225,7 @@ func (adder *Adder) Finalize() (ipld.Node, error) { return nil, err } - dir, ok := mr.GetValue().(*mfs.Directory) - if !ok { - return nil, fmt.Errorf("root is not a directory") - } + dir := mr.GetDirectory() root, err = dir.Child(name) if err != nil { diff --git a/fuse/ipns/ipns_unix.go b/fuse/ipns/ipns_unix.go index f7debc0b2cb..4f29d547dbc 100644 --- a/fuse/ipns/ipns_unix.go +++ b/fuse/ipns/ipns_unix.go @@ -118,14 +118,7 @@ func loadRoot(ctx context.Context, rt *keyRoot, ipfs *core.IpfsNode, name string rt.root = root - switch val := root.GetValue().(type) { - case *mfs.Directory: - return &Directory{dir: val}, nil - case *mfs.File: - return &FileNode{fi: val}, nil - default: - return nil, errors.New("unrecognized type") - } + return &Directory{dir: root.GetDirectory()}, nil } type keyRoot struct { diff --git a/mfs/mfs_test.go b/mfs/mfs_test.go index 9d03b45f22d..85ac54d0392 100644 --- a/mfs/mfs_test.go +++ b/mfs/mfs_test.go @@ -213,7 +213,7 @@ func TestBasic(t *testing.T) { defer cancel() ds, rt := setupRoot(ctx, t) - rootdir := rt.GetValue().(*Directory) + rootdir := rt.GetDirectory() // test making a basic dir _, err := rootdir.Mkdir("a") @@ -243,7 +243,7 @@ func TestMkdir(t *testing.T) { defer cancel() _, rt := setupRoot(ctx, t) - rootdir := rt.GetValue().(*Directory) + rootdir := rt.GetDirectory() dirsToMake := []string{"a", "B", "foo", "bar", "cats", "fish"} sort.Strings(dirsToMake) // sort for easy comparing later @@ -281,7 +281,7 @@ func TestDirectoryLoadFromDag(t *testing.T) { defer cancel() ds, rt := setupRoot(ctx, t) - rootdir := rt.GetValue().(*Directory) + rootdir := rt.GetDirectory() nd := getRandFile(t, ds, 1000) err := ds.Add(ctx, nd) @@ -373,7 +373,7 @@ func TestMfsFile(t *testing.T) { defer cancel() ds, rt := setupRoot(ctx, t) - rootdir := rt.GetValue().(*Directory) + rootdir := rt.GetDirectory() fisize := 1000 nd := getRandFile(t, ds, 1000) @@ -686,7 +686,7 @@ func actorReadFile(d *Directory) error { } func testActor(rt *Root, iterations int, errs chan error) { - d := rt.GetValue().(*Directory) + d := rt.GetDirectory() for i := 0; i < iterations; i++ { switch rand.Intn(5) { case 0: @@ -763,7 +763,7 @@ func TestConcurrentWriteAndFlush(t *testing.T) { defer cancel() ds, rt := setupRoot(ctx, t) - d := mkdirP(t, rt.GetValue().(*Directory), "foo/bar/baz") + d := mkdirP(t, rt.GetDirectory(), "foo/bar/baz") fn := fileNodeFromReader(t, ds, bytes.NewBuffer(nil)) err := d.AddChild("file", fn) if err != nil { @@ -786,7 +786,7 @@ func TestConcurrentWriteAndFlush(t *testing.T) { }() for i := 0; i < nloops; i++ { - _, err := rt.GetValue().GetNode() + _, err := rt.GetDirectory().GetNode() if err != nil { t.Fatal(err) } @@ -800,7 +800,7 @@ func TestFlushing(t *testing.T) { defer cancel() _, rt := setupRoot(ctx, t) - dir := rt.GetValue().(*Directory) + dir := rt.GetDirectory() c := mkdirP(t, dir, "a/b/c") d := mkdirP(t, dir, "a/b/d") e := mkdirP(t, dir, "a/b/e") @@ -901,7 +901,7 @@ func TestConcurrentReads(t *testing.T) { ds, rt := setupRoot(ctx, t) - rootdir := rt.GetValue().(*Directory) + rootdir := rt.GetDirectory() path := "a/b/c" d := mkdirP(t, rootdir, path) @@ -976,7 +976,7 @@ func TestConcurrentWrites(t *testing.T) { ds, rt := setupRoot(ctx, t) - rootdir := rt.GetValue().(*Directory) + rootdir := rt.GetDirectory() path := "a/b/c" d := mkdirP(t, rootdir, path) @@ -1011,7 +1011,7 @@ func TestFileDescriptors(t *testing.T) { defer cancel() ds, rt := setupRoot(ctx, t) - dir := rt.GetValue().(*Directory) + dir := rt.GetDirectory() nd := dag.NodeWithData(ft.FilePBData(nil, 0)) fi, err := NewFile("test", nd, dir, ds) diff --git a/mfs/ops.go b/mfs/ops.go index 6ade2bee0d3..20d2c5e749d 100644 --- a/mfs/ops.go +++ b/mfs/ops.go @@ -1,7 +1,6 @@ package mfs import ( - "errors" "fmt" "os" gopath "path" @@ -129,7 +128,7 @@ func Mkdir(r *Root, pth string, opts MkdirOpts) error { return fmt.Errorf("cannot create directory '/': Already exists") } - cur := r.GetValue().(*Directory) + cur := r.GetDirectory() for i, d := range parts[:len(parts)-1] { fsn, err := cur.Child(d) if err == os.ErrNotExist && opts.Mkparents { @@ -172,12 +171,11 @@ func Mkdir(r *Root, pth string, opts MkdirOpts) error { return nil } +// Lookup extracts the root directory and performs a lookup under it. +// TODO: Now that the root is always a directory, can this function +// be collapsed with `DirLookup`? Or at least be made a method of `Root`? func Lookup(r *Root, path string) (FSNode, error) { - dir, ok := r.GetValue().(*Directory) - if !ok { - log.Errorf("root not a dir: %#v", r.GetValue()) - return nil, errors.New("root was not a directory") - } + dir := r.GetDirectory() return DirLookup(dir, path) } diff --git a/mfs/system.go b/mfs/system.go index 975d3da67e1..6f3e10badca 100644 --- a/mfs/system.go +++ b/mfs/system.go @@ -53,8 +53,8 @@ type Root struct { // node is the merkledag root. node *dag.ProtoNode - // val represents the node. It can either be a File or a Directory. - val FSNode + // Root directory of the MFS layout. + dir *Directory repub *Republisher @@ -90,33 +90,29 @@ func NewRoot(parent context.Context, ds ipld.DAGService, node *dag.ProtoNode, pf switch pbn.GetType() { case ft.TDirectory, ft.THAMTShard: - rval, err := NewDirectory(parent, node.String(), node, root, ds) + newDir, err := NewDirectory(parent, node.String(), node, root, ds) if err != nil { return nil, err } - root.val = rval + root.dir = newDir case ft.TFile, ft.TMetadata, ft.TRaw: - fi, err := NewFile(node.String(), node, root, ds) - if err != nil { - return nil, err - } - root.val = fi + return nil, fmt.Errorf("root can't be a file (unixfs type: %s)", pbn.GetType()) default: return nil, fmt.Errorf("unrecognized unixfs type: %s", pbn.GetType()) } return root, nil } -// GetValue returns the value of Root. -func (kr *Root) GetValue() FSNode { - return kr.val +// GetDirectory returns the root directory. +func (kr *Root) GetDirectory() *Directory { + return kr.dir } // Flush signals that an update has occurred since the last publish, // and updates the Root republisher. func (kr *Root) Flush() error { - nd, err := kr.GetValue().GetNode() + nd, err := kr.GetDirectory().GetNode() if err != nil { return err } @@ -136,10 +132,7 @@ func (kr *Root) Flush() error { // A better implemented mfs system (one that does smarter internal caching and // refcounting) shouldnt need this method. func (kr *Root) FlushMemFree(ctx context.Context) error { - dir, ok := kr.GetValue().(*Directory) - if !ok { - return fmt.Errorf("invalid mfs structure, root should be a directory") - } + dir := kr.GetDirectory() if err := dir.Flush(); err != nil { return err @@ -172,7 +165,7 @@ func (kr *Root) closeChild(name string, nd ipld.Node, sync bool) error { } func (kr *Root) Close() error { - nd, err := kr.GetValue().GetNode() + nd, err := kr.GetDirectory().GetNode() if err != nil { return err } From ba7338543fcf9c0b6927bdf4c726941e39f2443c Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Fri, 29 Jun 2018 11:51:26 -0300 Subject: [PATCH 2/3] mfs: remove unused `Root` variables `node` and `Type` License: MIT Signed-off-by: Lucas Molas --- mfs/system.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/mfs/system.go b/mfs/system.go index 6f3e10badca..d92d55a2b4e 100644 --- a/mfs/system.go +++ b/mfs/system.go @@ -50,8 +50,6 @@ type FSNode interface { // Root represents the root of a filesystem tree. type Root struct { - // node is the merkledag root. - node *dag.ProtoNode // Root directory of the MFS layout. dir *Directory @@ -59,8 +57,6 @@ type Root struct { repub *Republisher dserv ipld.DAGService - - Type string } // PubFunc is the function used by the `publish()` method. @@ -77,7 +73,6 @@ func NewRoot(parent context.Context, ds ipld.DAGService, node *dag.ProtoNode, pf } root := &Root{ - node: node, repub: repub, dserv: ds, } From f5e7fe28dde7c082e2325276833321671a0f3001 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Fri, 29 Jun 2018 12:04:48 -0300 Subject: [PATCH 3/3] mfs: remove `DAGService` from `Root` The `Root` structure now explicitly contains a `Directory` (instead of an `FSNode` interface), use that `Directory`'s `DAGService` instead of its own `dserv` variable (which was used only once in `closeChild()`). The `DAGService` in the `Root` and the `Directory` was the same (passed as an argument in the `NewRoot` initializer function). This leaves the `Root` structure with only a `Directory` and a `Republisher` and allows to better rethink its role and whether if those two structures should be grouped together (and if that group's name should be `Root`). License: MIT Signed-off-by: Lucas Molas --- mfs/system.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/mfs/system.go b/mfs/system.go index d92d55a2b4e..5324b8bf8dd 100644 --- a/mfs/system.go +++ b/mfs/system.go @@ -55,8 +55,6 @@ type Root struct { dir *Directory repub *Republisher - - dserv ipld.DAGService } // PubFunc is the function used by the `publish()` method. @@ -74,7 +72,6 @@ func NewRoot(parent context.Context, ds ipld.DAGService, node *dag.ProtoNode, pf root := &Root{ repub: repub, - dserv: ds, } pbn, err := ft.FromBytes(node.Data()) @@ -148,7 +145,7 @@ func (kr *Root) FlushMemFree(ctx context.Context) error { // closeChild implements the childCloser interface, and signals to the publisher that // there are changes ready to be published. func (kr *Root) closeChild(name string, nd ipld.Node, sync bool) error { - err := kr.dserv.Add(context.TODO(), nd) + err := kr.GetDirectory().dserv.Add(context.TODO(), nd) if err != nil { return err }