diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index e88c15bd9..ae3edaf44 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -66,6 +66,11 @@ func (s *ItemStats) Add(other ItemStats) { s.TreeSizeInRepo += other.TreeSizeInRepo } +// ToNoder returns a restic.Node for a File. +type ToNoder interface { + ToNode(ignoreXattrListError bool) (*restic.Node, error) +} + type archiverRepo interface { restic.Loader restic.BlobSaver @@ -257,8 +262,8 @@ func (arch *Archiver) trackItem(item string, previous, current *restic.Node, s I } // nodeFromFileInfo returns the restic node from an os.FileInfo. -func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - node, err := arch.FS.NodeFromFileInfo(filename, fi, ignoreXattrListError) +func (arch *Archiver) nodeFromFileInfo(snPath, filename string, meta ToNoder, ignoreXattrListError bool) (*restic.Node, error) { + node, err := meta.ToNode(ignoreXattrListError) if !arch.WithAtime { node.AccessTime = node.ModTime } @@ -308,20 +313,14 @@ func (arch *Archiver) wrapLoadTreeError(id restic.ID, err error) error { // saveDir stores a directory in the repo and returns the node. snPath is the // path within the current snapshot. -func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, fi os.FileInfo, previous *restic.Tree, complete fileCompleteFunc) (d futureNode, err error) { +func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, meta fs.File, previous *restic.Tree, complete fileCompleteFunc) (d futureNode, err error) { debug.Log("%v %v", snPath, dir) - treeNode, err := arch.nodeFromFileInfo(snPath, dir, fi, false) + treeNode, names, err := arch.dirToNodeAndEntries(snPath, dir, meta) if err != nil { return futureNode{}, err } - names, err := fs.Readdirnames(arch.FS, dir, fs.O_NOFOLLOW) - if err != nil { - return futureNode{}, err - } - sort.Strings(names) - nodes := make([]futureNode, 0, len(names)) for _, name := range names { @@ -359,6 +358,29 @@ func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, fi return fn, nil } +func (arch *Archiver) dirToNodeAndEntries(snPath, dir string, meta fs.File) (node *restic.Node, names []string, err error) { + err = meta.MakeReadable() + if err != nil { + return nil, nil, fmt.Errorf("openfile for readdirnames failed: %w", err) + } + + node, err = arch.nodeFromFileInfo(snPath, dir, meta, false) + if err != nil { + return nil, nil, err + } + if node.Type != restic.NodeTypeDir { + return nil, nil, fmt.Errorf("directory %v changed type, refusing to archive", snPath) + } + + names, err = meta.Readdirnames(-1) + if err != nil { + return nil, nil, fmt.Errorf("readdirnames %v failed: %w", dir, err) + } + sort.Strings(names) + + return node, names, nil +} + // futureNode holds a reference to a channel that returns a FutureNodeResult // or a reference to an already existing result. If the result is available // immediately, then storing a reference directly requires less memory than @@ -448,8 +470,23 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous return futureNode{}, true, nil } + meta, err := arch.FS.OpenFile(target, fs.O_NOFOLLOW, true) + if err != nil { + debug.Log("open metadata for %v returned error: %v", target, err) + return filterError(err) + } + closeFile := true + defer func() { + if closeFile { + cerr := meta.Close() + if err == nil { + err = cerr + } + } + }() + // get file info and run remaining select functions that require file information - fi, err := arch.FS.Lstat(target) + fi, err := meta.Stat() if err != nil { debug.Log("lstat() for %v returned error: %v", target, err) return filterError(err) @@ -470,7 +507,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous debug.Log("%v hasn't changed, using old list of blobs", target) arch.trackItem(snPath, previous, previous, ItemStats{}, time.Since(start)) arch.CompleteBlob(previous.Size) - node, err := arch.nodeFromFileInfo(snPath, target, fi, false) + node, err := arch.nodeFromFileInfo(snPath, target, meta, false) if err != nil { return futureNode{}, false, err } @@ -497,28 +534,28 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous // reopen file and do an fstat() on the open file to check it is still // a file (and has not been exchanged for e.g. a symlink) - file, err := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW) + err := meta.MakeReadable() if err != nil { - debug.Log("Openfile() for %v returned error: %v", target, err) + debug.Log("MakeReadable() for %v returned error: %v", target, err) return filterError(err) } - fi, err = file.Stat() + fi, err := meta.Stat() if err != nil { debug.Log("stat() on opened file %v returned error: %v", target, err) - _ = file.Close() return filterError(err) } // make sure it's still a file if !fi.Mode().IsRegular() { err = errors.Errorf("file %v changed type, refusing to archive", target) - _ = file.Close() return filterError(err) } + closeFile = false + // Save will close the file, we don't need to do that - fn = arch.fileSaver.Save(ctx, snPath, target, file, fi, func() { + fn = arch.fileSaver.Save(ctx, snPath, target, meta, func() { arch.StartFile(snPath) }, func() { arch.trackItem(snPath, nil, nil, ItemStats{}, 0) @@ -538,7 +575,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous return futureNode{}, false, err } - fn, err = arch.saveDir(ctx, snPath, target, fi, oldSubtree, + fn, err = arch.saveDir(ctx, snPath, target, meta, oldSubtree, func(node *restic.Node, stats ItemStats) { arch.trackItem(snItem, previous, node, stats, time.Since(start)) }) @@ -554,7 +591,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous default: debug.Log(" %v other", target) - node, err := arch.nodeFromFileInfo(snPath, target, fi, false) + node, err := arch.nodeFromFileInfo(snPath, target, meta, false) if err != nil { return futureNode{}, false, err } @@ -688,7 +725,7 @@ func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *tree, } func (arch *Archiver) dirPathToNode(snPath, target string) (node *restic.Node, err error) { - meta, err := arch.FS.OpenFile(target, fs.O_RDONLY) + meta, err := arch.FS.OpenFile(target, 0, true) if err != nil { return nil, err } @@ -700,14 +737,9 @@ func (arch *Archiver) dirPathToNode(snPath, target string) (node *restic.Node, e }() debug.Log("%v, reading dir node data from %v", snPath, target) - fi, err := meta.Stat() - if err != nil { - return nil, errors.WithStack(err) - } - // in some cases reading xattrs for directories above the backup source is not allowed // thus ignore errors for such folders. - node, err = arch.nodeFromFileInfo(snPath, target, fi, true) + node, err = arch.nodeFromFileInfo(snPath, target, meta, true) if err != nil { return nil, err } diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index ff072fa36..0b2957bc6 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -76,17 +76,12 @@ func saveFile(t testing.TB, repo archiverRepo, filename string, filesystem fs.FS startCallback = true } - file, err := arch.FS.OpenFile(filename, fs.O_RDONLY|fs.O_NOFOLLOW) + file, err := arch.FS.OpenFile(filename, fs.O_NOFOLLOW, false) if err != nil { t.Fatal(err) } - fi, err := file.Stat() - if err != nil { - t.Fatal(err) - } - - res := arch.fileSaver.Save(ctx, "/", filename, file, fi, start, completeReading, complete) + res := arch.fileSaver.Save(ctx, "/", filename, file, start, completeReading, complete) fnr := res.take(ctx) if fnr.err != nil { @@ -556,11 +551,12 @@ func rename(t testing.TB, oldname, newname string) { } } -func nodeFromFI(t testing.TB, fs fs.FS, filename string, fi os.FileInfo) *restic.Node { - node, err := fs.NodeFromFileInfo(filename, fi, false) - if err != nil { - t.Fatal(err) - } +func nodeFromFile(t testing.TB, localFs fs.FS, filename string) *restic.Node { + meta, err := localFs.OpenFile(filename, fs.O_NOFOLLOW, true) + rtest.OK(t, err) + node, err := meta.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) return node } @@ -688,7 +684,7 @@ func TestFileChanged(t *testing.T) { fs := &fs.Local{} fiBefore := lstat(t, filename) - node := nodeFromFI(t, fs, filename, fiBefore) + node := nodeFromFile(t, fs, filename) if fileChanged(fs, fiBefore, node, 0) { t.Fatalf("unchanged file detected as changed") @@ -729,8 +725,8 @@ func TestFilChangedSpecialCases(t *testing.T) { t.Run("type-change", func(t *testing.T) { fi := lstat(t, filename) - node := nodeFromFI(t, &fs.Local{}, filename, fi) - node.Type = "restic.NodeTypeSymlink" + node := nodeFromFile(t, &fs.Local{}, filename) + node.Type = restic.NodeTypeSymlink if !fileChanged(&fs.Local{}, fi, node, 0) { t.Fatal("node with changed type detected as unchanged") } @@ -834,7 +830,8 @@ func TestArchiverSaveDir(t *testing.T) { wg, ctx := errgroup.WithContext(context.Background()) repo.StartPackUploader(ctx, wg) - arch := New(repo, fs.Track{FS: fs.Local{}}, Options{}) + testFS := fs.Track{FS: fs.Local{}} + arch := New(repo, testFS, Options{}) arch.runWorkers(ctx, wg) arch.summary = &Summary{} @@ -846,15 +843,11 @@ func TestArchiverSaveDir(t *testing.T) { back := rtest.Chdir(t, chdir) defer back() - fi, err := os.Lstat(test.target) - if err != nil { - t.Fatal(err) - } - - ft, err := arch.saveDir(ctx, "/", test.target, fi, nil, nil) - if err != nil { - t.Fatal(err) - } + meta, err := testFS.OpenFile(test.target, fs.O_NOFOLLOW, true) + rtest.OK(t, err) + ft, err := arch.saveDir(ctx, "/", test.target, meta, nil, nil) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) fnr := ft.take(ctx) node, stats := fnr.node, fnr.stats @@ -916,19 +909,16 @@ func TestArchiverSaveDirIncremental(t *testing.T) { wg, ctx := errgroup.WithContext(context.TODO()) repo.StartPackUploader(ctx, wg) - arch := New(repo, fs.Track{FS: fs.Local{}}, Options{}) + testFS := fs.Track{FS: fs.Local{}} + arch := New(repo, testFS, Options{}) arch.runWorkers(ctx, wg) arch.summary = &Summary{} - fi, err := os.Lstat(tempdir) - if err != nil { - t.Fatal(err) - } - - ft, err := arch.saveDir(ctx, "/", tempdir, fi, nil, nil) - if err != nil { - t.Fatal(err) - } + meta, err := testFS.OpenFile(tempdir, fs.O_NOFOLLOW, true) + rtest.OK(t, err) + ft, err := arch.saveDir(ctx, "/", tempdir, meta, nil, nil) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) fnr := ft.take(ctx) node, stats := fnr.node, fnr.stats @@ -1665,8 +1655,8 @@ type MockFS struct { bytesRead map[string]int // tracks bytes read from all opened files } -func (m *MockFS) OpenFile(name string, flag int) (fs.File, error) { - f, err := m.FS.OpenFile(name, flag) +func (m *MockFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { + f, err := m.FS.OpenFile(name, flag, metadataOnly) if err != nil { return f, err } @@ -2056,12 +2046,12 @@ type TrackFS struct { m sync.Mutex } -func (m *TrackFS) OpenFile(name string, flag int) (fs.File, error) { +func (m *TrackFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { m.m.Lock() m.opened[name]++ m.m.Unlock() - return m.FS.OpenFile(name, flag) + return m.FS.OpenFile(name, flag, metadataOnly) } type failSaveRepo struct { @@ -2210,48 +2200,39 @@ func snapshot(t testing.TB, repo archiverRepo, fs fs.FS, parent *restic.Snapshot return snapshot, node } -// StatFS allows overwriting what is returned by the Lstat function. -type StatFS struct { +type overrideFS struct { fs.FS - - OverrideLstat map[string]os.FileInfo - OnlyOverrideStat bool + overrideFI os.FileInfo + overrideNode *restic.Node + overrideErr error } -func (fs *StatFS) Lstat(name string) (os.FileInfo, error) { - if !fs.OnlyOverrideStat { - if fi, ok := fs.OverrideLstat[fixpath(name)]; ok { - return fi, nil - } +func (m *overrideFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { + f, err := m.FS.OpenFile(name, flag, metadataOnly) + if err != nil { + return f, err } - return fs.FS.Lstat(name) -} - -func (fs *StatFS) OpenFile(name string, flags int) (fs.File, error) { - if fi, ok := fs.OverrideLstat[fixpath(name)]; ok { - f, err := fs.FS.OpenFile(name, flags) - if err != nil { - return nil, err - } - - wrappedFile := fileStat{ - File: f, - fi: fi, - } - return wrappedFile, nil + if filepath.Base(name) == "testfile" { + return &overrideFile{f, m}, nil } - - return fs.FS.OpenFile(name, flags) + return f, nil } -type fileStat struct { +type overrideFile struct { fs.File - fi os.FileInfo + ofs *overrideFS } -func (f fileStat) Stat() (os.FileInfo, error) { - return f.fi, nil +func (f overrideFile) Stat() (os.FileInfo, error) { + return f.ofs.overrideFI, nil +} + +func (f overrideFile) ToNode(ignoreXattrListError bool) (*restic.Node, error) { + if f.ofs.overrideNode == nil { + return f.File.ToNode(ignoreXattrListError) + } + return f.ofs.overrideNode, f.ofs.overrideErr } // used by wrapFileInfo, use untyped const in order to avoid having a version @@ -2279,17 +2260,18 @@ func TestMetadataChanged(t *testing.T) { // get metadata fi := lstat(t, "testfile") localFS := &fs.Local{} - want, err := localFS.NodeFromFileInfo("testfile", fi, false) - if err != nil { - t.Fatal(err) - } + meta, err := localFS.OpenFile("testfile", fs.O_NOFOLLOW, true) + rtest.OK(t, err) + want, err := meta.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) - fs := &StatFS{ - FS: localFS, - OverrideLstat: map[string]os.FileInfo{ - "testfile": fi, - }, + fs := &overrideFS{ + FS: localFS, + overrideFI: fi, + overrideNode: &restic.Node{}, } + *fs.overrideNode = *want sn, node2 := snapshot(t, repo, fs, nil, "testfile") @@ -2309,7 +2291,8 @@ func TestMetadataChanged(t *testing.T) { } // modify the mode by wrapping it in a new struct, uses the consts defined above - fs.OverrideLstat["testfile"] = wrapFileInfo(fi) + fs.overrideFI = wrapFileInfo(fi) + rtest.Assert(t, !fileChanged(fs, fs.overrideFI, node2, 0), "testfile must not be considered as changed") // set the override values in the 'want' node which want.Mode = 0400 @@ -2318,16 +2301,13 @@ func TestMetadataChanged(t *testing.T) { want.UID = 51234 want.GID = 51235 } - // no user and group name - want.User = "" - want.Group = "" + // update mock node accordingly + fs.overrideNode.Mode = 0400 + fs.overrideNode.UID = want.UID + fs.overrideNode.GID = want.GID // make another snapshot _, node3 := snapshot(t, repo, fs, sn, "testfile") - // Override username and group to empty string - in case underlying system has user with UID 51234 - // See https://github.com/restic/restic/issues/2372 - node3.User = "" - node3.Group = "" // make sure that metadata was recorded successfully if !cmp.Equal(want, node3) { @@ -2342,7 +2322,7 @@ func TestMetadataChanged(t *testing.T) { func TestRacyFileSwap(t *testing.T) { files := TestDir{ - "file": TestFile{ + "testfile": TestFile{ Content: "foo bar test file", }, } @@ -2354,14 +2334,11 @@ func TestRacyFileSwap(t *testing.T) { // get metadata of current folder fi := lstat(t, ".") - tempfile := filepath.Join(tempdir, "file") + tempfile := filepath.Join(tempdir, "testfile") - statfs := &StatFS{ - FS: fs.Local{}, - OverrideLstat: map[string]os.FileInfo{ - tempfile: fi, - }, - OnlyOverrideStat: true, + statfs := &overrideFS{ + FS: fs.Local{}, + overrideFI: fi, } ctx, cancel := context.WithCancel(context.Background()) @@ -2388,14 +2365,19 @@ func TestRacyFileSwap(t *testing.T) { } } +type mockToNoder struct { + node *restic.Node + err error +} + +func (m *mockToNoder) ToNode(_ bool) (*restic.Node, error) { + return m.node, m.err +} + func TestMetadataBackupErrorFiltering(t *testing.T) { tempdir := t.TempDir() - repo := repository.TestRepository(t) - filename := filepath.Join(tempdir, "file") - rtest.OK(t, os.WriteFile(filename, []byte("example"), 0o600)) - fi, err := os.Stat(filename) - rtest.OK(t, err) + repo := repository.TestRepository(t) arch := New(repo, fs.Local{}, Options{}) @@ -2406,15 +2388,24 @@ func TestMetadataBackupErrorFiltering(t *testing.T) { return replacementErr } + nonExistNoder := &mockToNoder{ + node: &restic.Node{Type: restic.NodeTypeFile}, + err: fmt.Errorf("not found"), + } + // check that errors from reading extended metadata are properly filtered - node, err := arch.nodeFromFileInfo("file", filename+"invalid", fi, false) + node, err := arch.nodeFromFileInfo("file", filename+"invalid", nonExistNoder, false) rtest.Assert(t, node != nil, "node is missing") rtest.Assert(t, err == replacementErr, "expected %v got %v", replacementErr, err) rtest.Assert(t, filteredErr != nil, "missing inner error") // check that errors from reading irregular file are not filtered filteredErr = nil - node, err = arch.nodeFromFileInfo("file", filename, wrapIrregularFileInfo(fi), false) + nonExistNoder = &mockToNoder{ + node: &restic.Node{Type: restic.NodeTypeIrregular}, + err: fmt.Errorf(`unsupported file type "irregular"`), + } + node, err = arch.nodeFromFileInfo("file", filename, nonExistNoder, false) rtest.Assert(t, node != nil, "node is missing") rtest.Assert(t, filteredErr == nil, "error for irregular node should not have been filtered") rtest.Assert(t, strings.Contains(err.Error(), "irregular"), "unexpected error %q does not warn about irregular file mode", err) @@ -2434,17 +2425,19 @@ func TestIrregularFile(t *testing.T) { tempfile := filepath.Join(tempdir, "testfile") fi := lstat(t, "testfile") - statfs := &StatFS{ - FS: fs.Local{}, - OverrideLstat: map[string]os.FileInfo{ - tempfile: wrapIrregularFileInfo(fi), + override := &overrideFS{ + FS: fs.Local{}, + overrideFI: wrapIrregularFileInfo(fi), + overrideNode: &restic.Node{ + Type: restic.NodeTypeIrregular, }, + overrideErr: fmt.Errorf(`unsupported file type "irregular"`), } ctx, cancel := context.WithCancel(context.Background()) defer cancel() - arch := New(repo, fs.Track{FS: statfs}, Options{}) + arch := New(repo, fs.Track{FS: override}, Options{}) _, excluded, err := arch.save(ctx, "/", tempfile, nil) if err == nil { t.Fatalf("Save() should have failed") diff --git a/internal/archiver/archiver_unix_test.go b/internal/archiver/archiver_unix_test.go index 621f84826..deeab6459 100644 --- a/internal/archiver/archiver_unix_test.go +++ b/internal/archiver/archiver_unix_test.go @@ -57,12 +57,8 @@ func wrapIrregularFileInfo(fi os.FileInfo) os.FileInfo { } func statAndSnapshot(t *testing.T, repo archiverRepo, name string) (*restic.Node, *restic.Node) { - fi := lstat(t, name) - fs := &fs.Local{} - want, err := fs.NodeFromFileInfo(name, fi, false) - rtest.OK(t, err) - - _, node := snapshot(t, repo, fs, nil, name) + want := nodeFromFile(t, &fs.Local{}, name) + _, node := snapshot(t, repo, &fs.Local{}, nil, name) return want, node } diff --git a/internal/archiver/exclude.go b/internal/archiver/exclude.go index 54ced788a..418517fd9 100644 --- a/internal/archiver/exclude.go +++ b/internal/archiver/exclude.go @@ -135,9 +135,9 @@ func isExcludedByFile(filename, tagFilename, header string, rc *rejectionCache, return rejected } -func isDirExcludedByFile(dir, tagFilename, header string, fs fs.FS, warnf func(msg string, args ...interface{})) bool { - tf := fs.Join(dir, tagFilename) - _, err := fs.Lstat(tf) +func isDirExcludedByFile(dir, tagFilename, header string, fsInst fs.FS, warnf func(msg string, args ...interface{})) bool { + tf := fsInst.Join(dir, tagFilename) + _, err := fsInst.Lstat(tf) if errors.Is(err, os.ErrNotExist) { return false } @@ -153,7 +153,7 @@ func isDirExcludedByFile(dir, tagFilename, header string, fs fs.FS, warnf func(m // From this stage, errors mean tagFilename exists but it is malformed. // Warnings will be generated so that the user is informed that the // indented ignore-action is not performed. - f, err := fs.OpenFile(tf, os.O_RDONLY) + f, err := fsInst.OpenFile(tf, fs.O_RDONLY, false) if err != nil { warnf("could not open exclusion tagfile: %v", err) return false diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go index dccaa9442..ca8ec2fbb 100644 --- a/internal/archiver/file_saver.go +++ b/internal/archiver/file_saver.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "os" "sync" "github.com/restic/chunker" @@ -29,7 +28,7 @@ type fileSaver struct { CompleteBlob func(bytes uint64) - NodeFromFileInfo func(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) + NodeFromFileInfo func(snPath, filename string, meta ToNoder, ignoreXattrListError bool) (*restic.Node, error) } // newFileSaver returns a new file saver. A worker pool with fileWorkers is @@ -71,13 +70,12 @@ type fileCompleteFunc func(*restic.Node, ItemStats) // file is closed by Save. completeReading is only called if the file was read // successfully. complete is always called. If completeReading is called, then // this will always happen before calling complete. -func (s *fileSaver) Save(ctx context.Context, snPath string, target string, file fs.File, fi os.FileInfo, start func(), completeReading func(), complete fileCompleteFunc) futureNode { +func (s *fileSaver) Save(ctx context.Context, snPath string, target string, file fs.File, start func(), completeReading func(), complete fileCompleteFunc) futureNode { fn, ch := newFutureNode() job := saveFileJob{ snPath: snPath, target: target, file: file, - fi: fi, ch: ch, start: start, @@ -100,7 +98,6 @@ type saveFileJob struct { snPath string target string file fs.File - fi os.FileInfo ch chan<- futureNodeResult start func() @@ -109,7 +106,7 @@ type saveFileJob struct { } // saveFile stores the file f in the repo, then closes it. -func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPath string, target string, f fs.File, fi os.FileInfo, start func(), finishReading func(), finish func(res futureNodeResult)) { +func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPath string, target string, f fs.File, start func(), finishReading func(), finish func(res futureNodeResult)) { start() fnr := futureNodeResult{ @@ -156,7 +153,7 @@ func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat debug.Log("%v", snPath) - node, err := s.NodeFromFileInfo(snPath, target, fi, false) + node, err := s.NodeFromFileInfo(snPath, target, f, false) if err != nil { _ = f.Close() completeError(err) @@ -262,7 +259,7 @@ func (s *fileSaver) worker(ctx context.Context, jobs <-chan saveFileJob) { } } - s.saveFile(ctx, chnker, job.snPath, job.target, job.file, job.fi, job.start, func() { + s.saveFile(ctx, chnker, job.snPath, job.target, job.file, job.start, func() { if job.completeReading != nil { job.completeReading() } diff --git a/internal/archiver/file_saver_test.go b/internal/archiver/file_saver_test.go index 069cdc171..ce862f6fe 100644 --- a/internal/archiver/file_saver_test.go +++ b/internal/archiver/file_saver_test.go @@ -30,7 +30,7 @@ func createTestFiles(t testing.TB, num int) (files []string) { return files } -func startFileSaver(ctx context.Context, t testing.TB, fs fs.FS) (*fileSaver, context.Context, *errgroup.Group) { +func startFileSaver(ctx context.Context, t testing.TB, fsInst fs.FS) (*fileSaver, context.Context, *errgroup.Group) { wg, ctx := errgroup.WithContext(ctx) saveBlob := func(ctx context.Context, tpe restic.BlobType, buf *buffer, _ string, cb func(saveBlobResponse)) { @@ -49,8 +49,8 @@ func startFileSaver(ctx context.Context, t testing.TB, fs fs.FS) (*fileSaver, co } s := newFileSaver(ctx, wg, saveBlob, pol, workers, workers) - s.NodeFromFileInfo = func(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - return fs.NodeFromFileInfo(filename, fi, ignoreXattrListError) + s.NodeFromFileInfo = func(snPath, filename string, meta ToNoder, ignoreXattrListError bool) (*restic.Node, error) { + return meta.ToNode(ignoreXattrListError) } return s, ctx, wg @@ -72,17 +72,12 @@ func TestFileSaver(t *testing.T) { var results []futureNode for _, filename := range files { - f, err := testFs.OpenFile(filename, os.O_RDONLY) + f, err := testFs.OpenFile(filename, os.O_RDONLY, false) if err != nil { t.Fatal(err) } - fi, err := f.Stat() - if err != nil { - t.Fatal(err) - } - - ff := s.Save(ctx, filename, filename, f, fi, startFn, completeReadingFn, completeFn) + ff := s.Save(ctx, filename, filename, f, startFn, completeReadingFn, completeFn) results = append(results, ff) } diff --git a/internal/fs/file.go b/internal/fs/file.go index fa395b628..81ee4bc7a 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -67,7 +67,7 @@ func ResetPermissions(path string) error { // Readdirnames returns a list of file in a directory. Flags are passed to fs.OpenFile. // O_RDONLY and O_DIRECTORY are implied. func Readdirnames(filesystem FS, dir string, flags int) ([]string, error) { - f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags) + f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags, false) if err != nil { return nil, fmt.Errorf("openfile for readdirnames failed: %w", err) } diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 4f8b67090..5e6c72d0a 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -20,18 +20,16 @@ func (fs Local) VolumeName(path string) string { return filepath.VolumeName(path) } -// OpenFile is the generalized open call; most users will use Open -// or Create instead. It opens the named file with specified flag -// (O_RDONLY etc.) and perm, (0666 etc.) if applicable. If successful, -// methods on the returned File can be used for I/O. -// If there is an error, it will be of type *PathError. -func (fs Local) OpenFile(name string, flag int) (File, error) { - f, err := os.OpenFile(fixpath(name), flag, 0) - if err != nil { - return nil, err - } - _ = setFlags(f) - return f, nil +// OpenFile opens a file or directory for reading. +// +// If metadataOnly is set, an implementation MUST return a File object for +// arbitrary file types including symlinks. The implementation may internally use +// the given file path or a file handle. In particular, an implementation may +// delay actually accessing the underlying filesystem. +// +// Only the O_NOFOLLOW and O_DIRECTORY flags are supported. +func (fs Local) OpenFile(name string, flag int, metadataOnly bool) (File, error) { + return newLocalFile(name, flag, metadataOnly) } // Lstat returns the FileInfo structure describing the named file. @@ -53,10 +51,6 @@ func (fs Local) ExtendedStat(fi os.FileInfo) ExtendedFileInfo { return ExtendedStat(fi) } -func (fs Local) NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - return nodeFromFileInfo(path, fi, ignoreXattrListError) -} - // Join joins any number of path elements into a single path, adding a // Separator if necessary. Join calls Clean on the result; in particular, all // empty strings are ignored. On Windows, the result is a UNC path if and only @@ -97,3 +91,87 @@ func (fs Local) Base(path string) string { func (fs Local) Dir(path string) string { return filepath.Dir(path) } + +type localFile struct { + name string + flag int + f *os.File + fi os.FileInfo +} + +// See the File interface for a description of each method +var _ File = &localFile{} + +func newLocalFile(name string, flag int, metadataOnly bool) (*localFile, error) { + var f *os.File + if !metadataOnly { + var err error + f, err = os.OpenFile(fixpath(name), flag, 0) + if err != nil { + return nil, err + } + _ = setFlags(f) + } + return &localFile{ + name: name, + flag: flag, + f: f, + }, nil +} + +func (f *localFile) MakeReadable() error { + if f.f != nil { + panic("file is already readable") + } + + newF, err := newLocalFile(f.name, f.flag, false) + if err != nil { + return err + } + // replace state and also reset cached FileInfo + *f = *newF + return nil +} + +func (f *localFile) cacheFI() error { + if f.fi != nil { + return nil + } + var err error + if f.f != nil { + f.fi, err = f.f.Stat() + } else if f.flag&O_NOFOLLOW != 0 { + f.fi, err = os.Lstat(f.name) + } else { + f.fi, err = os.Stat(f.name) + } + return err +} + +func (f *localFile) Stat() (os.FileInfo, error) { + err := f.cacheFI() + // the call to cacheFI MUST happen before reading from f.fi + return f.fi, err +} + +func (f *localFile) ToNode(ignoreXattrListError bool) (*restic.Node, error) { + if err := f.cacheFI(); err != nil { + return nil, err + } + return nodeFromFileInfo(f.name, f.fi, ignoreXattrListError) +} + +func (f *localFile) Read(p []byte) (n int, err error) { + return f.f.Read(p) +} + +func (f *localFile) Readdirnames(n int) ([]string, error) { + return f.f.Readdirnames(n) +} + +func (f *localFile) Close() error { + if f.f != nil { + return f.f.Close() + } + return nil +} diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index e9bc66657..fe82b85e1 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -10,7 +10,6 @@ import ( "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/options" - "github.com/restic/restic/internal/restic" ) // VSSConfig holds extended options of windows volume shadow copy service. @@ -126,9 +125,9 @@ func (fs *LocalVss) DeleteSnapshots() { fs.snapshots = activeSnapshots } -// OpenFile wraps the Open method of the underlying file system. -func (fs *LocalVss) OpenFile(name string, flag int) (File, error) { - return fs.FS.OpenFile(fs.snapshotPath(name), flag) +// OpenFile wraps the OpenFile method of the underlying file system. +func (fs *LocalVss) OpenFile(name string, flag int, metadataOnly bool) (File, error) { + return fs.FS.OpenFile(fs.snapshotPath(name), flag, metadataOnly) } // Lstat wraps the Lstat method of the underlying file system. @@ -136,10 +135,6 @@ func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) { return fs.FS.Lstat(fs.snapshotPath(name)) } -func (fs *LocalVss) NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - return fs.FS.NodeFromFileInfo(fs.snapshotPath(path), fi, ignoreXattrListError) -} - // isMountPointIncluded is true if given mountpoint included by user. func (fs *LocalVss) isMountPointIncluded(mountPoint string) bool { if fs.excludeVolumes == nil { diff --git a/internal/fs/fs_local_vss_test.go b/internal/fs/fs_local_vss_test.go index db8d4b133..33c412fe9 100644 --- a/internal/fs/fs_local_vss_test.go +++ b/internal/fs/fs_local_vss_test.go @@ -327,7 +327,7 @@ func TestVSSFS(t *testing.T) { rtest.OK(t, err) rtest.Equals(t, origFi.Mode(), lstatFi.Mode()) - f, err := localVss.OpenFile(tempfile, os.O_RDONLY) + f, err := localVss.OpenFile(tempfile, os.O_RDONLY, false) rtest.OK(t, err) data, err := io.ReadAll(f) rtest.OK(t, err) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index a4efa8dea..8728b274c 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -49,12 +49,7 @@ func (fs *Reader) fi() os.FileInfo { } } -// OpenFile is the generalized open call; most users will use Open -// or Create instead. It opens the named file with specified flag -// (O_RDONLY etc.) and perm, (0666 etc.) if applicable. If successful, -// methods on the returned File can be used for I/O. -// If there is an error, it will be of type *os.PathError. -func (fs *Reader) OpenFile(name string, flag int) (f File, err error) { +func (fs *Reader) OpenFile(name string, flag int, _ bool) (f File, err error) { if flag & ^(O_RDONLY|O_NOFOLLOW) != 0 { return nil, pathError("open", name, fmt.Errorf("invalid combination of flags 0x%x", flag)) @@ -127,17 +122,6 @@ func (fs *Reader) ExtendedStat(fi os.FileInfo) ExtendedFileInfo { } } -func (fs *Reader) NodeFromFileInfo(path string, fi os.FileInfo, _ bool) (*restic.Node, error) { - node := buildBasicNode(path, fi) - - // fill minimal info with current values for uid, gid - node.UID = uint32(os.Getuid()) - node.GID = uint32(os.Getgid()) - node.ChangeTime = node.ModTime - - return node, nil -} - // Join joins any number of path elements into a single path, adding a // Separator if necessary. Join calls Clean on the result; in particular, all // empty strings are ignored. On Windows, the result is a UNC path if and only @@ -235,6 +219,10 @@ type fakeFile struct { // ensure that fakeFile implements File var _ File = fakeFile{} +func (f fakeFile) MakeReadable() error { + return nil +} + func (f fakeFile) Readdirnames(_ int) ([]string, error) { return nil, pathError("readdirnames", f.name, os.ErrInvalid) } @@ -251,6 +239,17 @@ func (f fakeFile) Stat() (os.FileInfo, error) { return f.FileInfo, nil } +func (f fakeFile) ToNode(_ bool) (*restic.Node, error) { + node := buildBasicNode(f.name, f.FileInfo) + + // fill minimal info with current values for uid, gid + node.UID = uint32(os.Getuid()) + node.GID = uint32(os.Getgid()) + node.ChangeTime = node.ModTime + + return node, nil +} + // fakeDir implements Readdirnames and Readdir, everything else is delegated to fakeFile. type fakeDir struct { entries []os.FileInfo diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index e7020bc9d..7e7f6e77c 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -16,7 +16,7 @@ import ( ) func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte) { - f, err := fs.OpenFile(filename, O_RDONLY) + f, err := fs.OpenFile(filename, O_RDONLY, false) if err != nil { t.Fatal(err) } @@ -37,7 +37,7 @@ func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte } func verifyDirectoryContents(t testing.TB, fs FS, dir string, want []string) { - f, err := fs.OpenFile(dir, os.O_RDONLY) + f, err := fs.OpenFile(dir, O_RDONLY, false) if err != nil { t.Fatal(err) } @@ -123,7 +123,7 @@ func TestFSReader(t *testing.T) { { name: "file/Stat", f: func(t *testing.T, fs FS) { - f, err := fs.OpenFile(filename, os.O_RDONLY) + f, err := fs.OpenFile(filename, O_RDONLY, true) if err != nil { t.Fatal(err) } @@ -295,7 +295,7 @@ func TestFSReaderMinFileSize(t *testing.T) { AllowEmptyFile: test.allowEmpty, } - f, err := fs.OpenFile("testfile", os.O_RDONLY) + f, err := fs.OpenFile("testfile", O_RDONLY, false) if err != nil { t.Fatal(err) } diff --git a/internal/fs/fs_track.go b/internal/fs/fs_track.go index 9912ac45b..9ebdbb8c4 100644 --- a/internal/fs/fs_track.go +++ b/internal/fs/fs_track.go @@ -16,8 +16,8 @@ type Track struct { } // OpenFile wraps the OpenFile method of the underlying file system. -func (fs Track) OpenFile(name string, flag int) (File, error) { - f, err := fs.FS.OpenFile(fixpath(name), flag) +func (fs Track) OpenFile(name string, flag int, metadataOnly bool) (File, error) { + f, err := fs.FS.OpenFile(name, flag, metadataOnly) if err != nil { return nil, err } @@ -31,7 +31,7 @@ type trackFile struct { func newTrackFile(stack []byte, filename string, file File) *trackFile { f := &trackFile{file} - runtime.SetFinalizer(f, func(_ *trackFile) { + runtime.SetFinalizer(f, func(_ any) { fmt.Fprintf(os.Stderr, "file %s not closed\n\nStacktrack:\n%s\n", filename, stack) panic("file " + filename + " not closed") }) diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 899888fb0..7ff777138 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -9,11 +9,18 @@ import ( // FS bundles all methods needed for a file system. type FS interface { - OpenFile(name string, flag int) (File, error) + // OpenFile opens a file or directory for reading. + // + // If metadataOnly is set, an implementation MUST return a File object for + // arbitrary file types including symlinks. The implementation may internally use + // the given file path or a file handle. In particular, an implementation may + // delay actually accessing the underlying filesystem. + // + // Only the O_NOFOLLOW and O_DIRECTORY flags are supported. + OpenFile(name string, flag int, metadataOnly bool) (File, error) Lstat(name string) (os.FileInfo, error) DeviceID(fi os.FileInfo) (deviceID uint64, err error) ExtendedStat(fi os.FileInfo) ExtendedFileInfo - NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) Join(elem ...string) string Separator() string @@ -26,11 +33,23 @@ type FS interface { Base(path string) string } -// File is an open file on a file system. +// File is an open file on a file system. When opened as metadataOnly, an +// implementation may opt to perform filesystem operations using the filepath +// instead of actually opening the file. type File interface { + // MakeReadable reopens a File that was opened metadataOnly for reading. + // The method must not be called for files that are opened for reading. + // If possible, the underlying file should be reopened atomically. + // MakeReadable must work for files and directories. + MakeReadable() error + io.Reader io.Closer Readdirnames(n int) ([]string, error) Stat() (os.FileInfo, error) + // ToNode returns a restic.Node for the File. The internally used os.FileInfo + // must be consistent with that returned by Stat(). In particular, the metadata + // returned by consecutive calls to Stat() and ToNode() must match. + ToNode(ignoreXattrListError bool) (*restic.Node, error) } diff --git a/internal/fs/node_test.go b/internal/fs/node_test.go index 58facceb1..65098e304 100644 --- a/internal/fs/node_test.go +++ b/internal/fs/node_test.go @@ -17,56 +17,26 @@ import ( rtest "github.com/restic/restic/internal/test" ) -func BenchmarkNodeFillUser(t *testing.B) { - tempfile, err := os.CreateTemp("", "restic-test-temp-") - if err != nil { - t.Fatal(err) - } - - fi, err := tempfile.Stat() - if err != nil { - t.Fatal(err) - } - +func BenchmarkNodeFromFileInfo(t *testing.B) { + tempfile, err := os.CreateTemp(t.TempDir(), "restic-test-temp-") + rtest.OK(t, err) path := tempfile.Name() + rtest.OK(t, tempfile.Close()) + fs := Local{} + f, err := fs.OpenFile(path, O_NOFOLLOW, true) + rtest.OK(t, err) + _, err = f.Stat() + rtest.OK(t, err) t.ResetTimer() for i := 0; i < t.N; i++ { - _, err := fs.NodeFromFileInfo(path, fi, false) + _, err := f.ToNode(false) rtest.OK(t, err) } - rtest.OK(t, tempfile.Close()) - rtest.RemoveAll(t, tempfile.Name()) -} - -func BenchmarkNodeFromFileInfo(t *testing.B) { - tempfile, err := os.CreateTemp("", "restic-test-temp-") - if err != nil { - t.Fatal(err) - } - - fi, err := tempfile.Stat() - if err != nil { - t.Fatal(err) - } - - path := tempfile.Name() - fs := Local{} - - t.ResetTimer() - - for i := 0; i < t.N; i++ { - _, err := fs.NodeFromFileInfo(path, fi, false) - if err != nil { - t.Fatal(err) - } - } - - rtest.OK(t, tempfile.Close()) - rtest.RemoveAll(t, tempfile.Name()) + rtest.OK(t, f.Close()) } func parseTime(s string) time.Time { @@ -249,14 +219,14 @@ func TestNodeRestoreAt(t *testing.T) { rtest.OK(t, NodeCreateAt(&test, nodePath)) rtest.OK(t, NodeRestoreMetadata(&test, nodePath, func(msg string) { rtest.OK(t, fmt.Errorf("Warning triggered for path: %s: %s", nodePath, msg)) })) - fi, err := os.Lstat(nodePath) - rtest.OK(t, err) - fs := &Local{} - n2, err := fs.NodeFromFileInfo(nodePath, fi, false) + meta, err := fs.OpenFile(nodePath, O_NOFOLLOW, true) rtest.OK(t, err) - n3, err := fs.NodeFromFileInfo(nodePath, fi, true) + n2, err := meta.ToNode(false) rtest.OK(t, err) + n3, err := meta.ToNode(true) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) rtest.Assert(t, n2.Equals(*n3), "unexpected node info mismatch %v", cmp.Diff(n2, n3)) rtest.Assert(t, test.Name == n2.Name, diff --git a/internal/fs/node_unix_test.go b/internal/fs/node_unix_test.go index 6b47eafba..1eb1ee506 100644 --- a/internal/fs/node_unix_test.go +++ b/internal/fs/node_unix_test.go @@ -114,16 +114,14 @@ func TestNodeFromFileInfo(t *testing.T) { return } - if fi.Sys() == nil { - t.Skip("fi.Sys() is nil") - return - } - fs := &Local{} - node, err := fs.NodeFromFileInfo(test.filename, fi, false) - if err != nil { - t.Fatal(err) - } + meta, err := fs.OpenFile(test.filename, O_NOFOLLOW, true) + rtest.OK(t, err) + node, err := meta.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) + + rtest.OK(t, err) switch node.Type { case restic.NodeTypeFile, restic.NodeTypeSymlink: diff --git a/internal/fs/node_windows_test.go b/internal/fs/node_windows_test.go index 1bb76b204..f75df54d3 100644 --- a/internal/fs/node_windows_test.go +++ b/internal/fs/node_windows_test.go @@ -222,11 +222,11 @@ func restoreAndGetNode(t *testing.T, tempDir string, testNode *restic.Node, warn test.OK(t, errors.Wrapf(err, "Failed to restore metadata for: %s", testPath)) fs := &Local{} - fi, err := fs.Lstat(testPath) - test.OK(t, errors.Wrapf(err, "Could not Lstat for path: %s", testPath)) - - nodeFromFileInfo, err := fs.NodeFromFileInfo(testPath, fi, false) + meta, err := fs.OpenFile(testPath, O_NOFOLLOW, true) + test.OK(t, err) + nodeFromFileInfo, err := meta.ToNode(false) test.OK(t, errors.Wrapf(err, "Could not get NodeFromFileInfo for path: %s", testPath)) + test.OK(t, meta.Close()) return testPath, nodeFromFileInfo } diff --git a/internal/restic/tree_test.go b/internal/restic/tree_test.go index f1979f135..07ca254f1 100644 --- a/internal/restic/tree_test.go +++ b/internal/restic/tree_test.go @@ -83,13 +83,17 @@ func TestNodeMarshal(t *testing.T) { } } -func TestNodeComparison(t *testing.T) { - fs := &fs.Local{} - fi, err := fs.Lstat("tree_test.go") +func nodeForFile(t *testing.T, name string) *restic.Node { + f, err := (&fs.Local{}).OpenFile(name, fs.O_NOFOLLOW, true) rtest.OK(t, err) + node, err := f.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, f.Close()) + return node +} - node, err := fs.NodeFromFileInfo("tree_test.go", fi, false) - rtest.OK(t, err) +func TestNodeComparison(t *testing.T) { + node := nodeForFile(t, "tree_test.go") n2 := *node rtest.Assert(t, node.Equals(n2), "nodes aren't equal") @@ -127,11 +131,7 @@ func TestTreeEqualSerialization(t *testing.T) { builder := restic.NewTreeJSONBuilder() for _, fn := range files[:i] { - fs := &fs.Local{} - fi, err := fs.Lstat(fn) - rtest.OK(t, err) - node, err := fs.NodeFromFileInfo(fn, fi, false) - rtest.OK(t, err) + node := nodeForFile(t, fn) rtest.OK(t, tree.Insert(node)) rtest.OK(t, builder.AddNode(node))