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.

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

Add Capability function to query fs capabilities #59

Merged
merged 5 commits into from
Jun 11, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
46 changes: 46 additions & 0 deletions fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,28 @@ var (
ErrCrossedBoundary = errors.New("chroot boundary crossed")
)

// Capability holds the supported features of a filesystem.
type Capability uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation should be more clear about what a capability means. A capability, defined as filesystem-wide, can only be a hint and not a guarantee, because:

  • Lots of filesystems, including osfs depending on the underlying filesystem, will have additional restrictions depending on the actual filesystem implementation, operating system, mount options, etc.
  • In some cases, even if it is theoretically possible to know that a filesystem is fully read-only (e.g. os filesystem with read-only mount), we just do not check it.

Users should always be prepared to handle gracefully any possible error and do not assume that a capability makes any function returning an error always returning nil.


const (
// CapWrite means that the fs is writable.
CapWrite Capability = 1 << iota
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the names to WriteCapability and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// CapRead means that the fs is readable.
CapRead
// CapReadAndWrite is the ability to open a file in read and write mode.
CapReadAndWrite
// CapSeek means it is able to move position inside the file.
CapSeek
// CapTruncate means that a file can be truncated.
CapTruncate
// CapLock is the ability to lock a file.
CapLock

// CapAll lists all capable features.
CapAll Capability = CapWrite | CapRead | CapReadAndWrite |
CapSeek | CapTruncate | CapLock
)

// Filesystem abstract the operations in a storage-agnostic interface.
// Each method implementation mimics the behavior of the equivalent functions
// at the os package from the standard library.
Expand Down Expand Up @@ -143,3 +165,27 @@ type File interface {
// Truncate the file.
Truncate(size int64) error
}

// Capable interface can return the available features of a filesystem.
type Capable interface {
// Capabilities returns the capabilities of a filesystem in bit flags.
Capabilities() Capability
}

// Capabilities returns the features supported by a filesystem. If the FS
// does not implement Capable interface it returns all features.
func Capabilities(fs Basic) Capability {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to maintain backwards compatibility, this method should assume a fixed set of capabilities for filesystems not implementing Capable, not something that can evolve over time.

Maybe we could rename CapAll to CapDefault and state explicitly in the documentation that CapDefault values will never change without a major version bump.

Copy link

Choose a reason for hiding this comment

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

My comment is silly and very personal :) but just adding 2 cents...
For me Cap sounds like Capacity (especially that we have in go global function cap which returns capacity). So maybe we can rename Capability to Ability (or FSMode) and consts to Readable, Writable, Seekable, Lockable, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is something like this for the interfaces implemented by the FS. This implementation may not be very Goish. It tries to mimic open flags (O_RDONLY | O_SYNC) for operations that are not implemented in the billy FS even if the methods are there. One example is go-billy-siva. The files can not be opened in read and write mode. We can check with this flags before using one algorithm or the other:

if billy.CapabilityCheck(mySivaFS, billy.ReadAndWrite) {
....
}

Other example is memory FS. The locking methods are no-ops.

Capability may be changed to Feature if it seems too close to capacity.

Copy link

Choose a reason for hiding this comment

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

Feature sounds good to me. But I'm not totally against capability, but just abbreviation cap looks more like capacity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using Capability prefix instead of Cap also looks more in line with our convention of avoiding abbreviations, specially if there is any ambiguity.

capable, ok := fs.(Capable)
if !ok {
return CapAll
}

return capable.Capabilities()
}

// CapabilityCheck tests the filesystem for the provided capabilities and
// returns true in case it supports all of them.
func CapabilityCheck(fs Basic, capabilities Capability) bool {
fsCaps := Capabilities(fs)
return fsCaps&capabilities == capabilities
}
37 changes: 37 additions & 0 deletions fs_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package billy_test

import (
"testing"

. "gopkg.in/src-d/go-billy.v4"
"gopkg.in/src-d/go-billy.v4/memfs"

. "gopkg.in/check.v1"
)

type FSSuite struct{}

func Test(t *testing.T) { TestingT(t) }

var _ = Suite(&FSSuite{})

func (s *FSSuite) TestCapabilities(c *C) {
cases := []struct {
caps Capability
expected bool
}{
{CapLock, false},
{CapRead, true},
{CapRead | CapWrite, true},
{CapRead | CapWrite | CapReadAndWrite | CapTruncate, true},
{CapRead | CapWrite | CapReadAndWrite | CapTruncate | CapLock, false},
{CapTruncate | CapLock, false},
}

// This filesystem supports all capabilities except for CapLock
mem := memfs.New()

for _, e := range cases {
c.Assert(CapabilityCheck(mem, e.caps), Equals, e.expected)
}
}
5 changes: 5 additions & 0 deletions helper/chroot/chroot.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,11 @@ func (fs *ChrootHelper) Underlying() billy.Basic {
return fs.underlying
}

// Capabilities implements the Capable interface.
func (fs *ChrootHelper) Capabilities() billy.Capability {
return billy.Capabilities(fs.underlying)
}

type file struct {
billy.File
name string
Expand Down
5 changes: 5 additions & 0 deletions helper/mount/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ func (h *Mount) Underlying() billy.Basic {
return h.underlying
}

// Capabilities implements the Capable interface.
func (fs *Mount) Capabilities() billy.Capability {
return billy.Capabilities(fs.underlying)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why fs.underlying and not fs.source? If Capabilities are global to the filesystem, this should probably return fs.underlying.Capabilities()&fs.source.Capabilities(), which is the common denominator, even if there is some directory with additional capabilities.

}

func (fs *Mount) getBasicAndPath(path string) (billy.Basic, string) {
path = cleanPath(path)
if !fs.isMountpoint(path) {
Expand Down
5 changes: 5 additions & 0 deletions helper/polyfill/polyfill.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,8 @@ func (h *Polyfill) Root() string {
func (h *Polyfill) Underlying() billy.Basic {
return h.Basic
}

// Capabilities implements the Capable interface.
func (h *Polyfill) Capabilities() billy.Capability {
return billy.Capabilities(h.Basic)
}
11 changes: 10 additions & 1 deletion memfs/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,15 @@ func (fs *Memory) Readlink(link string) (string, error) {
return string(f.content.bytes), nil
}

// Capabilities implements the Capable interface.
func (fs *Memory) Capabilities() billy.Capability {
return billy.CapWrite |
billy.CapRead |
billy.CapReadAndWrite |
billy.CapSeek |
billy.CapTruncate
}

type file struct {
name string
content *content
Expand Down Expand Up @@ -273,7 +282,7 @@ func (f *file) Close() error {
func (f *file) Truncate(size int64) error {
if size < int64(len(f.content.bytes)) {
f.content.bytes = f.content.bytes[:size]
} else if more := int(size)-len(f.content.bytes); more > 0 {
} else if more := int(size) - len(f.content.bytes); more > 0 {
f.content.bytes = append(f.content.bytes, make([]byte, more)...)
}

Expand Down
9 changes: 9 additions & 0 deletions memfs/memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package memfs
import (
"testing"

"gopkg.in/src-d/go-billy.v4"
"gopkg.in/src-d/go-billy.v4/test"

. "gopkg.in/check.v1"
Expand All @@ -20,3 +21,11 @@ var _ = Suite(&MemorySuite{})
func (s *MemorySuite) SetUpTest(c *C) {
s.FilesystemSuite = test.NewFilesystemSuite(New())
}

func (s *MemorySuite) TestCapabilities(c *C) {
_, ok := s.FS.(billy.Capable)
c.Assert(ok, Equals, true)

caps := billy.Capabilities(s.FS)
c.Assert(caps, Equals, billy.CapAll&^billy.CapLock)
}
5 changes: 5 additions & 0 deletions osfs/os.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ func (fs *OS) Readlink(link string) (string, error) {
return os.Readlink(link)
}

// Capabilities implements the Capable interface.
func (fs *OS) Capabilities() billy.Capability {
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation of this should state that actual capabilities are dependent on the underlying filesystem. So, for example, if the filesystem is mounted as read-only, it cannot be written and it will return error on opening in write mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice that each filesystem could query these, that is, detect that the FS is mounted read only and do not return CapWrite.

Either way the Capability was more about what was implemented in the billy filesystem than in the underlying OS fs. 👍 to explicitly tell this in the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, adding some checking for osfs would be awesome. And probably it would still be no guarantee, since we might have a nasty fuse filesystem that is mounted as read-write but actually be pure read-only.

return billy.CapAll
}

// file is a wrapper for an os.File which adds support for file locking.
type file struct {
*os.File
Expand Down
12 changes: 11 additions & 1 deletion osfs/os_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ import (
"path/filepath"
"testing"

. "gopkg.in/check.v1"
"gopkg.in/src-d/go-billy.v4"
"gopkg.in/src-d/go-billy.v4/test"

. "gopkg.in/check.v1"
)

func Test(t *testing.T) { TestingT(t) }
Expand Down Expand Up @@ -36,3 +38,11 @@ func (s *OSSuite) TestOpenDoesNotCreateDir(c *C) {
_, err = os.Stat(filepath.Join(s.path, "dir"))
c.Assert(os.IsNotExist(err), Equals, true)
}

func (s *OSSuite) TestCapabilities(c *C) {
_, ok := s.FS.(billy.Capable)
c.Assert(ok, Equals, true)

caps := billy.Capabilities(s.FS)
c.Assert(caps, Equals, billy.CapAll)
}