Fuse v2 #4772
Fuse v2 #4772
Conversation
|
Your website preview is ready! Hooray! 🎉 Built with commit eecef1f |
src/client/pkg/require/require.go
Outdated
| if len(msgArgs) > 0 { | ||
| err, ok := msgArgs[0].(error) | ||
| if ok { | ||
| var st go_errors.StackTrace |
There was a problem hiding this comment.
We shouldn't need to do this, our errors package should just re-export this like it does everything else.
src/client/pkg/require/require.go
Outdated
| err, ok := msgArgs[0].(error) | ||
| if ok { | ||
| var st go_errors.StackTrace | ||
| for err != nil { |
There was a problem hiding this comment.
This might be a good util to put in the errors package as an exported function.
There was a problem hiding this comment.
Good call, it's copy pasted twice in this review so should be a util.
src/server/pfs/cmds/mount_unix.go
Outdated
| } | ||
| } | ||
| if flag != "" { | ||
| if flag == "w" || flag == "rw" { |
There was a problem hiding this comment.
This assumes that 'r' will always precede 'w' - chmod doesn't seem to require this. I think you should just check for the presence of each character and error on unrecognized chars.
There was a problem hiding this comment.
Changed, the only thing weird about it is that people can pass letters multiple times, i.e. "rwwrwrw" is the same as "rw" I didn't really see any reason to prevent that.
There was a problem hiding this comment.
Yeah, chmod allows that as well, so I think it's fine to copy their semantics.
| var oneOff bool | ||
| var repo string | ||
| var branch string | ||
|
|
||
| var pr *io.PipeReader | ||
| var pw *io.PipeWriter |
There was a problem hiding this comment.
Is it just me or are there a lot of places in this function that don't close pw? Not really related to this change, though.
There was a problem hiding this comment.
The code here is kinda convoluted, but I think each io.Pipe() call has a matching pw.Close() call. There's two close sites, one that happens before we assign a new value to pw and one that happens at the end no matter what. That one is actually a little confusing, because it actually calls pw.CloseWithError(err), which makes it look like it's handling an error, rather than normal execution, but under normal execution err will be equal to io.EOF and pw.Close() is actually just implemented as pw.CloseWithError(io.EOF) so I simplified it a bit.
src/server/pfs/fuse/fuse.go
Outdated
| } | ||
| server, err := fs.Mount(target, root, opts.getFuse()) | ||
| if err != nil { | ||
| return errors.Wrap(err, "fs.Mount") |
There was a problem hiding this comment.
For places like this (where the stack makes it clear where the error came from, and the wrap isn't adding any new information), I added an errors.WithStack to use instead of errors.Wrap. Also, this should be used for any errors from non-pachyderm code, like the ioutil.TempDir and os.RemoveAll above.
Co-authored-by: Tryneus <marc.hesse@gmail.com>
Co-authored-by: Tryneus <marc.hesse@gmail.com>
Co-authored-by: Tryneus <marc.hesse@gmail.com>
|
Alright, I think I addressed everything here. Ready for another pass, make sure to take a look at the error stack traversing / printing code. I factored that into an exported function, which I think covers everything you were asking for but I'm not 100% sure I understood what you were asking for. |
src/server/pfs/fuse/fuse.go
Outdated
| commits[repo] = branch | ||
| } | ||
| } | ||
| // branches := opts.getBranches() |
Migrating them fucked me up quite nicely >.<
This PR does two things:
This also meaningfully changes the architecture of the fuse client. It's now implemented as a loopback filesystem, which the fuse library already had a built in implementation of that used as the starting point. This saves us from having to actually implement the full POSIX filesystem API, since we can just proxy calls to the underlying filesystem. The only thing that needed to be added was code to materialize the filesystem as the user requests pieces of it. So while this is more code than the system it's replacing, it's simpler because most of the code is just proxying various requests to and from the filesystem.
TODO:
consider on-disk caching to speed up remounts