documentation notes

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()

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)
 }

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

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 {