Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Set fileDescriptor.closed=true in Close() method. #46

Closed
nitishm opened this issue Dec 21, 2018 · 4 comments
Closed

Set fileDescriptor.closed=true in Close() method. #46

nitishm opened this issue Dec 21, 2018 · 4 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws) status/blocked Unable to be worked further until needs are met

Comments

@nitishm
Copy link

nitishm commented Dec 21, 2018

go-mfs/fd.go

Line 41 in e5a375d

closed bool

The fileDescriptor.closed member is never set to anything other than false (by the constructor).

go-mfs/fd.go

Lines 85 to 114 in e5a375d

func (fi *fileDescriptor) Close() error {
defer func() {
switch fi.perms {
case OpenReadOnly:
fi.inode.desclock.RUnlock()
case OpenWriteOnly, OpenReadWrite:
fi.inode.desclock.Unlock()
}
// TODO: `closed` should be set here.
}()
if fi.closed {
panic("attempted to close file descriptor twice!")
}
if fi.hasChanges {
err := fi.mod.Sync()
if err != nil {
return err
}
fi.hasChanges = false
// explicitly stay locked for flushUp call,
// it will manage the lock for us
return fi.flushUp(fi.sync)
}
return nil
}

The only place I can think of this value getting set is in the Close() method. If there is agreement we can set it to true here.

If there is no other use case of the closed member we should just eliminate it.

@nitishm
Copy link
Author

nitishm commented Dec 21, 2018

@schomatis @Stebalien Do you guys agree ?

@schomatis
Copy link
Contributor

@nitishm Thanks for raising this issue so it doesn't get lost in the TODO comments, let's punt on this a bit after we review the entire way the file descriptor operates in #8.

@schomatis schomatis added the status/blocked Unable to be worked further until needs are met label Dec 21, 2018
@schomatis schomatis added this to the Review the MFS interface milestone Dec 21, 2018
@Stebalien Stebalien added the kind/bug A bug in existing code (including security flaws) label Dec 27, 2018
@nitishm
Copy link
Author

nitishm commented Jan 8, 2019

@schomatis @Stebalien I think this can be closed. Its no longer an issue after merging #49

@schomatis
Copy link
Contributor

Yes!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug A bug in existing code (including security flaws) status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

No branches or pull requests

3 participants