BSD flags support in cgofuse

[billziss-gh]

A recent issue (winfsp/winfsp#114) in WinFsp asked for BSD flags support in the FUSE layer. I have added this functionality to WinFsp-FUSE and I am looking to add it to cgofuse.

The proposed changes to cgofuse are as follows:

• Add a Flags uint32 field to struct Stat_t: [link]. File systems that wish to support this functionality may fill the Flags field during Getattr.

• Add a Chflags(path string, flags uint32) int method to FileSystemInterface: [link]

• Add the BSD/OSX UF_HIDDEN, UF_READONLY, UF_SYSTEM, UF_ARCHIVE constants.

• Add func (host *FileSystemHost) SetCapBsdFlags(value bool) to FileSystemHost. When this capability is enabled, the file system is expected to fill the Flags field of struct Stat_t. It may also optionally implement Chflags. QUESTION: do we really need this or should we assume that this capability is always enabled for cgofuse file systems?

• This functionality will be supported on WinFsp-FUSE and OSXFUSE, as they both support the BSD flags functionality.

@ncw my main concern with this scheme is backward compatibility. Since releasing a new go binary requires recompilation these changes look backward compatible and safe to me. Since you have a lot more experience with go than me, I would appreciate your feedback.

[ncw]

Here are some thoughts about backwards compatibility for you

Add a Flags uint32 field to struct Stat_t. File systems that wish to support this functionality may fill the Flags field during Getattr.

I think that is fine. In general adding new fields to structs is an encouraged way of adding new features in a backwards compatible way. In go you are always encouraged to used the named fields of structs rather than initialise them with unnamed fields (go vet will warn if you don't) so I can't see this causing a problem

Add a Chflags(path string, flags uint32) int method to FileSystemInterface

This potentially breaks code as you are adding an item to an interface that the user has to specify. When the user calls fuse.NewFileSystemHost(fsys) it may break at that point unless they embedded FileSystemBase. I chose not to do that in rclone to make sure I implemented all the features.

However it is a really clean break - the code won't compile and the error message will say something about a missing method "Chflags" which makes it a really easy fix.

You have provided FileSystemBase so you might argue that any breakage is the users fault...

Personally I'd be happy with a break like that with rclone. I vendor all of rclone's dependencies and I update them early in the release cycle normally. I expect a small amount of breakage at that point. You may feel differently though!

If you wanted to avoid the break, then you would make an additional interface say

type Chflagser interface {
    Chflags() int // or whatever
}

which the fsys could optionally support and you could do a type assertion to look for its presence.

This might make a lot of sense - if the interface is present then you know the fsys supports the new flags for definite.

(Aside: Some might argue that the pattern of having a FileSystemBase and a FileSystemInterface with lots of methods not all of which are required would be better satisfied by having smaller interfaces for each method which you type assert to discover the capabilities. This is the way that bazil/fuse does it. However it is too late to change that now!)

Add the BSD/OSX UF_HIDDEN, UF_READONLY, UF_SYSTEM, UF_ARCHIVE constants.

I think these are uncontroversial.

Add func (host *FileSystemHost) SetCapBsdFlags(value bool) to FileSystemHost. When this capability is enabled, the file system is expected to fill the Flags field of struct Stat_t. It may also optionally implement Chflags

That seems reasonable.

QUESTION: do we really need this or should we assume that this capability is always enabled for cgofuse file systems?

If the flags default to 0 then it probably isn't necessary as unset fields in the struct will be 0.

[billziss-gh]

Thank you for the well thought-out comments.

If you wanted to avoid the break, then you would make an additional interface say

type Chflagser interface {
    Chflags() int // or whatever
}

I like this idea and will probably do as you are suggesting. BTW, WinFsp-FUSE now supports the OSXFUSE methods: chflags, setcrtime, setchgtime. If I want to add all these new operations in cgofuse, would your recommendation be to add one interface per method?

type Chflagser interface {
    Chflags() int
}

type Setcrtimer interface {
    Setcrtime() int
}

type Setchgtimer interface {
    Setchgtime() int
}

[I must admit that I am not hugely fond of this proliferation of interfaces (not to mention the funny interface names that Go recommends).]

QUESTION: do we really need this or should we assume that this capability is always enabled for cgofuse file systems?

If the flags default to 0 then it probably isn't necessary as unset fields in the struct will be 0.

I agree. SetCapBsdFlags is unnecessary.

(Aside: Some might argue that the pattern of having a FileSystemBase and a FileSystemInterface with lots of methods not all of which are required would be better satisfied by having smaller interfaces for each method which you type assert to discover the capabilities. This is the way that bazil/fuse does it. However it is too late to change that now!)

This is something I often struggle with. Sometimes I think that a better solution for such problems is the following:

package main

import "fmt"

type FileSystemOperations struct {
	Chflags func(string, uint32) int
	// ...
}

func mychflags(path string, flags uint32) int {
	fmt.Println(path, flags)
	return 0
}

func main() {
	ops := FileSystemOperations{Chflags: mychflags}
	if nil != ops.Chflags {
		ops.Chflags("hello", 42)
	}
}

Looks like writing C with Go syntax, but appears to me more flexible.

[ncw]

I like this idea and will probably do as you are suggesting. BTW, WinFsp-FUSE now supports the OSXFUSE methods: chflags, setcrtime, setchgtime. If I want to add all these new operations in cgofuse, would your recommendation be to add one interface per method?

Yes, unless you can't implement one method without another.

[I must admit that I am not hugely fond of this proliferation of interfaces (not to mention the funny interface names that Go recommends).]

You don't actually need to name interfaces, but I think you'll want to so users can check that their implementation defines all the interfaces you expect.

Yes interface proliferation is a problem and the -er naming sometimes works but sometimes doesn't. I'd say feel free to name them differently - looking through the go stdlib only about ½ of the interfaces are named in that style.

[billziss-gh]

Commit 58d06a2 adds support for BSD flags, chflags, setcrtime and setchgtime.
Commit 4644d6a adds the BSD UF_* constants.
Commit d5b944d adds chflags, setcrtime, setchgtime to memfs.

As per your suggestion I created three new interfaces FileSystemChflags, FileSystemSetcrtime and FileSystemSetchgtime. If you decide to take a look, let me know if you have any feedback.

[ncw]

I gave the code a quick review - it looks great :-)

[billziss-gh]

Thanks. I will merge into master and close this.