diff --git a/dir.go b/dir.go index d488eb2..61f85d0 100644 --- a/dir.go +++ b/dir.go @@ -76,45 +76,27 @@ func (d *Directory) SetCidBuilder(b cid.Builder) { d.unixfsDir.SetCidBuilder(b) } -// This method implements the `parent` interface. It first updates -// the child entry in the underlying UnixFS directory and then, if `fullSync` -// is set, it: -// 1. DAG: saves the newly created directory node with the updated entry. -// 2. MFS: propagates the update upwards (through this same interface) -// repeating the whole process in the parent. -func (d *Directory) updateChildEntry(c child, fullSync bool) error { - - // There's a local flush (`closeChildUpdate`) and a propagated flush (`updateChildEntry`). - - newDirNode, err := d.closeChildUpdate(c, fullSync) +// This method implements the `parent` interface. It first does the local +// update of the child entry in the underlying UnixFS directory and saves +// the newly created directory node with the updated entry in the DAG +// service. Then it propagates the update upwards (through this same +// interface) repeating the whole process in the parent. +func (d *Directory) updateChildEntry(c child) error { + newDirNode, err := d.localUpdate(c) if err != nil { return err } - // TODO: The `sync` seems to be tightly coupling this two pieces of code, - // we use the node returned by `closeChildUpdate` (which entails a copy) - // only if `sync` is set, and we are discarding it otherwise. At the very - // least the `if sync {` clause at the end of `closeChildUpdate` should - // be merged with this one (the use of the `lock` is stopping this at the - // moment, re-evaluate when its purpose has been better understood). - - if fullSync { - return d.parent.updateChildEntry(child{d.name, newDirNode}, true) - // Setting `fullSync` to true here means, if the original child that - // initiated the update process wanted to propagate it upwards then - // continue to do so all the way up to the root, that is, the only - // time `fullSync` can be false is in the first call (which will be - // the *only* call), we either update the first parent entry or *all* - // the parent's. - } - - return nil + // Continue to propagate the update process upwards + // (all the way up to the root). + return d.parent.updateChildEntry(child{d.name, newDirNode}) } // This method implements the part of `updateChildEntry` that needs // to be locked around: in charge of updating the UnixFS layer and -// generating the new node reflecting the update. -func (d *Directory) closeChildUpdate(c child, fullSync bool) (*dag.ProtoNode, error) { +// generating the new node reflecting the update. It also stores the +// new node in the DAG layer. +func (d *Directory) localUpdate(c child) (*dag.ProtoNode, error) { d.lock.Lock() defer d.lock.Unlock() @@ -125,34 +107,20 @@ func (d *Directory) closeChildUpdate(c child, fullSync bool) (*dag.ProtoNode, er // TODO: Clearly define how are we propagating changes to lower layers // like UnixFS. - if fullSync { - return d.flushCurrentNode() - } - return nil, nil -} - -// Recreate the underlying UnixFS directory node and save it in the DAG layer. -func (d *Directory) flushCurrentNode() (*dag.ProtoNode, error) { nd, err := d.unixfsDir.GetNode() if err != nil { return nil, err } - err = d.dagService.Add(d.ctx, nd) - if err != nil { - return nil, err - } - // TODO: This method is called in `closeChildUpdate` while the lock is - // taken, we need the lock while operating on `unixfsDir` to create the - // new node but do we also need to keep the lock while adding it to the - // DAG service? Evaluate refactoring these two methods together and better - // redistributing the node. - pbnd, ok := nd.(*dag.ProtoNode) if !ok { return nil, dag.ErrNotProtobuf } - // TODO: Why do we check the node *after* adding it to the DAG service? + + err = d.dagService.Add(d.ctx, nd) + if err != nil { + return nil, err + } return pbnd.Copy().(*dag.ProtoNode), nil // TODO: Why do we need a copy? @@ -374,7 +342,7 @@ func (d *Directory) Flush() error { return err } - return d.parent.updateChildEntry(child{d.name, nd}, true) + return d.parent.updateChildEntry(child{d.name, nd}) } // AddChild adds the node 'nd' under this directory giving it the name 'name' diff --git a/fd.go b/fd.go index e260a60..ea04bc9 100644 --- a/fd.go +++ b/fd.go @@ -123,9 +123,8 @@ 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`? Propagate the changes upward -// to the root flushing every node in the path (the "up" -// part of `flushUp`). +// If `fullSync` is set the changes are propagated upwards +// (the `Up` part of `flushUp`). func (fi *fileDescriptor) flushUp(fullSync bool) error { nd, err := fi.mod.GetNode() if err != nil { @@ -151,7 +150,11 @@ func (fi *fileDescriptor) flushUp(fullSync bool) error { fi.inode.nodeLock.Unlock() // TODO: Maybe all this logic should happen in `File`. - return parent.updateChildEntry(child{name, nd}, fullSync) + if fullSync { + return parent.updateChildEntry(child{name, nd}) + } + + return nil } // Seek implements io.Seeker diff --git a/root.go b/root.go index 6e91d02..9810961 100644 --- a/root.go +++ b/root.go @@ -45,13 +45,12 @@ type child struct { // the entry/link of the directory that pointed to the modified node). type parent interface { // Method called by a child to its parent to signal to update the content - // pointed to in the entry by that child's name. The child sends as - // arguments its own information (under the `child` structure) and a flag - // (`fullsync`) indicating whether or not to propagate the update upwards: - // modifying a directory entry entails modifying its contents which means - // that its parent (the parent's parent) will also need to be updated (and - // so on). - updateChildEntry(c child, fullSync bool) error + // pointed to in the entry by that child's name. The child sends its own + // information in the `child` structure. As modifying a directory entry + // entails modifying its contents the parent will also call *its* parent's + // `updateChildEntry` to update the entry pointing to the new directory, + // this mechanism is in turn repeated until reaching the `Root`. + updateChildEntry(c child) error } type NodeType int @@ -189,7 +188,7 @@ func (kr *Root) FlushMemFree(ctx context.Context) error { // TODO: The `sync` argument isn't used here (we've already reached // the top), document it and maybe make it an anonymous variable (if // that's possible). -func (kr *Root) updateChildEntry(c child, fullSync bool) error { +func (kr *Root) updateChildEntry(c child) error { err := kr.GetDirectory().dagService.Add(context.TODO(), c.Node) if err != nil { return err