From 3a995172b759460cc483c5bc592149aad0c341da Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 21:37:32 +0200 Subject: [PATCH 1/9] fs: rewrite Reader to build fs tree up front This adds proper support for filenames that include directories. For example, `/foo/bar` would result in an error when trying to open `/foo`. The directory tree is now build upfront. This ensures let's the directory tree construction be handled only once. All accessors then only have to look up the constructed directory entries. --- cmd/restic/cmd_backup.go | 10 +-- internal/archiver/archiver_test.go | 20 ++--- internal/fs/fs_reader.go | 138 ++++++++++++++++++----------- internal/fs/fs_reader_test.go | 118 +++++++++++++++--------- internal/restorer/restorer_test.go | 8 +- 5 files changed, 177 insertions(+), 117 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 70c0d2fb9..62f2184ae 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -591,12 +591,10 @@ func runBackup(ctx context.Context, opts BackupOptions, gopts GlobalOptions, ter return err } } - targetFS = &fs.Reader{ - ModTime: timeStamp, - Name: filename, - Mode: 0644, - ReadCloser: source, - } + targetFS = fs.NewReader(filename, source, fs.ReaderOptions{ + ModTime: timeStamp, + Mode: 0644, + }) targets = []string{filename} } diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 9a25f7fad..7b511ec84 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -174,12 +174,10 @@ func TestArchiverSaveFileReaderFS(t *testing.T) { ts := time.Now() filename := "xx" - readerFs := &fs.Reader{ - ModTime: ts, - Mode: 0123, - Name: filename, - ReadCloser: io.NopCloser(strings.NewReader(test.Data)), - } + readerFs := fs.NewReader(filename, io.NopCloser(strings.NewReader(test.Data)), fs.ReaderOptions{ + ModTime: ts, + Mode: 0123, + }) node, stats := saveFile(t, repo, filename, readerFs) @@ -288,12 +286,10 @@ func TestArchiverSaveReaderFS(t *testing.T) { ts := time.Now() filename := "xx" - readerFs := &fs.Reader{ - ModTime: ts, - Mode: 0123, - Name: filename, - ReadCloser: io.NopCloser(strings.NewReader(test.Data)), - } + readerFs := fs.NewReader(filename, io.NopCloser(strings.NewReader(test.Data)), fs.ReaderOptions{ + ModTime: ts, + Mode: 0123, + }) arch := New(repo, readerFs, Options{}) arch.Error = func(item string, err error) error { diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index bbe5c95ab..7334bd5be 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -19,97 +19,129 @@ import ( // be opened once, all subsequent open calls return syscall.EIO. For Lstat(), // the provided FileInfo is returned. type Reader struct { - Name string - io.ReadCloser + items map[string]readerItem +} - // for FileInfo +type ReaderOptions struct { Mode os.FileMode ModTime time.Time Size int64 AllowEmptyFile bool +} - open sync.Once +type readerItem struct { + open *sync.Once + fi *ExtendedFileInfo + rc io.ReadCloser + allowEmptyFile bool + + children []string } // statically ensure that Local implements FS. var _ FS = &Reader{} +func NewReader(name string, r io.ReadCloser, opts ReaderOptions) *Reader { + items := make(map[string]readerItem) + name = readerCleanPath(name) + + isFile := true + for { + if isFile { + fi := &ExtendedFileInfo{ + Name: path.Base(name), + Mode: opts.Mode, + ModTime: opts.ModTime, + Size: opts.Size, + } + items[name] = readerItem{ + open: &sync.Once{}, + fi: fi, + rc: r, + allowEmptyFile: opts.AllowEmptyFile, + } + isFile = false + } else { + fi := &ExtendedFileInfo{ + Name: path.Base(name), + Mode: os.ModeDir | 0755, + ModTime: time.Now(), // FIXME + Size: 0, + } + items[name] = readerItem{ + fi: fi, + // keep the children set during the previous iteration + children: items[name].children, + } + } + + parent := path.Dir(name) + if parent == name { + break + } + // add the current file to the children of the parent directory + item := items[parent] + item.children = append(item.children, path.Base(name)) + items[parent] = item + + name = parent + } + return &Reader{ + items: items, + } +} + +func readerCleanPath(name string) string { + return path.Clean("/" + name) +} + // VolumeName returns leading volume name, for the Reader file system it's // always the empty string. func (fs *Reader) VolumeName(_ string) string { return "" } -func (fs *Reader) fi() *ExtendedFileInfo { - return &ExtendedFileInfo{ - Name: fs.Name, - Mode: fs.Mode, - ModTime: fs.ModTime, - Size: fs.Size, - } -} - 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)) } - switch name { - case fs.Name: - fs.open.Do(func() { - f = newReaderFile(fs.ReadCloser, fs.fi(), fs.AllowEmptyFile) + name = readerCleanPath(name) + item, ok := fs.items[name] + if !ok { + return nil, pathError("open", name, syscall.ENOENT) + } + + // Check if the path matches our target file + if item.rc != nil { + item.open.Do(func() { + f = newReaderFile(item.rc, item.fi, item.allowEmptyFile) }) if f == nil { return nil, pathError("open", name, syscall.EIO) } - return f, nil - case "/", ".": - f = fakeDir{ - entries: []string{fs.fi().Name}, - } return f, nil } - return nil, pathError("open", name, syscall.ENOENT) + f = fakeDir{ + entries: slices.Clone(item.children), + } + return f, nil } // Lstat returns the FileInfo structure describing the named file. -// If the file is a symbolic link, the returned FileInfo -// describes the symbolic link. Lstat makes no attempt to follow the link. // If there is an error, it will be of type *os.PathError. func (fs *Reader) Lstat(name string) (*ExtendedFileInfo, error) { - getDirInfo := func(name string) *ExtendedFileInfo { - return &ExtendedFileInfo{ - Name: fs.Base(name), - Size: 0, - Mode: os.ModeDir | 0755, - ModTime: time.Now(), - } + name = readerCleanPath(name) + item, ok := fs.items[name] + if !ok { + return nil, pathError("lstat", name, os.ErrNotExist) } - - switch name { - case fs.Name: - return fs.fi(), nil - case "/", ".": - return getDirInfo(name), nil - } - - dir := fs.Dir(fs.Name) - for { - if dir == "/" || dir == "." { - break - } - if name == dir { - return getDirInfo(name), nil - } - dir = fs.Dir(dir) - } - - return nil, pathError("lstat", name, os.ErrNotExist) + return item.fi, nil } // Join joins any number of path elements into a single path, adding a @@ -137,7 +169,7 @@ func (fs *Reader) IsAbs(_ string) bool { // // For the Reader, all paths are absolute. func (fs *Reader) Abs(p string) (string, error) { - return path.Clean(p), nil + return readerCleanPath(p), nil } // Clean returns the cleaned path. For details, see filepath.Clean. diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index 257bfbbac..cd2a68b1b 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -82,27 +82,30 @@ func checkFileInfo(t testing.TB, fi *ExtendedFileInfo, filename string, modtime } } -func TestFSReader(t *testing.T) { - data := test.Random(55, 1<<18+588) - now := time.Now() - filename := "foobar" +type fsTest []struct { + name string + f func(t *testing.T, fs FS) +} - var tests = []struct { - name string - f func(t *testing.T, fs FS) - }{ +func createReadDirTest(fpath, filename string) fsTest { + return fsTest{ { - name: "Readdirnames-slash", + name: "Readdirnames-slash-" + fpath, f: func(t *testing.T, fs FS) { - verifyDirectoryContents(t, fs, "/", []string{filename}) + verifyDirectoryContents(t, fs, "/"+fpath, []string{filename}) }, }, { - name: "Readdirnames-current", + name: "Readdirnames-current-" + fpath, f: func(t *testing.T, fs FS) { - verifyDirectoryContents(t, fs, ".", []string{filename}) + verifyDirectoryContents(t, fs, path.Clean(fpath), []string{filename}) }, }, + } +} + +func createFileTest(filename string, now time.Time, data []byte) fsTest { + return fsTest{ { name: "file/OpenFile", f: func(t *testing.T, fs FS) { @@ -141,70 +144,108 @@ func TestFSReader(t *testing.T) { checkFileInfo(t, fi, filename, now, 0644, false) }, }, + } +} + +func createDirTest(fpath string) fsTest { + return fsTest{ { - name: "dir/Lstat-slash", + name: "dir/Lstat-slash-" + fpath, f: func(t *testing.T, fs FS) { - fi, err := fs.Lstat("/") + fi, err := fs.Lstat("/" + fpath) if err != nil { t.Fatal(err) } - checkFileInfo(t, fi, "/", time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) }, }, { - name: "dir/Lstat-current", + name: "dir/Lstat-current-" + fpath, f: func(t *testing.T, fs FS) { - fi, err := fs.Lstat(".") + fi, err := fs.Lstat("./" + fpath) if err != nil { t.Fatal(err) } - checkFileInfo(t, fi, ".", time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) }, }, { - name: "dir/Lstat-error-not-exist", + name: "dir/Lstat-error-not-exist-" + fpath, f: func(t *testing.T, fs FS) { - _, err := fs.Lstat("other") + _, err := fs.Lstat(fpath + "/other") if !errors.Is(err, os.ErrNotExist) { t.Fatal(err) } }, }, { - name: "dir/Open-slash", + name: "dir/Open-slash-" + fpath, f: func(t *testing.T, fs FS) { - fi, err := fs.Lstat("/") + fi, err := fs.Lstat("/" + fpath) if err != nil { t.Fatal(err) } - checkFileInfo(t, fi, "/", time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) }, }, { - name: "dir/Open-current", + name: "dir/Open-current-" + fpath, f: func(t *testing.T, fs FS) { - fi, err := fs.Lstat(".") + fi, err := fs.Lstat("./" + fpath) if err != nil { t.Fatal(err) } - checkFileInfo(t, fi, ".", time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) }, }, } +} + +func TestFSReader(t *testing.T) { + data := test.Random(55, 1<<18+588) + now := time.Now() + filename := "foobar" + + tests := createReadDirTest("", filename) + tests = append(tests, createFileTest(filename, now, data)...) + tests = append(tests, createDirTest("")...) for _, test := range tests { - fs := &Reader{ - Name: filename, - ReadCloser: io.NopCloser(bytes.NewReader(data)), - + fs := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ Mode: 0644, Size: int64(len(data)), ModTime: now, - } + }) + + t.Run(test.name, func(t *testing.T) { + test.f(t, fs) + }) + } +} + +func TestFSReaderNested(t *testing.T) { + data := test.Random(55, 1<<18+588) + now := time.Now() + filename := "foo/sub/bar" + + tests := createReadDirTest("", "foo") + tests = append(tests, createReadDirTest("foo", "sub")...) + tests = append(tests, createReadDirTest("foo/sub", "bar")...) + tests = append(tests, createFileTest(filename, now, data)...) + tests = append(tests, createDirTest("")...) + tests = append(tests, createDirTest("foo")...) + tests = append(tests, createDirTest("foo/sub")...) + + for _, test := range tests { + fs := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ + Mode: 0644, + Size: int64(len(data)), + ModTime: now, + }) t.Run(test.name, func(t *testing.T) { test.f(t, fs) @@ -232,16 +273,13 @@ func TestFSReaderDir(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - fs := &Reader{ - Name: test.filename, - ReadCloser: io.NopCloser(bytes.NewReader(data)), - + fs := NewReader(test.filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ Mode: 0644, Size: int64(len(data)), ModTime: now, - } + }) - dir := path.Dir(fs.Name) + dir := path.Dir(test.filename) for { if dir == "/" || dir == "." { break @@ -287,13 +325,11 @@ func TestFSReaderMinFileSize(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - fs := &Reader{ - Name: "testfile", - ReadCloser: io.NopCloser(strings.NewReader(test.data)), + fs := NewReader("testfile", io.NopCloser(strings.NewReader(test.data)), ReaderOptions{ Mode: 0644, ModTime: time.Now(), AllowEmptyFile: test.allowEmpty, - } + }) f, err := fs.OpenFile("testfile", O_RDONLY, false) if err != nil { diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index b48ae137c..cd0307359 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -908,11 +908,9 @@ func TestRestorerSparseFiles(t *testing.T) { var zeros [1<<20 + 13]byte - target := &fs.Reader{ - Mode: 0600, - Name: "/zeros", - ReadCloser: io.NopCloser(bytes.NewReader(zeros[:])), - } + target := fs.NewReader("/zeros", io.NopCloser(bytes.NewReader(zeros[:])), fs.ReaderOptions{ + Mode: 0600, + }) sc := archiver.NewScanner(target) err := sc.Scan(context.TODO(), []string{"/zeros"}) rtest.OK(t, err) From 19f48084ea2d30cbc15d4327ea8a1bdb96d443d2 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 21:43:31 +0200 Subject: [PATCH 2/9] fs/reader: use modification time for file and directories This ensures that a fixed input generates a fully deterministic output file structure. --- internal/fs/fs_reader.go | 2 +- internal/fs/fs_reader_test.go | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 7334bd5be..7e6aa0f58 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -66,7 +66,7 @@ func NewReader(name string, r io.ReadCloser, opts ReaderOptions) *Reader { fi := &ExtendedFileInfo{ Name: path.Base(name), Mode: os.ModeDir | 0755, - ModTime: time.Now(), // FIXME + ModTime: opts.ModTime, Size: 0, } items[name] = readerItem{ diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index cd2a68b1b..f49cc0d8d 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -69,7 +69,7 @@ func checkFileInfo(t testing.TB, fi *ExtendedFileInfo, filename string, modtime t.Errorf("Mode has wrong value, want 0%o, got 0%o", mode, fi.Mode) } - if !modtime.Equal(time.Time{}) && !fi.ModTime.Equal(modtime) { + if !fi.ModTime.Equal(modtime) { t.Errorf("ModTime has wrong value, want %v, got %v", modtime, fi.ModTime) } @@ -147,7 +147,7 @@ func createFileTest(filename string, now time.Time, data []byte) fsTest { } } -func createDirTest(fpath string) fsTest { +func createDirTest(fpath string, now time.Time) fsTest { return fsTest{ { name: "dir/Lstat-slash-" + fpath, @@ -157,7 +157,7 @@ func createDirTest(fpath string) fsTest { t.Fatal(err) } - checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, { @@ -168,7 +168,7 @@ func createDirTest(fpath string) fsTest { t.Fatal(err) } - checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, { @@ -188,7 +188,7 @@ func createDirTest(fpath string) fsTest { t.Fatal(err) } - checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, { @@ -199,7 +199,7 @@ func createDirTest(fpath string) fsTest { t.Fatal(err) } - checkFileInfo(t, fi, "/"+fpath, time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, } @@ -212,7 +212,7 @@ func TestFSReader(t *testing.T) { tests := createReadDirTest("", filename) tests = append(tests, createFileTest(filename, now, data)...) - tests = append(tests, createDirTest("")...) + tests = append(tests, createDirTest("", now)...) for _, test := range tests { fs := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ @@ -236,9 +236,9 @@ func TestFSReaderNested(t *testing.T) { tests = append(tests, createReadDirTest("foo", "sub")...) tests = append(tests, createReadDirTest("foo/sub", "bar")...) tests = append(tests, createFileTest(filename, now, data)...) - tests = append(tests, createDirTest("")...) - tests = append(tests, createDirTest("foo")...) - tests = append(tests, createDirTest("foo/sub")...) + tests = append(tests, createDirTest("", now)...) + tests = append(tests, createDirTest("foo", now)...) + tests = append(tests, createDirTest("foo/sub", now)...) for _, test := range tests { fs := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ @@ -290,7 +290,7 @@ func TestFSReaderDir(t *testing.T) { t.Fatal(err) } - checkFileInfo(t, fi, dir, time.Time{}, os.ModeDir|0755, true) + checkFileInfo(t, fi, dir, now, os.ModeDir|0755, true) dir = path.Dir(dir) } From 70e1037a49e88dcee1efecf0a136bd3139610742 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 21:49:25 +0200 Subject: [PATCH 3/9] fs/reader: fix open+stat handling --- internal/fs/fs_reader.go | 3 +++ internal/fs/fs_reader_test.go | 30 ++++++++++++++++++++---------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 7e6aa0f58..97e09d1d2 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -128,6 +128,9 @@ func (fs *Reader) OpenFile(name string, flag int, _ bool) (f File, err error) { } f = fakeDir{ + fakeFile: fakeFile{ + fi: item.fi, + }, entries: slices.Clone(item.children), } return f, nil diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index f49cc0d8d..ef95083ae 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -183,28 +183,38 @@ func createDirTest(fpath string, now time.Time) fsTest { { name: "dir/Open-slash-" + fpath, f: func(t *testing.T, fs FS) { - fi, err := fs.Lstat("/" + fpath) - if err != nil { - t.Fatal(err) - } - + fi := fsStatDir(t, fs, "/"+fpath) checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, { name: "dir/Open-current-" + fpath, f: func(t *testing.T, fs FS) { - fi, err := fs.Lstat("./" + fpath) - if err != nil { - t.Fatal(err) - } - + fi := fsStatDir(t, fs, "./"+fpath) checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, } } +func fsStatDir(t *testing.T, fs FS, fpath string) *ExtendedFileInfo { + f, err := fs.OpenFile(fpath, O_RDONLY, false) + if err != nil { + t.Fatal(err) + } + + fi, err := f.Stat() + if err != nil { + t.Fatal(err) + } + + err = f.Close() + if err != nil { + t.Fatal(err) + } + return fi +} + func TestFSReader(t *testing.T) { data := test.Random(55, 1<<18+588) now := time.Now() From e7c1e4f1ff86002fe2f419c55ba1108f39fd16c1 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 21:50:47 +0200 Subject: [PATCH 4/9] fs/reader: deduplicate test code --- internal/fs/fs_reader_test.go | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index ef95083ae..3df3906aa 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -126,21 +126,7 @@ func createFileTest(filename string, now time.Time, data []byte) fsTest { { name: "file/Stat", f: func(t *testing.T, fs FS) { - f, err := fs.OpenFile(filename, O_RDONLY, true) - if err != nil { - t.Fatal(err) - } - - fi, err := f.Stat() - if err != nil { - t.Fatal(err) - } - - err = f.Close() - if err != nil { - t.Fatal(err) - } - + fi := fsOpenAndStat(t, fs, filename, true) checkFileInfo(t, fi, filename, now, 0644, false) }, }, @@ -183,22 +169,22 @@ func createDirTest(fpath string, now time.Time) fsTest { { name: "dir/Open-slash-" + fpath, f: func(t *testing.T, fs FS) { - fi := fsStatDir(t, fs, "/"+fpath) + fi := fsOpenAndStat(t, fs, "/"+fpath, false) checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, { name: "dir/Open-current-" + fpath, f: func(t *testing.T, fs FS) { - fi := fsStatDir(t, fs, "./"+fpath) + fi := fsOpenAndStat(t, fs, "./"+fpath, false) checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, }, } } -func fsStatDir(t *testing.T, fs FS, fpath string) *ExtendedFileInfo { - f, err := fs.OpenFile(fpath, O_RDONLY, false) +func fsOpenAndStat(t *testing.T, fs FS, fpath string, metadataOnly bool) *ExtendedFileInfo { + f, err := fs.OpenFile(fpath, O_RDONLY, metadataOnly) if err != nil { t.Fatal(err) } From 6e91ea3397f6f5b20ea2725733a98aa970fcf298 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 21:54:15 +0200 Subject: [PATCH 5/9] fs/reader: use test helpers --- internal/fs/fs_reader_test.go | 98 ++++++++++------------------------- 1 file changed, 28 insertions(+), 70 deletions(-) diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index 3df3906aa..5176e467d 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -17,19 +17,11 @@ import ( func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte) { f, err := fs.OpenFile(filename, O_RDONLY, false) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) buf, err := io.ReadAll(f) - if err != nil { - t.Fatal(err) - } - - err = f.Close() - if err != nil { - t.Fatal(err) - } + test.OK(t, err) + test.OK(t, f.Close()) if !cmp.Equal(want, buf) { t.Error(cmp.Diff(want, buf)) @@ -38,19 +30,11 @@ 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, O_RDONLY, false) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) entries, err := f.Readdirnames(-1) - if err != nil { - t.Fatal(err) - } - - err = f.Close() - if err != nil { - t.Fatal(err) - } + test.OK(t, err) + test.OK(t, f.Close()) sort.Strings(want) sort.Strings(entries) @@ -116,9 +100,7 @@ func createFileTest(filename string, now time.Time, data []byte) fsTest { name: "file/Lstat", f: func(t *testing.T, fs FS) { fi, err := fs.Lstat(filename) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) checkFileInfo(t, fi, filename, now, 0644, false) }, @@ -139,9 +121,7 @@ func createDirTest(fpath string, now time.Time) fsTest { name: "dir/Lstat-slash-" + fpath, f: func(t *testing.T, fs FS) { fi, err := fs.Lstat("/" + fpath) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, @@ -150,9 +130,7 @@ func createDirTest(fpath string, now time.Time) fsTest { name: "dir/Lstat-current-" + fpath, f: func(t *testing.T, fs FS) { fi, err := fs.Lstat("./" + fpath) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) checkFileInfo(t, fi, "/"+fpath, now, os.ModeDir|0755, true) }, @@ -161,9 +139,7 @@ func createDirTest(fpath string, now time.Time) fsTest { name: "dir/Lstat-error-not-exist-" + fpath, f: func(t *testing.T, fs FS) { _, err := fs.Lstat(fpath + "/other") - if !errors.Is(err, os.ErrNotExist) { - t.Fatal(err) - } + test.Assert(t, errors.Is(err, os.ErrNotExist), "unexpected error, got %v, expected %v", err, os.ErrNotExist) }, }, { @@ -185,19 +161,11 @@ func createDirTest(fpath string, now time.Time) fsTest { func fsOpenAndStat(t *testing.T, fs FS, fpath string, metadataOnly bool) *ExtendedFileInfo { f, err := fs.OpenFile(fpath, O_RDONLY, metadataOnly) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) fi, err := f.Stat() - if err != nil { - t.Fatal(err) - } - - err = f.Close() - if err != nil { - t.Fatal(err) - } + test.OK(t, err) + test.OK(t, f.Close()) return fi } @@ -267,24 +235,22 @@ func TestFSReaderDir(t *testing.T) { }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - fs := NewReader(test.filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ + for _, tst := range tests { + t.Run(tst.name, func(t *testing.T) { + fs := NewReader(tst.filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ Mode: 0644, Size: int64(len(data)), ModTime: now, }) - dir := path.Dir(test.filename) + dir := path.Dir(tst.filename) for { if dir == "/" || dir == "." { break } fi, err := fs.Lstat(dir) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) checkFileInfo(t, fi, dir, now, os.ModeDir|0755, true) @@ -319,38 +285,30 @@ func TestFSReaderMinFileSize(t *testing.T) { }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - fs := NewReader("testfile", io.NopCloser(strings.NewReader(test.data)), ReaderOptions{ + for _, tst := range tests { + t.Run(tst.name, func(t *testing.T) { + fs := NewReader("testfile", io.NopCloser(strings.NewReader(tst.data)), ReaderOptions{ Mode: 0644, ModTime: time.Now(), - AllowEmptyFile: test.allowEmpty, + AllowEmptyFile: tst.allowEmpty, }) f, err := fs.OpenFile("testfile", O_RDONLY, false) - if err != nil { - t.Fatal(err) - } + test.OK(t, err) buf, err := io.ReadAll(f) - if test.readMustErr { + if tst.readMustErr { if err == nil { t.Fatal("expected error not found, got nil") } } else { - if err != nil { - t.Fatal(err) - } + test.OK(t, err) } - if string(buf) != test.data { - t.Fatalf("wrong data returned, want %q, got %q", test.data, string(buf)) - } - - err = f.Close() - if err != nil { - t.Fatal(err) + if string(buf) != tst.data { + t.Fatalf("wrong data returned, want %q, got %q", tst.data, string(buf)) } + test.OK(t, f.Close()) }) } } From ddd48f1e9834500f926de11ddaa110b0cb2f18e6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 21:57:45 +0200 Subject: [PATCH 6/9] fs/reader: test file not exist case --- internal/fs/fs_reader_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index 5176e467d..bde097d5c 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -96,6 +96,13 @@ func createFileTest(filename string, now time.Time, data []byte) fsTest { verifyFileContentOpenFile(t, fs, filename, data) }, }, + { + name: "file/Open-error-not-exist", + f: func(t *testing.T, fs FS) { + _, err := fs.OpenFile(filename+"/other", O_RDONLY, false) + test.Assert(t, errors.Is(err, os.ErrNotExist), "unexpected error, got %v, expected %v", err, os.ErrNotExist) + }, + }, { name: "file/Lstat", f: func(t *testing.T, fs FS) { From 9f39e8a1d3cb1fb07ca2776d933f7c5a81aa3aa6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 22:07:31 +0200 Subject: [PATCH 7/9] fs/reader: return proper error on invalid filename --- cmd/restic/cmd_backup.go | 5 ++++- internal/archiver/archiver_test.go | 7 ++++--- internal/fs/fs_reader.go | 7 +++++-- internal/fs/fs_reader_test.go | 26 ++++++++++++++------------ internal/restorer/restorer_test.go | 5 +++-- 5 files changed, 30 insertions(+), 20 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 62f2184ae..acb5500fd 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -591,10 +591,13 @@ func runBackup(ctx context.Context, opts BackupOptions, gopts GlobalOptions, ter return err } } - targetFS = fs.NewReader(filename, source, fs.ReaderOptions{ + targetFS, err = fs.NewReader(filename, source, fs.ReaderOptions{ ModTime: timeStamp, Mode: 0644, }) + if err != nil { + return fmt.Errorf("failed to backup from stdin: %w", err) + } targets = []string{filename} } diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 7b511ec84..906c83011 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -174,10 +174,11 @@ func TestArchiverSaveFileReaderFS(t *testing.T) { ts := time.Now() filename := "xx" - readerFs := fs.NewReader(filename, io.NopCloser(strings.NewReader(test.Data)), fs.ReaderOptions{ + readerFs, err := fs.NewReader(filename, io.NopCloser(strings.NewReader(test.Data)), fs.ReaderOptions{ ModTime: ts, Mode: 0123, }) + rtest.OK(t, err) node, stats := saveFile(t, repo, filename, readerFs) @@ -286,11 +287,11 @@ func TestArchiverSaveReaderFS(t *testing.T) { ts := time.Now() filename := "xx" - readerFs := fs.NewReader(filename, io.NopCloser(strings.NewReader(test.Data)), fs.ReaderOptions{ + readerFs, err := fs.NewReader(filename, io.NopCloser(strings.NewReader(test.Data)), fs.ReaderOptions{ ModTime: ts, Mode: 0123, }) - + rtest.OK(t, err) arch := New(repo, readerFs, Options{}) arch.Error = func(item string, err error) error { t.Errorf("archiver error for %v: %v", item, err) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 97e09d1d2..c4e98be0f 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -42,9 +42,12 @@ type readerItem struct { // statically ensure that Local implements FS. var _ FS = &Reader{} -func NewReader(name string, r io.ReadCloser, opts ReaderOptions) *Reader { +func NewReader(name string, r io.ReadCloser, opts ReaderOptions) (*Reader, error) { items := make(map[string]readerItem) name = readerCleanPath(name) + if name == "/" { + return nil, fmt.Errorf("invalid filename specified") + } isFile := true for { @@ -89,7 +92,7 @@ func NewReader(name string, r io.ReadCloser, opts ReaderOptions) *Reader { } return &Reader{ items: items, - } + }, nil } func readerCleanPath(name string) string { diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index bde097d5c..083e8a1a5 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -185,15 +185,16 @@ func TestFSReader(t *testing.T) { tests = append(tests, createFileTest(filename, now, data)...) tests = append(tests, createDirTest("", now)...) - for _, test := range tests { - fs := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ + for _, tst := range tests { + fs, err := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ Mode: 0644, Size: int64(len(data)), ModTime: now, }) + test.OK(t, err) - t.Run(test.name, func(t *testing.T) { - test.f(t, fs) + t.Run(tst.name, func(t *testing.T) { + tst.f(t, fs) }) } } @@ -211,15 +212,16 @@ func TestFSReaderNested(t *testing.T) { tests = append(tests, createDirTest("foo", now)...) tests = append(tests, createDirTest("foo/sub", now)...) - for _, test := range tests { - fs := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ + for _, tst := range tests { + fs, err := NewReader(filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ Mode: 0644, Size: int64(len(data)), ModTime: now, }) + test.OK(t, err) - t.Run(test.name, func(t *testing.T) { - test.f(t, fs) + t.Run(tst.name, func(t *testing.T) { + tst.f(t, fs) }) } } @@ -244,12 +246,12 @@ func TestFSReaderDir(t *testing.T) { for _, tst := range tests { t.Run(tst.name, func(t *testing.T) { - fs := NewReader(tst.filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ + fs, err := NewReader(tst.filename, io.NopCloser(bytes.NewReader(data)), ReaderOptions{ Mode: 0644, Size: int64(len(data)), ModTime: now, }) - + test.OK(t, err) dir := path.Dir(tst.filename) for { if dir == "/" || dir == "." { @@ -294,12 +296,12 @@ func TestFSReaderMinFileSize(t *testing.T) { for _, tst := range tests { t.Run(tst.name, func(t *testing.T) { - fs := NewReader("testfile", io.NopCloser(strings.NewReader(tst.data)), ReaderOptions{ + fs, err := NewReader("testfile", io.NopCloser(strings.NewReader(tst.data)), ReaderOptions{ Mode: 0644, ModTime: time.Now(), AllowEmptyFile: tst.allowEmpty, }) - + test.OK(t, err) f, err := fs.OpenFile("testfile", O_RDONLY, false) test.OK(t, err) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index cd0307359..0b5e34d12 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -908,11 +908,12 @@ func TestRestorerSparseFiles(t *testing.T) { var zeros [1<<20 + 13]byte - target := fs.NewReader("/zeros", io.NopCloser(bytes.NewReader(zeros[:])), fs.ReaderOptions{ + target, err := fs.NewReader("/zeros", io.NopCloser(bytes.NewReader(zeros[:])), fs.ReaderOptions{ Mode: 0600, }) + rtest.OK(t, err) sc := archiver.NewScanner(target) - err := sc.Scan(context.TODO(), []string{"/zeros"}) + err = sc.Scan(context.TODO(), []string{"/zeros"}) rtest.OK(t, err) arch := archiver.New(repo, target, archiver.Options{}) From 5bb9d0d996bf9ad3552ee3ebc27b6292f1e881ec Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 22:14:27 +0200 Subject: [PATCH 8/9] backup: test subdirectories in stdin filenames work --- cmd/restic/cmd_backup_integration_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index 06d71e345..0002b207f 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -632,12 +632,15 @@ func TestStdinFromCommand(t *testing.T) { testSetupBackupData(t, env) opts := BackupOptions{ - StdinCommand: true, - StdinFilename: "stdin", + StdinCommand: true, + // test that subdirectories are handled correctly + StdinFilename: "stdin/subdir/file", } testRunBackup(t, filepath.Dir(env.testdata), []string{"python", "-c", "import sys; print('something'); sys.exit(0)"}, opts, env.gopts) - testListSnapshots(t, env.gopts, 1) + snapshots := testListSnapshots(t, env.gopts, 1) + files := testRunLs(t, env.gopts, snapshots[0].String()) + rtest.Assert(t, includes(files, "/stdin/subdir/file"), "file %q missing from snapshot, got %v", "stdin/subdir/file", files) testRunCheck(t, env.gopts) } From 45e09dca2a773b7a68260172ac1507c308b5cc7b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 11 Apr 2025 22:19:33 +0200 Subject: [PATCH 9/9] add changelog for --stdin-filename with/directory --- changelog/unreleased/issue-5324 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 changelog/unreleased/issue-5324 diff --git a/changelog/unreleased/issue-5324 b/changelog/unreleased/issue-5324 new file mode 100644 index 000000000..0f5ff77e4 --- /dev/null +++ b/changelog/unreleased/issue-5324 @@ -0,0 +1,14 @@ +Bugfix: Correctly handle `backup --stdin-filename` with directories + +In restic 0.18.0, the `backup` command failed if a filename that includes +a least a directory was passed to `--stdin-filename`. For example, +`--stdin-filename /foo/bar` resulted in the following error: + +``` +Fatal: unable to save snapshot: open /foo: no such file or directory +``` + +This has been fixed now. + +https://github.com/restic/restic/issues/5324 +https://github.com/restic/restic/pull/5356