Add Capability function to query fs capabilities #59

jfontan · 2018-05-16T09:26:46Z

This adds the Capable interface that can be implemented in filesystems
to return the features suported by it. This first iteration has the
following capablities:

CapWrite
CapRead
CapReadAndWrite
CapSeek
CapTruncate
CapLock

billy.Capablilities(fs) can be used to query a fs. The capabilities
are implemented as bit flags. If a filesystem does not implement Capable
interface then all capabilities will be returned as fall back.

This adds the Capable interface that can be implemented in filesystems to return the features suported by it. This first iteration has the following capablities: * CapWrite * CapRead * CapReadAndWrite * CapSeek * CapTruncate * CapLock `billy.Capablilities(fs)` can be used to query a fs. The capabilities are implemented as bit flags. If a filesystem does not implement Capable interface then all capabilities will be returned as fall back. Signed-off-by: Javi Fontan <jfontan@gmail.com>

Signed-off-by: Javi Fontan <jfontan@gmail.com>

ajnavarro

@smola @mcuadros I like a lot the idea, and with this, we can remove the special compile flag that we have in go-git to be able to use siva filesystem.

But anyways, maybe we need to do a second pass to define capabilities.

smola · 2018-06-04T10:33:35Z

fs.go

+
+// 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 {


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.

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, ...

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.

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

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

smola · 2018-06-04T10:36:03Z

helper/mount/mount.go

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


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.

smola · 2018-06-04T10:38:58Z

osfs/os.go

@@ -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 {


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.

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.

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.

smola · 2018-06-04T10:43:19Z

fs.go

@@ -13,6 +13,28 @@ var (
 	ErrCrossedBoundary = errors.New("chroot boundary crossed")
 )

+// Capability holds the supported features of a filesystem.
+type Capability uint64


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.

CapDefault has all the capabilities supported in the current release and should not be changed until a new major release. Also changed Capabilities of mount to return the common capabilities from both filesystems. Signed-off-by: Javi Fontan <jfontan@gmail.com>

jfontan · 2018-06-04T15:07:14Z

@smola I believe I have addressed your concerns in the latest commit.

@ajnavarro The capabilities added are what I could get from the top of my head and the use cases I have. I agree that more people should take a look and refine them.

mcuadros · 2018-06-06T08:12:44Z

fs.go

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


I would change the names to WriteCapability and so on.

mcuadros · 2018-06-06T08:13:01Z

fs.go

+	// CapDefault lists all capable features supported by filesystems without
+	// Capability interface. This list should not be changed until a major
+	// version is released.
+	CapDefault Capability = CapWrite | CapRead | CapReadAndWrite |


Same here, DefaultCapabilities

Signed-off-by: Javi Fontan <jfontan@gmail.com>

smola · 2018-06-06T17:41:04Z

@jfontan Could you add some more tests? https://codecov.io/gh/src-d/go-billy/compare/df053870ae7070b0350624ba5a22161ba3796cc0...5bb98d7916f7d944463d78e61e9f5b59dd166360/diff

Check capabilities on polyfill, mount and chroot.
Check that Capabilities top-level function falls back to DefaultCapabilities for a filesystem that does not implement it.

Also use mock FS instead of memfs to test capabilities Signed-off-by: Javi Fontan <jfontan@gmail.com>

jfontan · 2018-06-08T09:08:01Z

@smola done

jfontan requested review from smola, ajnavarro and mcuadros May 16, 2018 09:26

Add CapabilityCheck helper function

d198577

Signed-off-by: Javi Fontan <jfontan@gmail.com>

ajnavarro approved these changes Jun 4, 2018

View reviewed changes

smola suggested changes Jun 4, 2018

View reviewed changes

smola approved these changes Jun 5, 2018

View reviewed changes

mcuadros suggested changes Jun 6, 2018

View reviewed changes

Rename capability names to <name>Capability

5bb98d7

Signed-off-by: Javi Fontan <jfontan@gmail.com>

Add capability tests to chroot, mount and polyfill

c1e3d52

Also use mock FS instead of memfs to test capabilities Signed-off-by: Javi Fontan <jfontan@gmail.com>

smola approved these changes Jun 8, 2018

View reviewed changes

mcuadros approved these changes Jun 11, 2018

View reviewed changes

mcuadros merged commit 83cf655 into src-d:master Jun 11, 2018

jfontan mentioned this pull request Jun 25, 2018

New release that comes with capabilities interface #60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Capability function to query fs capabilities #59

Add Capability function to query fs capabilities #59

jfontan commented May 16, 2018

ajnavarro left a comment •

edited

Loading

smola Jun 4, 2018

kuba-- Jun 5, 2018

jfontan Jun 5, 2018

kuba-- Jun 5, 2018

smola Jun 6, 2018

smola Jun 4, 2018

smola Jun 4, 2018

jfontan Jun 4, 2018

smola Jun 5, 2018

smola Jun 4, 2018

jfontan commented Jun 4, 2018

mcuadros Jun 6, 2018

jfontan Jun 6, 2018

mcuadros Jun 6, 2018

jfontan Jun 6, 2018

smola commented Jun 6, 2018

jfontan commented Jun 8, 2018

Add Capability function to query fs capabilities #59

Add Capability function to query fs capabilities #59

Conversation

jfontan commented May 16, 2018

ajnavarro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfontan commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smola commented Jun 6, 2018

jfontan commented Jun 8, 2018

ajnavarro left a comment •

edited

Loading