From b150dd02357707df59d6d14646cac7f2fbd45891 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sun, 16 Oct 2022 11:32:38 +0200 Subject: [PATCH 1/3] all: Replace some errors.Wrap calls by errors.WithStack Mostly changed the ones that repeat the name of a system call, which is already contained in os.PathError.Op. internal/fs.Reader had to be changed to actually return such errors. --- internal/archiver/archiver.go | 12 ++++---- internal/fs/fs_reader.go | 40 ++++++++++++++------------- internal/fs/fs_reader_test.go | 3 +- internal/repository/packer_manager.go | 4 +-- internal/restic/node.go | 16 +++++------ internal/restic/node_linux.go | 2 +- internal/restorer/restorer.go | 2 +- 7 files changed, 41 insertions(+), 38 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 37f1a378d..a56965d63 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -183,7 +183,7 @@ func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo) } // overwrite name to match that within the snapshot node.Name = path.Base(snPath) - return node, errors.Wrap(err, "NodeFromFileInfo") + return node, errors.WithStack(err) } // loadSubtree tries to load the subtree referenced by node. In case of an error, nil is returned. @@ -352,7 +352,7 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous debug.Log("lstat() for %v returned error: %v", target, err) err = arch.error(abstarget, err) if err != nil { - return FutureNode{}, false, errors.Wrap(err, "Lstat") + return FutureNode{}, false, errors.WithStack(err) } return FutureNode{}, true, nil } @@ -404,7 +404,7 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous debug.Log("Openfile() for %v returned error: %v", target, err) err = arch.error(abstarget, err) if err != nil { - return FutureNode{}, false, errors.Wrap(err, "Lstat") + return FutureNode{}, false, errors.WithStack(err) } return FutureNode{}, true, nil } @@ -415,7 +415,7 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous _ = file.Close() err = arch.error(abstarget, err) if err != nil { - return FutureNode{}, false, errors.Wrap(err, "Lstat") + return FutureNode{}, false, errors.WithStack(err) } return FutureNode{}, true, nil } @@ -524,7 +524,7 @@ func join(elem ...string) string { func (arch *Archiver) statDir(dir string) (os.FileInfo, error) { fi, err := arch.FS.Stat(dir) if err != nil { - return nil, errors.Wrap(err, "Lstat") + return nil, errors.WithStack(err) } tpe := fi.Mode() & (os.ModeType | os.ModeCharDevice) @@ -626,7 +626,7 @@ func (arch *Archiver) SaveTree(ctx context.Context, snPath string, atree *Tree, func readdirnames(filesystem fs.FS, dir string, flags int) ([]string, error) { f, err := filesystem.OpenFile(dir, fs.O_RDONLY|flags, 0) if err != nil { - return nil, errors.Wrap(err, "Open") + return nil, errors.WithStack(err) } entries, err := f.Readdirnames(-1) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index dac4aac9e..1551ad919 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -1,6 +1,7 @@ package fs import ( + "fmt" "io" "os" "path" @@ -47,7 +48,7 @@ func (fs *Reader) Open(name string) (f File, err error) { }) if f == nil { - return nil, syscall.EIO + return nil, pathError("open", name, syscall.EIO) } return f, nil @@ -58,7 +59,7 @@ func (fs *Reader) Open(name string) (f File, err error) { return f, nil } - return nil, syscall.ENOENT + return nil, pathError("open", name, syscall.ENOENT) } func (fs *Reader) fi() os.FileInfo { @@ -74,10 +75,11 @@ func (fs *Reader) fi() os.FileInfo { // 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. +// If there is an error, it will be of type *os.PathError. func (fs *Reader) OpenFile(name string, flag int, perm os.FileMode) (f File, err error) { if flag & ^(O_RDONLY|O_NOFOLLOW) != 0 { - return nil, errors.Errorf("invalid combination of flags 0x%x", flag) + return nil, pathError("open", name, + fmt.Errorf("invalid combination of flags 0x%x", flag)) } fs.open.Do(func() { @@ -85,14 +87,14 @@ func (fs *Reader) OpenFile(name string, flag int, perm os.FileMode) (f File, err }) if f == nil { - return nil, syscall.EIO + return nil, pathError("open", name, syscall.EIO) } return f, nil } // Stat returns a FileInfo describing the named file. If there is an error, it -// will be of type *PathError. +// will be of type *os.PathError. func (fs *Reader) Stat(name string) (os.FileInfo, error) { return fs.Lstat(name) } @@ -100,7 +102,7 @@ func (fs *Reader) Stat(name string) (os.FileInfo, error) { // 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 *PathError. +// If there is an error, it will be of type *os.PathError. func (fs *Reader) Lstat(name string) (os.FileInfo, error) { getDirInfo := func(name string) os.FileInfo { fi := fakeFileInfo{ @@ -130,7 +132,7 @@ func (fs *Reader) Lstat(name string) (os.FileInfo, error) { dir = fs.Dir(dir) } - return nil, os.ErrNotExist + return nil, pathError("lstat", name, os.ErrNotExist) } // Join joins any number of path elements into a single path, adding a @@ -207,11 +209,7 @@ func (r *readerFile) Read(p []byte) (int, error) { // return an error if we did not read any data if err == io.EOF && !r.AllowEmptyFile && !r.bytesRead { - return n, &os.PathError{ - Path: r.fakeFile.name, - Op: "read", - Err: ErrFileEmpty, - } + return n, pathError("read", r.fakeFile.name, ErrFileEmpty) } return n, err @@ -239,19 +237,19 @@ func (f fakeFile) Fd() uintptr { } func (f fakeFile) Readdirnames(n int) ([]string, error) { - return nil, os.ErrInvalid + return nil, pathError("readdirnames", f.name, os.ErrInvalid) } func (f fakeFile) Readdir(n int) ([]os.FileInfo, error) { - return nil, os.ErrInvalid + return nil, pathError("readdir", f.name, os.ErrInvalid) } func (f fakeFile) Seek(int64, int) (int64, error) { - return 0, os.ErrInvalid + return 0, pathError("seek", f.name, os.ErrInvalid) } func (f fakeFile) Read(p []byte) (int, error) { - return 0, os.ErrInvalid + return 0, pathError("read", f.name, os.ErrInvalid) } func (f fakeFile) Close() error { @@ -274,7 +272,7 @@ type fakeDir struct { func (d fakeDir) Readdirnames(n int) ([]string, error) { if n > 0 { - return nil, errors.New("not implemented") + return nil, pathError("readdirnames", d.name, errors.New("not implemented")) } names := make([]string, 0, len(d.entries)) for _, entry := range d.entries { @@ -286,7 +284,7 @@ func (d fakeDir) Readdirnames(n int) ([]string, error) { func (d fakeDir) Readdir(n int) ([]os.FileInfo, error) { if n > 0 { - return nil, errors.New("not implemented") + return nil, pathError("readdir", d.name, errors.New("not implemented")) } return d.entries, nil } @@ -322,3 +320,7 @@ func (fi fakeFileInfo) IsDir() bool { func (fi fakeFileInfo) Sys() interface{} { return nil } + +func pathError(op, name string, err error) *os.PathError { + return &os.PathError{Op: op, Path: name, Err: err} +} diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index a9fff7a98..d3ef5608a 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -2,6 +2,7 @@ package fs import ( "bytes" + "errors" "io" "os" "path" @@ -288,7 +289,7 @@ func TestFSReader(t *testing.T) { name: "dir/Lstat-error-not-exist", f: func(t *testing.T, fs FS) { _, err := fs.Lstat("other") - if err != os.ErrNotExist { + if !errors.Is(err, os.ErrNotExist) { t.Fatal(err) } }, diff --git a/internal/repository/packer_manager.go b/internal/repository/packer_manager.go index 7536f47a5..4422e3418 100644 --- a/internal/repository/packer_manager.go +++ b/internal/repository/packer_manager.go @@ -110,7 +110,7 @@ func (r *packerManager) newPacker() (packer *Packer, err error) { debug.Log("create new pack") tmpfile, err := fs.TempFile("", "restic-temp-pack-") if err != nil { - return nil, errors.Wrap(err, "fs.TempFile") + return nil, errors.WithStack(err) } bufWr := bufio.NewWriter(tmpfile) @@ -183,7 +183,7 @@ func (r *Repository) savePacker(ctx context.Context, t restic.BlobType, p *Packe if runtime.GOOS != "windows" { err = fs.RemoveIfExists(p.tmpfile.Name()) if err != nil { - return errors.Wrap(err, "Remove") + return errors.WithStack(err) } } diff --git a/internal/restic/node.go b/internal/restic/node.go index a240384be..a1aff18ac 100644 --- a/internal/restic/node.go +++ b/internal/restic/node.go @@ -191,14 +191,14 @@ func (node Node) restoreMetadata(path string) error { debug.Log("not running as root, ignoring lchown permission error for %v: %v", path, err) } else { - firsterr = errors.Wrap(err, "Lchown") + firsterr = errors.WithStack(err) } } if node.Type != "symlink" { if err := fs.Chmod(path, node.Mode); err != nil { if firsterr != nil { - firsterr = errors.Wrap(err, "Chmod") + firsterr = errors.WithStack(err) } } } @@ -250,7 +250,7 @@ func (node Node) RestoreTimestamps(path string) error { func (node Node) createDirAt(path string) error { err := fs.Mkdir(path, node.Mode) if err != nil && !os.IsExist(err) { - return errors.Wrap(err, "Mkdir") + return errors.WithStack(err) } return nil @@ -259,7 +259,7 @@ func (node Node) createDirAt(path string) error { func (node Node) createFileAt(ctx context.Context, path string, repo Repository) error { f, err := fs.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) if err != nil { - return errors.Wrap(err, "OpenFile") + return errors.WithStack(err) } err = node.writeNodeContent(ctx, repo, f) @@ -270,7 +270,7 @@ func (node Node) createFileAt(ctx context.Context, path string, repo Repository) } if closeErr != nil { - return errors.Wrap(closeErr, "Close") + return errors.WithStack(closeErr) } return nil @@ -286,7 +286,7 @@ func (node Node) writeNodeContent(ctx context.Context, repo Repository, f *os.Fi _, err = f.Write(buf) if err != nil { - return errors.Wrap(err, "Write") + return errors.WithStack(err) } } @@ -300,7 +300,7 @@ func (node Node) createSymlinkAt(path string) error { } if err := fs.Symlink(node.LinkTarget, path); err != nil { - return errors.Wrap(err, "Symlink") + return errors.WithStack(err) } return nil @@ -591,7 +591,7 @@ func (node *Node) fillExtra(path string, fi os.FileInfo) error { node.LinkTarget, err = fs.Readlink(path) node.Links = uint64(stat.nlink()) if err != nil { - return errors.Wrap(err, "Readlink") + return errors.WithStack(err) } case "dev": node.Device = uint64(stat.rdev()) diff --git a/internal/restic/node_linux.go b/internal/restic/node_linux.go index 2eb80db90..85a363830 100644 --- a/internal/restic/node_linux.go +++ b/internal/restic/node_linux.go @@ -13,7 +13,7 @@ import ( func (node Node) restoreSymlinkTimestamps(path string, utimes [2]syscall.Timespec) error { dir, err := fs.Open(filepath.Dir(path)) if err != nil { - return errors.Wrap(err, "Open") + return errors.WithStack(err) } times := []unix.Timespec{ diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 2f2e71e3d..4dfe3c3a8 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -184,7 +184,7 @@ func (res *Restorer) restoreHardlinkAt(node *restic.Node, target, path, location } err := fs.Link(target, path) if err != nil { - return errors.Wrap(err, "CreateHardlink") + return errors.WithStack(err) } // TODO investigate if hardlinks have separate metadata on any supported system return res.restoreNodeMetadataTo(node, path, location) From d9002f050eb5034cbe69df5d7bff067a2ec9e8a1 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sun, 27 Nov 2022 20:03:22 +0100 Subject: [PATCH 2/3] backend: Don't Wrap errors from url.Parse The messages from url.Error.Error already start with the word "parse". --- internal/backend/rest/config.go | 3 +-- internal/backend/s3/config.go | 2 +- internal/backend/sftp/config.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/backend/rest/config.go b/internal/backend/rest/config.go index 51ff3b27c..388153fce 100644 --- a/internal/backend/rest/config.go +++ b/internal/backend/rest/config.go @@ -34,9 +34,8 @@ func ParseConfig(s string) (interface{}, error) { s = prepareURL(s) u, err := url.Parse(s) - if err != nil { - return nil, errors.Wrap(err, "url.Parse") + return nil, errors.WithStack(err) } cfg := NewConfig() diff --git a/internal/backend/s3/config.go b/internal/backend/s3/config.go index 775580450..9050e20f4 100644 --- a/internal/backend/s3/config.go +++ b/internal/backend/s3/config.go @@ -52,7 +52,7 @@ func ParseConfig(s string) (interface{}, error) { // bucket name and prefix url, err := url.Parse(s[3:]) if err != nil { - return nil, errors.Wrap(err, "url.Parse") + return nil, errors.WithStack(err) } if url.Path == "" { diff --git a/internal/backend/sftp/config.go b/internal/backend/sftp/config.go index 2f3b3697c..76d6d145d 100644 --- a/internal/backend/sftp/config.go +++ b/internal/backend/sftp/config.go @@ -42,7 +42,7 @@ func ParseConfig(s string) (interface{}, error) { // parse the "sftp://user@host/path" url format url, err := url.Parse(s) if err != nil { - return nil, errors.Wrap(err, "url.Parse") + return nil, errors.WithStack(err) } if url.User != nil { user = url.User.Username() From 1678392a6d7f921a175972f59e209d460f6f0216 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Wed, 23 Nov 2022 21:12:06 +0100 Subject: [PATCH 3/3] checker: Make ErrLegacyLayout a value, not a type --- cmd/restic/cmd_check.go | 2 +- internal/checker/checker.go | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/cmd/restic/cmd_check.go b/cmd/restic/cmd_check.go index 09f308564..68198377f 100644 --- a/cmd/restic/cmd_check.go +++ b/cmd/restic/cmd_check.go @@ -268,7 +268,7 @@ func runCheck(ctx context.Context, opts CheckOptions, gopts GlobalOptions, args if checker.IsOrphanedPack(err) { orphanedPacks++ Verbosef("%v\n", err) - } else if _, ok := err.(*checker.ErrLegacyLayout); ok { + } else if err == checker.ErrLegacyLayout { Verbosef("repository still uses the S3 legacy layout\nPlease run `restic migrate s3legacy` to correct this.\n") } else { errorsFound = true diff --git a/internal/checker/checker.go b/internal/checker/checker.go index 54e2f76ba..b2512969e 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -59,11 +59,7 @@ func New(repo restic.Repository, trackUnused bool) *Checker { } // ErrLegacyLayout is returned when the repository uses the S3 legacy layout. -type ErrLegacyLayout struct{} - -func (e *ErrLegacyLayout) Error() string { - return "repository uses S3 legacy layout" -} +var ErrLegacyLayout = errors.New("repository uses S3 legacy layout") // ErrDuplicatePacks is returned when a pack is found in more than one index. type ErrDuplicatePacks struct { @@ -231,7 +227,7 @@ func (c *Checker) Packs(ctx context.Context, errChan chan<- error) { defer close(errChan) if isS3Legacy(c.repo.Backend()) { - errChan <- &ErrLegacyLayout{} + errChan <- ErrLegacyLayout } debug.Log("checking for %d packs", len(c.packs))