diff --git a/dir.go b/dir.go index f26b136..d9b3ffa 100644 --- a/dir.go +++ b/dir.go @@ -25,10 +25,14 @@ type Directory struct { dserv ipld.DAGService parent childCloser + // Cache. + // TODO: Should this be a single cache of `FSNode`s? childDirs map[string]*Directory files map[string]*File lock sync.Mutex + // TODO: What content is being protected here exactly? The entire directory? + ctx context.Context // UnixFS directory implementation used for creating, @@ -75,6 +79,9 @@ func (d *Directory) SetCidBuilder(b cid.Builder) { // closeChild updates the child by the given name to the dag node 'nd' // and changes its own dag node func (d *Directory) closeChild(name string, nd ipld.Node, sync bool) error { + + // There's a local flush (`closeChildUpdate`) and a propagated flush (`closeChild`). + mynd, err := d.closeChildUpdate(name, nd, sync) if err != nil { return err @@ -87,10 +94,13 @@ func (d *Directory) closeChild(name string, nd ipld.Node, sync bool) error { } // closeChildUpdate is the portion of closeChild that needs to be locked around +// TODO: Definitely document this. func (d *Directory) closeChildUpdate(name string, nd ipld.Node, sync bool) (*dag.ProtoNode, error) { d.lock.Lock() defer d.lock.Unlock() + // TODO: Clearly define how are we propagating changes to lower layers + // like UnixFS. err := d.updateChild(name, nd) if err != nil { return nil, err @@ -201,6 +211,8 @@ func (d *Directory) Uncache(name string) { defer d.lock.Unlock() delete(d.files, name) delete(d.childDirs, name) + // TODO: We definitely need to join these maps if we are manipulating + // them like this. } // childFromDag searches through this directories dag node for a child link @@ -393,6 +405,7 @@ func (d *Directory) AddUnixFSChild(name string, node ipld.Node) error { return nil } +// TODO: Difference between `sync` and `Flush`. func (d *Directory) sync() error { for name, dir := range d.childDirs { nd, err := dir.GetNode() diff --git a/fd.go b/fd.go index 0f0d3d4..e8664bf 100644 --- a/fd.go +++ b/fd.go @@ -9,6 +9,12 @@ import ( context "context" ) +// One `File` can have many `FileDescriptor`s associated to it +// (only one if it's RW, many if they are RO, see `File.desclock`). +// A `FileDescriptor` contains the "view" of the file (through an +// instance of a `DagModifier`), that's why it (and not the `File`) +// has the responsibility to `Flush` (which cristalizes that view +// in the `File`'s `Node`. type FileDescriptor interface { io.Reader CtxReadFull(context.Context, []byte) (int, error) @@ -32,6 +38,7 @@ type fileDescriptor struct { sync bool hasChanges bool + // TODO: Where is this variable set? closed bool } @@ -84,6 +91,7 @@ func (fi *fileDescriptor) Close() error { case OpenWriteOnly, OpenReadWrite: fi.inode.desclock.Unlock() } + // TODO: `closed` should be set here. }() if fi.closed { @@ -106,6 +114,8 @@ func (fi *fileDescriptor) Close() error { return nil } +// TODO: Who uses `Sync` and who `Flush`? Do the consumers of this API +// know about the (undocumented) `fullsync` argument? func (fi *fileDescriptor) Sync() error { return fi.flushUp(false) } @@ -116,6 +126,7 @@ func (fi *fileDescriptor) Flush() error { // flushUp syncs the file and adds it to the dagservice // it *must* be called with the File's lock taken +// TODO: What is `fullsync`? func (fi *fileDescriptor) flushUp(fullsync bool) error { nd, err := fi.mod.GetNode() if err != nil { @@ -129,9 +140,12 @@ func (fi *fileDescriptor) flushUp(fullsync bool) error { fi.inode.nodelk.Lock() fi.inode.node = nd + // TODO: Create a `SetNode` method. name := fi.inode.name parent := fi.inode.parent + // TODO: Can the parent be modified? Do we need to do this inside the lock? fi.inode.nodelk.Unlock() + // TODO: Maybe all this logic should happen in `File`. return parent.closeChild(name, nd, fullsync) } diff --git a/file.go b/file.go index 0a49646..076d356 100644 --- a/file.go +++ b/file.go @@ -18,10 +18,18 @@ type File struct { name string + // Lock to coordinate the `FileDescriptor`s associated to this file. desclock sync.RWMutex dserv ipld.DAGService + + // This isn't any node, it's the root node that represents the + // entire DAG of nodes that comprise the file. + // TODO: Rename, there should be an explicit term for these root nodes + // of a particular sub-DAG that abstract an upper layer's entity. node ipld.Node + + // TODO: Rename. nodelk sync.Mutex RawLeaves bool @@ -115,6 +123,7 @@ func (fi *File) Size() (int64, error) { } // GetNode returns the dag node associated with this file +// TODO: Use this method and do not access the `nodelk` directly anywhere else. func (fi *File) GetNode() (ipld.Node, error) { fi.nodelk.Lock() defer fi.nodelk.Unlock() @@ -135,6 +144,7 @@ func (fi *File) Flush() error { func (fi *File) Sync() error { // just being able to take the writelock means the descriptor is synced + // TODO: Why? fi.desclock.Lock() fi.desclock.Unlock() return nil diff --git a/system.go b/system.go index 704e5b5..0981f87 100644 --- a/system.go +++ b/system.go @@ -1,4 +1,5 @@ // package mfs implements an in memory model of a mutable IPFS filesystem. +// TODO: Develop on this line (and move it elsewhere), delete the rest. // // It consists of four main structs: // 1) The Filesystem @@ -24,12 +25,23 @@ import ( logging "github.com/ipfs/go-log" ) +// TODO: Remove if not used. var ErrNotExist = errors.New("no such rootfs") var log = logging.Logger("mfs") +// TODO: Remove if not used. var ErrIsDirectory = errors.New("error: is a directory") +// TODO: Rename (avoid "close" terminology, if anything +// we are persisting/flushing changes). +// This is always a directory (since we are referring to the parent), +// can be an intermediate directory in the filesystem or the `Root`. +// TODO: What is `fullsync`? (unnamed `bool` argument) +// TODO: There are two types of persistance/flush that need to be +// distinguished here, one at the DAG level (when I store the modified +// nodes in the DAG service) and one in the UnixFS/MFS level (when I modify +// the entry/link of the directory that pointed to the modified node). type childCloser interface { closeChild(string, ipld.Node, bool) error } @@ -41,7 +53,8 @@ const ( TDir ) -// FSNode represents any node (directory, root, or file) in the mfs filesystem. +// FSNode represents any node (directory, or file) in the MFS filesystem. +// Not to be confused with the `unixfs.FSNode`. type FSNode interface { GetNode() (ipld.Node, error) Flush() error @@ -67,9 +80,6 @@ type Root struct { repub *Republisher } -// PubFunc is the function used by the `publish()` method. -type PubFunc func(context.Context, cid.Cid) error - // NewRoot creates a new Root and starts up a republisher routine for it. func NewRoot(parent context.Context, ds ipld.DAGService, node *dag.ProtoNode, pf PubFunc) (*Root, error) { @@ -142,18 +152,22 @@ func (kr *Root) FlushMemFree(ctx context.Context) error { dir.lock.Lock() defer dir.lock.Unlock() + for name := range dir.files { delete(dir.files, name) } for name := range dir.childDirs { delete(dir.childDirs, name) } + // TODO: Can't we just create new maps? return nil } // closeChild implements the childCloser interface, and signals to the publisher that // there are changes ready to be published. +// This is the only thing that separates a `Root` from a `Directory`. +// TODO: Evaluate merging both. func (kr *Root) closeChild(name string, nd ipld.Node, sync bool) error { err := kr.GetDirectory().dserv.Add(context.TODO(), nd) if err != nil { @@ -180,6 +194,11 @@ func (kr *Root) Close() error { return nil } +// TODO: Separate the remaining code in another file: `republisher.go`. + +// PubFunc is the function used by the `publish()` method. +type PubFunc func(context.Context, cid.Cid) error + // Republisher manages when to publish a given entry. type Republisher struct { TimeoutLong time.Duration @@ -250,6 +269,8 @@ func (np *Republisher) Update(c cid.Cid) { } // Run is the main republisher loop. +// TODO: Document according to: +// https://github.com/ipfs/go-ipfs/issues/5092#issuecomment-398524255. func (np *Republisher) Run() { for { select {