Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mfs: make Root value a Directory #5170

Merged
merged 3 commits into from
Jul 16, 2018
Merged

mfs: make Root value a Directory #5170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08922d2
mfs: make `Root` value a `Directory`
schomatis Jun 29, 2018
ba73385
mfs: remove unused `Root` variables `node` and `Type`
schomatis Jun 29, 2018
f5e7fe2
mfs: remove `DAGService` from `Root`
schomatis Jun 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/corerepo/gc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
10 changes: 4 additions & 6 deletions core/coreunix/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
9 changes: 1 addition & 8 deletions fuse/ipns/ipns_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,7 @@ func loadRoot(ctx context.Context, rt *keyRoot, ipfs *core.IpfsNode, name string

rt.root = root

switch val := root.GetValue().(type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I need to get better about submitting comments instead of editing them till my browser crashes and they disappear...

This is the one real reason to allow the root to be something other than a directory. However, really, the "root" is just a way to tie a file/directory in an MFS tree to a publisher. I'd rather just:

  1. Hold a directory directly as you suggested here MFS Root vs MFS File/Directory #5066 (comment).
  2. Provide some way to "watch" files/directories.

Currently, we have the childCloser but I wonder if we want a more general system. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I need to get better about submitting comments instead of editing them till my browser crashes and they disappear...

I'm glad I'm not the only one, I'm seriously considering submitting a feature request to GitHub that would allow me to explicitly save my drafts..

Thoughts?

I was considering adding that logic to the directory implementation of childCloser, if the directory has a parent call the parent's closeChild (current behavior), if not (you are the root) call the republisher and tell it things have changed. That would require every directory to be aware of the republisher, either with a pointer to it or some other way (which is not nice). Anyway, I need to keep studying how the republisher works and see if I can come up with something better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a more general purpose notification boadcast system where users could register and unregister any number of notifiees (where the parent directory would register itself as a notifiee).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate a bit more on that? I thought the directory was the notifier (I may be misunderstanding the word notifiee, that's the one that receives the notification, right?).

Copy link
Member

@Kubuxu Kubuxu Jul 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated:
You can save a code comment with a "Start Review", comments will be visible only to you until you send the review.

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 {
Expand Down
22 changes: 11 additions & 11 deletions mfs/mfs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
Expand All @@ -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")
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
12 changes: 5 additions & 7 deletions mfs/ops.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package mfs

import (
"errors"
"fmt"
"os"
gopath "path"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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`?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used in many places within the code. I would be okay with making this a method of Root but would not collapse it with DirLookup and DirLookup will work on any directory and not just the 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)
}
Expand Down
39 changes: 12 additions & 27 deletions mfs/system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,11 @@ type FSNode interface {

// Root represents the root of a filesystem tree.
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

dserv ipld.DAGService

Type string
}

// PubFunc is the function used by the `publish()` method.
Expand All @@ -77,9 +71,7 @@ func NewRoot(parent context.Context, ds ipld.DAGService, node *dag.ProtoNode, pf
}

root := &Root{
node: node,
repub: repub,
dserv: ds,
}

pbn, err := ft.FromBytes(node.Data())
Expand All @@ -90,33 +82,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
}
Expand All @@ -136,10 +124,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
Expand All @@ -160,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
}
Expand All @@ -172,7 +157,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
}
Expand Down