Closed stevegt closed 1 year ago
why do you want to implement the '.' and '..' entries?
@stevegt Yes gocryptfs has . and .. , check out https://github.com/rfjakob/gocryptfs/commit/18befda0e6f1a690fa000951df8f9a4a61daebbb
@hanwen xfstests generic/401
breaks with . and .. missing, but I guess other things will explode too, as everything else provides them.
@rfjakob Oh! I hadn't realized you'd switched to the fs API -- I've still been looking at your 1.8.0 code. Oh, this is excellent. Thank you.
@hanwen I'm tilting at the windmill of hoping to write a POSIXish fuse filesystem written in Go for the UI of an open-source project that we've been working on for the last several months (haven't published yet or I'd have a link for you). I'm at the stage of needing to finally decide between go-fuse, bazil, and jacobsa/fuse. You already know this, but go-fuse seems to win on features, performance, and currency, and all we're missing is more docs and reference code for the v2 fs API. Now that I know gocryptfs has been updated to v2, this is going to help a lot, I think.
I'm working a conference this week, but in between or immediately after I'll see what I can learn from the newer gocryptfs implementation, provide a code snippet here and close this issue myself.
I think it would be easy to make '.' and '..' work correctly. '..' is the parent, so it should be possible to lookup the parent's ino number and populate that in the readdir call.
I just now realized that recent versions of gocryptfs are also now showing a ?
in place of the inode number for the mounted filesystem's root directory "." entry. I've filed an issue over there, but given that I'm running into the same symptom with a toy filesystem, there's a possibility the problem may be here.
As far as I can tell, this line in fs/bridge.go looks like a contributing factor:
out.Ino = n.stableAttr.Ino
At the time that executes, in both gocryptfs and my toy filesystem, n.stableAttr.Ino is equal to zero, which means if we set Ino earlier in e.g. Getattr(), it's gonna get erased.
@hanwen Is there already a way to set StableAttr.Ino on the root node? It seems like this should be a thing, but after an afternoon of digging around I haven't seen a way to do it yet, given that fs.Inode.stableAttr isn't exported -- is there an accessor somewhere I'm missing?
Here's a version of hello
which reproduces the problem -- includes several comments showing a few attempts at a workaround:
package main
import (
"context"
"flag"
"log"
"os"
"os/signal"
"syscall"
"github.com/hanwen/go-fuse/v2/fs"
"github.com/hanwen/go-fuse/v2/fuse"
)
type HelloRoot struct {
fs.Inode
}
var _ = (fs.NodeOnAdder)((*HelloRoot)(nil))
func (r *HelloRoot) OnAdd(ctx context.Context) {
ch := r.NewPersistentInode(ctx,
&fs.MemRegularFile{
Data: []byte("hello big world!\n"),
Attr: fuse.Attr{
Mode: 0644,
},
}, fs.StableAttr{Ino: 10})
r.AddChild("hello.txt", ch, false)
// r.AddChild(".", r.EmbeddedInode(), false)
}
var _ = (fs.NodeGetattrer)((*HelloRoot)(nil))
func (r *HelloRoot) Getattr(ctx context.Context, fh fs.FileHandle, out *fuse.AttrOut) syscall.Errno {
// fmt.Printf("fh %#v\n", fh)
out.Attr.Ino = 1
out.Mode = 0755
return 0
}
/*
var _ = (fs.NodeLookuper)((*HelloRoot)(nil))
func (r *HelloRoot) Lookup(ctx context.Context, name string, out *fuse.EntryOut) (*fs.Inode, syscall.Errno) {
if name == "." {
out.Attr.Ino = 1
return r.EmbeddedInode(), 0
}
return nil, 0
}
*/
func (r *HelloRoot) Readdir(ctx context.Context) (stream fs.DirStream, errno syscall.Errno) {
entries := []fuse.DirEntry{
{Mode: syscall.S_IFDIR, Name: "."},
{Mode: syscall.S_IFDIR, Name: ".."},
}
for name, child := range r.Children() {
entry := fuse.DirEntry{Mode: child.Mode(), Name: name}
entries = append(entries, entry)
}
return fs.NewListDirStream(entries), 0
}
func main() {
debug := flag.Bool("debug", false, "print debug data")
flag.Parse()
if len(flag.Args()) < 1 {
log.Fatal("Usage:\n hello MOUNTPOINT")
}
var server *fuse.Server
// unmount on exit
defer umount(server)
// unmount on SIGINT or SIGTERM
sig := make(chan os.Signal)
signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
go func() {
<-sig
umount(server)
os.Exit(1)
}()
opts := &fs.Options{}
opts.Debug = *debug
opts.AllowOther = true
server, err := fs.Mount(flag.Arg(0), &HelloRoot{}, opts)
if err != nil {
log.Fatalf("Mount fail: %v\n", err)
}
server.Wait()
}
func umount(server *fuse.Server) {
if server != nil {
server.Unmount()
}
}
Checking the ./example directory, looks like the symptom is limited to the fs API. Examples using nodefs instead do show an inode number for the root node:
example | imports | root inode number |
---|---|---|
autounionfs | nodefs, pathfs | 1 |
hello | fs | "?" |
loopback | fs | "?" |
memfs | nodefs | 1 |
multizip | fs | "?" |
The symptom is orthogonal to whether the filesystem implements "." -- just run ls -lid MOUNTPOINT
. I'll rename this issue.
The inode number "1" in the above nodefs examples comes from this stanza in fsmount.go:
if out.Ino == 0 {
out.Ino = nodeId
}
I don't see anything equivalent in the fs API code -- I'm likely missing something, but the only places I see out.Ino explicitly set are two locations, both of which set it to n.stableAttr.Ino, and a third location which sets it to b.automaticIno.
The automatic inode numbering is apparently not kicking in for the root node, and more grepping still hasn't shown me a way to set the root node's stableAttr.Ino explicitly.
I see a few possible solutions:
Simplest would be to wrap the out.Ino assignment in the following if statement.
if out.Ino == 0 {
out.Ino = n.stableAttr.Ino
}
Export fs.Inode.stableAttr to enable users to set rootNode.StableAttr.Ino before mounting the filesystem.
Add a constructor to enable setting fs.Inode.stableAttr bits on the root node.
func NewRootInode(InodeEmbedder, StableAttr) (InodeEmbedder)
As far as I can tell, (1) would fix gocryptfs, since @rfjakob is already setting out.Ino in the root node's Getattr. But I don't know if the caveats in (1) would break anything else.
Exporting fs.Inode.stableAttr as in (2) would require touching 30 lines of code and has that "stable" concern but otherwise looks straightforward.
So far it feels to me like (3) is the right long-term solution -- it closes what appears to me to be a gap in the fs API -- we have NewInode and NewPersistentInode to set StableAttr on non-root nodes, but those methods can't be called for a root node because they need a ctx and a non-nil bridge.
Am I missing anything?
Could we call Getattr on the root node to populate stableAttr ?
Could we call Getattr on the root node to populate stableAttr ?
Unless I'm misunderstanding something, we can't say rootNode.stableAttr.Ino = 42
from inside Getattr because stableAttr isn't exported. I'm feeling a bit thick though -- how could there not be a way of setting StableAttr on the root node?
I meant, could bridge call getattr once on mount
I meant, could bridge call getattr once on mount
That then would be an option (4), calling Inode.Getattr() from someplace during mount and using the resulting out
values to populate the root node's StableAttr. That call to Inode.Getattr() could not be done from the existing rawBridge.getattr() though, because the existing rawBridge.getattr() always discards the out.Ino it gets from Inode.Getattr(). Option (1) would fix the latter.
I'm starting to think we're going to end up with a combination of either (1) and (3) or (1) and (4).
Although it occurs to me that (3) alone, skipping (1), is the least invasive of the four options and least likely to break or inadvertently change the behavior of existing callers.
Looking at this a couple of months later -- after not looking at the code in the elapsed time, my brain sees (3) as easiest to understand conceptually as a caller, and likely easiest to implement and maintain. I've been on a detour chasing several other things in the interim, but will implement (3) when I get back to this if there are no objections and if someone else doesn't do it first.
couldn't we provide stableAttr for the root node in the mount options?
couldn't we provide stableAttr for the root node in the mount options?
You mean pass Ino in as a field in the Options struct in Mount(..., options *Options)
and then assign it to StableAttr.Ino in NewNodeFs instead of what going on here? (I'll call that option (4) for discussion.)
https://github.com/hanwen/go-fuse/blob/24a1dfe6b4f8d478275d5cf671d982c4ddd8c904/fs/bridge.go#L291
Or do you mean pass in an entire StableAttr struct as a field in the mount options? Calling this option (5).
Either makes sense to me -- I was focused on constructors when writing options 1-3 above and didn't think about the mount phase as an opportunity to set Ino.
I've created PR #404 implementing option (5) above -- StableAttr field in Options. Has an example that works, but no new test cases 'cause I haven't yet been able to grok how to follow the go-fuse testing conventions, and didn't want to dive into figuring that out if we actually want option (4) instead.
implemented in 615a0a7. Thanks @adlternative !
implemented in 615a0a7. Thanks @adlternative !
Thanks for review too! :-)
Does anyone have any examples of the right way to implement "." and ".." dirents when using the fs api? Or do most callers just not implement those? I've tried several things but this is the closest I've come -- I've not been able to figure out why the inode number isn't showing up:
That is when using something like this:
Some of the other things I've tried are here, commented out: