From a744dee94c4d2d05354c0c88b15023d9a0889a8c Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Mon, 21 Dec 2020 13:01:34 +0100 Subject: [PATCH] lib/fs: Correct wrapping order for meaningful log-caller (#7209) --- lib/db/set.go | 2 +- lib/fs/casefs.go | 4 +-- lib/fs/filesystem.go | 17 ++++++++++++- lib/fs/mtimefs.go | 57 ++++++++++++++++++++---------------------- lib/fs/mtimefs_test.go | 16 +++++++----- 5 files changed, 56 insertions(+), 40 deletions(-) diff --git a/lib/db/set.go b/lib/db/set.go index ef412abec..713b7e61e 100644 --- a/lib/db/set.go +++ b/lib/db/set.go @@ -388,7 +388,7 @@ func (s *FileSet) SetIndexID(device protocol.DeviceID, id protocol.IndexID) { } } -func (s *FileSet) MtimeFS() *fs.MtimeFS { +func (s *FileSet) MtimeFS() fs.Filesystem { opStr := fmt.Sprintf("%s MtimeFS()", s.folder) l.Debugf(opStr) prefix, err := s.db.keyer.GenerateMtimesKey(nil, []byte(s.folder)) diff --git a/lib/fs/casefs.go b/lib/fs/casefs.go index ca5c59562..b755ac6ff 100644 --- a/lib/fs/casefs.go +++ b/lib/fs/casefs.go @@ -52,7 +52,7 @@ type caseFilesystemRegistry struct { startCleaner sync.Once } -func (r *caseFilesystemRegistry) get(fs Filesystem) *caseFilesystem { +func (r *caseFilesystemRegistry) get(fs Filesystem) Filesystem { r.mut.Lock() defer r.mut.Unlock() @@ -98,7 +98,7 @@ type caseFilesystem struct { // case-sensitive one. However it will add some overhead and thus shouldn't be // used if the filesystem is known to already behave case-sensitively. func NewCaseFilesystem(fs Filesystem) Filesystem { - return globalCaseFilesystemRegistry.get(fs) + return wrapFilesystem(fs, globalCaseFilesystemRegistry.get) } func (f *caseFilesystem) Chmod(name string, mode FileMode) error { diff --git a/lib/fs/filesystem.go b/lib/fs/filesystem.go index d03c73978..77caf3ac2 100644 --- a/lib/fs/filesystem.go +++ b/lib/fs/filesystem.go @@ -260,6 +260,21 @@ func Canonicalize(file string) (string, error) { return file, nil } +// wrapFilesystem should always be used when wrapping a Filesystem. +// It ensures proper wrapping order, which right now means: +// `logFilesystem` needs to be the outermost wrapper for caller lookup. +func wrapFilesystem(fs Filesystem, wrapFn func(Filesystem) Filesystem) Filesystem { + logFs, ok := fs.(*logFilesystem) + if ok { + fs = logFs.Filesystem + } + fs = wrapFn(fs) + if ok { + fs = &logFilesystem{fs} + } + return fs +} + // unwrapFilesystem removes "wrapping" filesystems to expose the underlying filesystem. func unwrapFilesystem(fs Filesystem) Filesystem { for { @@ -268,7 +283,7 @@ func unwrapFilesystem(fs Filesystem) Filesystem { fs = sfs.Filesystem case *walkFilesystem: fs = sfs.Filesystem - case *MtimeFS: + case *mtimeFS: fs = sfs.Filesystem default: return sfs diff --git a/lib/fs/mtimefs.go b/lib/fs/mtimefs.go index 12437a130..c41da08ed 100644 --- a/lib/fs/mtimefs.go +++ b/lib/fs/mtimefs.go @@ -17,41 +17,38 @@ type database interface { Delete(key string) error } -// The MtimeFS is a filesystem with nanosecond mtime precision, regardless -// of what shenanigans the underlying filesystem gets up to. A nil MtimeFS -// just does the underlying operations with no additions. -type MtimeFS struct { +type mtimeFS struct { Filesystem chtimes func(string, time.Time, time.Time) error db database caseInsensitive bool } -type MtimeFSOption func(*MtimeFS) +type MtimeFSOption func(*mtimeFS) func WithCaseInsensitivity(v bool) MtimeFSOption { - return func(f *MtimeFS) { + return func(f *mtimeFS) { f.caseInsensitive = v } } -func NewMtimeFS(underlying Filesystem, db database, options ...MtimeFSOption) *MtimeFS { - f := &MtimeFS{ - Filesystem: underlying, - chtimes: underlying.Chtimes, // for mocking it out in the tests - db: db, - } - for _, opt := range options { - opt(f) - } - return f +// NewMtimeFS returns a filesystem with nanosecond mtime precision, regardless +// of what shenanigans the underlying filesystem gets up to. +func NewMtimeFS(fs Filesystem, db database, options ...MtimeFSOption) Filesystem { + return wrapFilesystem(fs, func(underlying Filesystem) Filesystem { + f := &mtimeFS{ + Filesystem: underlying, + chtimes: underlying.Chtimes, // for mocking it out in the tests + db: db, + } + for _, opt := range options { + opt(f) + } + return f + }) } -func (f *MtimeFS) Chtimes(name string, atime, mtime time.Time) error { - if f == nil { - return f.chtimes(name, atime, mtime) - } - +func (f *mtimeFS) Chtimes(name string, atime, mtime time.Time) error { // Do a normal Chtimes call, don't care if it succeeds or not. f.chtimes(name, atime, mtime) @@ -66,7 +63,7 @@ func (f *MtimeFS) Chtimes(name string, atime, mtime time.Time) error { return nil } -func (f *MtimeFS) Stat(name string) (FileInfo, error) { +func (f *mtimeFS) Stat(name string) (FileInfo, error) { info, err := f.Filesystem.Stat(name) if err != nil { return nil, err @@ -86,7 +83,7 @@ func (f *MtimeFS) Stat(name string) (FileInfo, error) { return info, nil } -func (f *MtimeFS) Lstat(name string) (FileInfo, error) { +func (f *mtimeFS) Lstat(name string) (FileInfo, error) { info, err := f.Filesystem.Lstat(name) if err != nil { return nil, err @@ -106,7 +103,7 @@ func (f *MtimeFS) Lstat(name string) (FileInfo, error) { return info, nil } -func (f *MtimeFS) Walk(root string, walkFn WalkFunc) error { +func (f *mtimeFS) Walk(root string, walkFn WalkFunc) error { return f.Filesystem.Walk(root, func(path string, info FileInfo, err error) error { if info != nil { real, virtual, loadErr := f.load(path) @@ -125,7 +122,7 @@ func (f *MtimeFS) Walk(root string, walkFn WalkFunc) error { }) } -func (f *MtimeFS) Create(name string) (File, error) { +func (f *mtimeFS) Create(name string) (File, error) { fd, err := f.Filesystem.Create(name) if err != nil { return nil, err @@ -133,7 +130,7 @@ func (f *MtimeFS) Create(name string) (File, error) { return mtimeFile{fd, f}, nil } -func (f *MtimeFS) Open(name string) (File, error) { +func (f *mtimeFS) Open(name string) (File, error) { fd, err := f.Filesystem.Open(name) if err != nil { return nil, err @@ -141,7 +138,7 @@ func (f *MtimeFS) Open(name string) (File, error) { return mtimeFile{fd, f}, nil } -func (f *MtimeFS) OpenFile(name string, flags int, mode FileMode) (File, error) { +func (f *mtimeFS) OpenFile(name string, flags int, mode FileMode) (File, error) { fd, err := f.Filesystem.OpenFile(name, flags, mode) if err != nil { return nil, err @@ -152,7 +149,7 @@ func (f *MtimeFS) OpenFile(name string, flags int, mode FileMode) (File, error) // "real" is the on disk timestamp // "virtual" is what want the timestamp to be -func (f *MtimeFS) save(name string, real, virtual time.Time) { +func (f *mtimeFS) save(name string, real, virtual time.Time) { if f.caseInsensitive { name = UnicodeLowercase(name) } @@ -172,7 +169,7 @@ func (f *MtimeFS) save(name string, real, virtual time.Time) { f.db.PutBytes(name, bs) } -func (f *MtimeFS) load(name string) (real, virtual time.Time, err error) { +func (f *mtimeFS) load(name string) (real, virtual time.Time, err error) { if f.caseInsensitive { name = UnicodeLowercase(name) } @@ -205,7 +202,7 @@ func (m mtimeFileInfo) ModTime() time.Time { type mtimeFile struct { File - fs *MtimeFS + fs *mtimeFS } func (f mtimeFile) Stat() (FileInfo, error) { diff --git a/lib/fs/mtimefs_test.go b/lib/fs/mtimefs_test.go index e218fedff..3f7dc6c07 100644 --- a/lib/fs/mtimefs_test.go +++ b/lib/fs/mtimefs_test.go @@ -27,7 +27,7 @@ func TestMtimeFS(t *testing.T) { // a random time with nanosecond precision testTime := time.Unix(1234567890, 123456789) - mtimefs := NewMtimeFS(newBasicFilesystem("."), make(mapStore)) + mtimefs := newMtimeFS(newBasicFilesystem("."), make(mapStore)) // Do one Chtimes call that will go through to the normal filesystem mtimefs.chtimes = os.Chtimes @@ -90,7 +90,7 @@ func TestMtimeFSWalk(t *testing.T) { defer func() { _ = os.RemoveAll(dir) }() underlying := NewFilesystem(FilesystemTypeBasic, dir) - mtimefs := NewMtimeFS(underlying, make(mapStore)) + mtimefs := newMtimeFS(underlying, make(mapStore)) mtimefs.chtimes = failChtimes if err := ioutil.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0644); err != nil { @@ -144,7 +144,7 @@ func TestMtimeFSOpen(t *testing.T) { defer func() { _ = os.RemoveAll(dir) }() underlying := NewFilesystem(FilesystemTypeBasic, dir) - mtimefs := NewMtimeFS(underlying, make(mapStore)) + mtimefs := newMtimeFS(underlying, make(mapStore)) mtimefs.chtimes = failChtimes if err := ioutil.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0644); err != nil { @@ -196,7 +196,7 @@ func TestMtimeFSInsensitive(t *testing.T) { t.Skip("need case insensitive FS") } - theTest := func(t *testing.T, fs *MtimeFS, shouldSucceed bool) { + theTest := func(t *testing.T, fs *mtimeFS, shouldSucceed bool) { os.RemoveAll("testdata") defer os.RemoveAll("testdata") os.Mkdir("testdata", 0755) @@ -223,12 +223,12 @@ func TestMtimeFSInsensitive(t *testing.T) { // The test should fail with a case sensitive mtimefs t.Run("with case sensitive mtimefs", func(t *testing.T) { - theTest(t, NewMtimeFS(newBasicFilesystem("."), make(mapStore)), false) + theTest(t, newMtimeFS(newBasicFilesystem("."), make(mapStore)), false) }) // And succeed with a case insensitive one. t.Run("with case insensitive mtimefs", func(t *testing.T) { - theTest(t, NewMtimeFS(newBasicFilesystem("."), make(mapStore), WithCaseInsensitivity(true)), true) + theTest(t, newMtimeFS(newBasicFilesystem("."), make(mapStore), WithCaseInsensitivity(true)), true) }) } @@ -261,3 +261,7 @@ func failChtimes(name string, mtime, atime time.Time) error { func evilChtimes(name string, mtime, atime time.Time) error { return os.Chtimes(name, mtime.Add(300*time.Hour).Truncate(time.Hour), atime.Add(300*time.Hour).Truncate(time.Hour)) } + +func newMtimeFS(fs Filesystem, db database, options ...MtimeFSOption) *mtimeFS { + return NewMtimeFS(fs, db, options...).(*mtimeFS) +}