From 4b0ca9ddabd342a552dc831099d13d185fe3f572 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Tue, 23 Apr 2019 22:23:35 +0200 Subject: [PATCH 1/3] Add test for #2249 --- internal/archiver/archiver_test.go | 174 +++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 419b3a91c..1d0d0f6de 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/restic/restic/internal/checker" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" @@ -1916,3 +1917,176 @@ func TestArchiverAbortEarlyOnError(t *testing.T) { }) } } + +func snapshot(t testing.TB, repo restic.Repository, fs fs.FS, parent restic.ID, filename string) (restic.ID, *restic.Node) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + arch := New(repo, fs, Options{}) + + sopts := SnapshotOptions{ + Time: time.Now(), + ParentSnapshot: parent, + } + snapshot, snapshotID, err := arch.Snapshot(ctx, []string{filename}, sopts) + if err != nil { + t.Fatal(err) + } + + tree, err := repo.LoadTree(ctx, *snapshot.Tree) + if err != nil { + t.Fatal(err) + } + + node := tree.Find(filename) + if node == nil { + t.Fatalf("unable to find node for testfile in snapshot") + } + + return snapshotID, node +} + +func chmod(t testing.TB, filename string, mode os.FileMode) { + err := os.Chmod(filename, mode) + if err != nil { + t.Fatal(err) + } +} + +// StatFS allows overwriting what is returned by the Lstat function. +type StatFS struct { + fs.FS + + OverrideLstat map[string]os.FileInfo +} + +func (fs *StatFS) Lstat(name string) (os.FileInfo, error) { + if fi, ok := fs.OverrideLstat[name]; ok { + return fi, nil + } + + return fs.FS.Lstat(name) +} + +func (fs *StatFS) OpenFile(name string, flags int, perm os.FileMode) (fs.File, error) { + if fi, ok := fs.OverrideLstat[name]; ok { + f, err := fs.FS.OpenFile(name, flags, perm) + if err != nil { + return nil, err + } + + wrappedFile := fileStat{ + File: f, + fi: fi, + } + return wrappedFile, nil + } + + return fs.FS.OpenFile(name, flags, perm) +} + +type fileStat struct { + fs.File + fi os.FileInfo +} + +func (f fileStat) Stat() (os.FileInfo, error) { + return f.fi, nil +} + +type wrappedFileInfo struct { + os.FileInfo + sys interface{} + mode os.FileMode +} + +func (fi wrappedFileInfo) Sys() interface{} { + return fi.sys +} + +func (fi wrappedFileInfo) Mode() os.FileMode { + return fi.mode +} + +func TestMetadataChanged(t *testing.T) { + files := TestDir{ + "testfile": TestFile{ + Content: "foo bar test file", + }, + } + + tempdir, repo, cleanup := prepareTempdirRepoSrc(t, files) + defer cleanup() + + back := fs.TestChdir(t, tempdir) + defer back() + + // get metadata + fi := lstat(t, "testfile") + want, err := restic.NodeFromFileInfo("testfile", fi) + if err != nil { + t.Fatal(err) + } + + fs := &StatFS{ + FS: fs.Local{}, + OverrideLstat: map[string]os.FileInfo{ + "testfile": fi, + }, + } + + snapshotID, node2 := snapshot(t, repo, fs, restic.ID{}, "testfile") + + // set some values so we can then compare the nodes + want.Content = node2.Content + want.Path = "" + want.ExtendedAttributes = nil + + // make sure that metadata was recorded successfully + if !cmp.Equal(want, node2) { + t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2)) + } + + // modify the mode + stat, ok := fi.Sys().(*syscall.Stat_t) + if ok { + // change a few values + stat.Mode = 0400 + stat.Uid = 1234 + stat.Gid = 1235 + + // wrap the os.FileInfo so we can return a modified stat_t + fi = wrappedFileInfo{ + FileInfo: fi, + sys: stat, + mode: 0400, + } + fs.OverrideLstat["testfile"] = fi + } else { + // skip the test on this platform + t.Skipf("unable to modify os.FileInfo, Sys() returned %T", fi.Sys()) + } + + want, err = restic.NodeFromFileInfo("testfile", fi) + if err != nil { + t.Fatal(err) + } + + // make another snapshot + snapshotID, node3 := snapshot(t, repo, fs, snapshotID, "testfile") + + // set some values so we can then compare the nodes + want.Content = node2.Content + want.Path = "" + want.ExtendedAttributes = nil + + // make sure that metadata was recorded successfully + if !cmp.Equal(want, node3) { + t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2)) + } + + // make sure the content matches + TestEnsureFileContent(context.Background(), t, repo, "testfile", node3, files["testfile"].(TestFile)) + + checker.TestCheckRepo(t, repo) +} From 389067fb8ba5ec667b55e200e7481c5070bdbc03 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 24 Apr 2019 15:07:26 +0200 Subject: [PATCH 2/3] Only use list of blobs for old node Closes #2249 --- internal/archiver/archiver.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index b21f79e87..f72f1b338 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -384,12 +384,19 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous return FutureNode{}, true, nil } - // use previous node if the file hasn't changed + // use previous list of blobs if the file hasn't changed if previous != nil && !fileChanged(fi, previous, arch.IgnoreInode) { - debug.Log("%v hasn't changed, returning old node", target) + debug.Log("%v hasn't changed, using old list of blobs", target) arch.CompleteItem(snPath, previous, previous, ItemStats{}, time.Since(start)) arch.CompleteBlob(snPath, previous.Size) - fn.node = previous + fn.node, err = arch.nodeFromFileInfo(target, fi) + if err != nil { + return FutureNode{}, false, err + } + + // copy list of blobs + fn.node.Content = previous.Content + _ = file.Close() return fn, false, nil } From 4f45b14f25befdb27a2110c0f05e4927ce84198b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 24 Apr 2019 15:17:25 +0200 Subject: [PATCH 3/3] Add changelog file --- changelog/unreleased/issue-2249 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/issue-2249 diff --git a/changelog/unreleased/issue-2249 b/changelog/unreleased/issue-2249 new file mode 100644 index 000000000..3fb4163f7 --- /dev/null +++ b/changelog/unreleased/issue-2249 @@ -0,0 +1,6 @@ +Bugfix: Read fresh metadata for unmodified files + +Restic took all metadata for files which were detected as unmodified, not taking into account changed metadata (ownership, mode). This is now corrected. + +https://github.com/restic/restic/issues/2249 +https://github.com/restic/restic/pull/2252