Use lateset go-fuse libs and remove dups #159
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the go-fuse library to a newer version and refactors the CachedFileHandle implementation to embed and delegate to *fs.LoopbackFile instead of duplicating file operation implementations. This removes approximately 200 lines of duplicate code (file locking, attribute operations, etc.) and relies on the upstream library's implementations.
Key changes:
- Updates go-fuse from v2.9.0 to v2.9.1-0.20260109090005-01dad59e2820
- Refactors
CachedFileHandleto embed*fs.LoopbackFileand delegate file operations - Removes duplicate implementations of file locking, attributes, and other file operations
- Adds comprehensive test coverage for stat prefetching and small file prefetching features
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates go-fuse dependency to newer version |
| go.sum | Adds checksums for new go-fuse version |
| pkg/sandboxfs/cached_loopback.go | Refactors CachedFileHandle to use embedded LoopbackFile, removes ~200 lines of duplicate code |
| pkg/sandboxfs/prefetch_test.go | Adds new tests for stat prefetching functionality |
| pkg/sandboxfs/small_file_prefetch_test.go | Adds new tests for small file prefetching functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| github.com/googleapis/gax-go/v2 v2.14.0 // indirect | ||
| github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.1.0 // indirect | ||
| github.com/hanwen/go-fuse/v2 v2.9.0 // indirect | ||
| github.com/hanwen/go-fuse/v2 v2.9.1-0.20260109090005-01dad59e2820 // indirect |
There was a problem hiding this comment.
The PR title contains a spelling error: "lateset" should be "latest".
| n, err := syscall.Pwrite(f.fd, data, off) | ||
| if err != nil { | ||
| return 0, fs.ToErrno(err) | ||
| n, errno := f.LoopbackFile.Write(ctx, data, off) |
There was a problem hiding this comment.
The Write method lacks a nil check before calling f.LoopbackFile.Write. If f.LoopbackFile is nil (which can happen after Release sets it to nil, or if switchToCachedFd fails), this will cause a panic. Consider adding a nil check at the beginning of the method similar to the check in switchToCachedFd.
| // Flush implements FileFlusher - commits cache on flush | ||
| func (f *CachedFileHandle) Flush(ctx context.Context) syscall.Errno { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() |
There was a problem hiding this comment.
The Flush method lacks a nil check before calling f.LoopbackFile.Flush. If f.LoopbackFile is nil (which can happen after Release sets it to nil), this will cause a panic. Consider adding a nil check at the beginning of the method.
| defer f.mu.Unlock() | |
| defer f.mu.Unlock() | |
| // If the underlying file has been released, there is nothing to flush. | |
| if f.LoopbackFile == nil { | |
| return 0 | |
| } |
| // Read implements FileReader | ||
| func (f *CachedFileHandle) Read(ctx context.Context, dest []byte, off int64) (fuse.ReadResult, syscall.Errno) { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() |
There was a problem hiding this comment.
The Read method calls f.PassthroughFd() without checking if f.LoopbackFile is nil. If f.LoopbackFile is nil (which can happen after Release sets it to nil), calling PassthroughFd() will cause a panic. Consider adding a nil check at the beginning of the method.
| defer f.mu.Unlock() | |
| defer f.mu.Unlock() | |
| if f.LoopbackFile == nil { | |
| return nil, syscall.EBADF | |
| } |
|
|
||
| // Any seek aborts caching | ||
| f.abortCaching() | ||
|
|
There was a problem hiding this comment.
The Lseek method lacks a nil check before calling f.LoopbackFile.Lseek. If f.LoopbackFile is nil (which can happen after Release sets it to nil), this will cause a panic. Consider adding a nil check at the beginning of the method.
| if f.LoopbackFile == nil { | |
| return 0, syscall.EBADF | |
| } |
| if errno != 0 { | ||
| return errno | ||
| } | ||
| fd, _ := f.PassthroughFd() |
There was a problem hiding this comment.
The Flush method calls f.PassthroughFd() without checking if f.LoopbackFile is nil. While there's a check for errno != 0 from the earlier Flush call, if f.LoopbackFile becomes nil between the Flush call and PassthroughFd call (unlikely but theoretically possible in concurrent scenarios), this could cause a panic. Consider adding a nil check before calling PassthroughFd.
|
|
||
| return fs.ToErrno(syscall.Close(fd)) | ||
| oldfile := f.LoopbackFile | ||
| f.LoopbackFile = nil |
There was a problem hiding this comment.
The Release method doesn't check if f.LoopbackFile is nil before calling oldfile.Release(ctx). If Release is called multiple times (which shouldn't happen but could in error scenarios), this will cause a panic. Consider adding a check like 'if oldfile == nil { return 0 }' before calling Release.
| f.LoopbackFile = nil | |
| f.LoopbackFile = nil | |
| if oldfile == nil { | |
| return 0 | |
| } |
No description provided.