From 57627a307f438969d0ae9eae42fd68593ee8d3ae Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:03:07 +0100 Subject: [PATCH 01/14] Add config for golangci-lint --- .golangci.yml | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..55ad1a266 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,57 @@ +# This is the configuration for golangci-lint for the restic project. +# +# A sample config with all settings is here: +# https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml + +linters: + # only enable the linters listed below + disable-all: true + enable: + # make sure all errors returned by functions are handled + - errcheck + + # find unused code + - deadcode + + # show how code can be simplified + - gosimple + + # # make sure code is formatted + - gofmt + + # examine code and report suspicious constructs, such as Printf calls whose + # arguments do not align with the format string + - govet + + # make sure names and comments are used according to the conventions + - golint + + # detect when assignments to existing variables are not used + - ineffassign + + # run static analysis and find errors + - staticcheck + + # find unused variables, functions, structs, types, etc. + - unused + + # find unused struct fields + - structcheck + + # find unused global variables + - varcheck + + # parse and typecheck code + - typecheck + +issues: + # don't use the default exclude rules, this hides (among others) ignored + # errors from Close() calls + exclude-use-default: false + + # list of things to not warn about + exclude: + # golint: do not warn about missing comments for exported stuff + - exported (function|method|var|type|const) `.*` should have comment or be unexported + # golint: ignore constants in all caps + - don't use ALL_CAPS in Go names; use CamelCase From b3d5bf7c9964643a01f679817b36e2a61b2b3fd6 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:03:44 +0100 Subject: [PATCH 02/14] Update golangci-lint version --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 537e1c5c5..85b8f202b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -241,7 +241,7 @@ jobs: uses: golangci/golangci-lint-action@v2 with: # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. - version: v1.29 + version: v1.36 # Optional: show only new issues if it's a pull request. The default value is `false`. only-new-issues: true args: --verbose --timeout 5m From 1632a84e7bf441bb36a7fe4c3ee92990c302fe26 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:19:27 +0100 Subject: [PATCH 03/14] Add a section explaining golangci-lint --- CONTRIBUTING.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c63dc0d72..903e4459f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -141,6 +141,14 @@ Installing the script `fmt-check` from https://github.com/edsrzf/gofmt-git-hook locally as a pre-commit hook checks formatting before committing automatically, just copy this script to `.git/hooks/pre-commit`. +The project is using the program +[`golangci-lint`](https://github.com/golangci/golangci-lint) to run a list of +linters and checkers. It will be run on the code when you submit a PR. In order +to check your code beforehand, you can run `golangci-lint run` manually. +Eventually, we will enable `golangci-lint` for the whole code base. For now, +you can ignore warnings printed for lines you did not modify, those will be +ignored by the CI. + For each pull request, several different systems run the integration tests on Linux, macOS and Windows. We won't merge any code that does not pass all tests for all systems, so when a tests fails, try to find out what's wrong and fix From 75f53955ee3ffe3c5199dee35810ce35c96b8806 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:32:00 +0100 Subject: [PATCH 04/14] errcheck: Add error checks Most added checks are straight forward. --- internal/backend/foreground_test.go | 4 ++- internal/fuse/snapshots_dir.go | 45 +++++++++++++++++++----- internal/hashing/writer.go | 10 +++++- internal/repository/index_test.go | 6 +++- internal/repository/master_index_test.go | 5 ++- internal/repository/repack_test.go | 6 +++- internal/repository/repository.go | 11 +++--- 7 files changed, 69 insertions(+), 18 deletions(-) diff --git a/internal/backend/foreground_test.go b/internal/backend/foreground_test.go index 34c55d1f3..81adefe32 100644 --- a/internal/backend/foreground_test.go +++ b/internal/backend/foreground_test.go @@ -23,7 +23,9 @@ func TestForeground(t *testing.T) { bg, err := backend.StartForeground(cmd) rtest.OK(t, err) - defer cmd.Wait() + defer func() { + rtest.OK(t, cmd.Wait()) + }() err = bg() rtest.OK(t, err) diff --git a/internal/fuse/snapshots_dir.go b/internal/fuse/snapshots_dir.go index bdbdbe60b..34484b597 100644 --- a/internal/fuse/snapshots_dir.go +++ b/internal/fuse/snapshots_dir.go @@ -229,7 +229,10 @@ func updateSnapshots(ctx context.Context, root *Root) error { if root.snCount != len(snapshots) { root.snCount = len(snapshots) - root.repo.LoadIndex(ctx) + err := root.repo.LoadIndex(ctx) + if err != nil { + return err + } root.snapshots = snapshots } root.lastCheck = time.Now() @@ -272,7 +275,10 @@ func (d *SnapshotsDir) ReadDirAll(ctx context.Context) ([]fuse.Dirent, error) { debug.Log("ReadDirAll()") // update snapshots - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update snapshot names updateSnapshotNames(d, d.root.cfg.SnapshotTemplate) @@ -314,7 +320,10 @@ func (d *SnapshotsIDSDir) ReadDirAll(ctx context.Context) ([]fuse.Dirent, error) debug.Log("ReadDirAll()") // update snapshots - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update snapshot ids updateSnapshotIDSNames(d) @@ -348,7 +357,10 @@ func (d *HostsDir) ReadDirAll(ctx context.Context) ([]fuse.Dirent, error) { debug.Log("ReadDirAll()") // update snapshots - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update host names updateHostsNames(d) @@ -382,7 +394,10 @@ func (d *TagsDir) ReadDirAll(ctx context.Context) ([]fuse.Dirent, error) { debug.Log("ReadDirAll()") // update snapshots - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update tag names updateTagNames(d) @@ -443,7 +458,10 @@ func (d *SnapshotsDir) Lookup(ctx context.Context, name string) (fs.Node, error) sn, ok := d.names[name] if !ok { // could not find entry. Updating repository-state - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update snapshot names updateSnapshotNames(d, d.root.cfg.SnapshotTemplate) @@ -476,7 +494,10 @@ func (d *SnapshotsIDSDir) Lookup(ctx context.Context, name string) (fs.Node, err sn, ok := d.names[name] if !ok { // could not find entry. Updating repository-state - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update snapshot ids updateSnapshotIDSNames(d) @@ -499,7 +520,10 @@ func (d *HostsDir) Lookup(ctx context.Context, name string) (fs.Node, error) { _, ok := d.hosts[name] if !ok { // could not find entry. Updating repository-state - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update host names updateHostsNames(d) @@ -522,7 +546,10 @@ func (d *TagsDir) Lookup(ctx context.Context, name string) (fs.Node, error) { _, ok := d.tags[name] if !ok { // could not find entry. Updating repository-state - updateSnapshots(ctx, d.root) + err := updateSnapshots(ctx, d.root) + if err != nil { + return nil, err + } // update tag names updateTagNames(d) diff --git a/internal/hashing/writer.go b/internal/hashing/writer.go index 8eb157a9f..0b2d8c5b2 100644 --- a/internal/hashing/writer.go +++ b/internal/hashing/writer.go @@ -21,8 +21,16 @@ func NewWriter(w io.Writer, h hash.Hash) *Writer { // Write wraps the write method of the underlying writer and also hashes all data. func (h *Writer) Write(p []byte) (int, error) { + // write the data to the underlying writing n, err := h.w.Write(p) - h.h.Write(p[:n]) + + // according to the interface documentation, Write() on a hash.Hash never + // returns an error. + _, hashErr := h.h.Write(p[:n]) + if hashErr != nil { + panic(hashErr) + } + return n, err } diff --git a/internal/repository/index_test.go b/internal/repository/index_test.go index 1084558a6..c4f0179db 100644 --- a/internal/repository/index_test.go +++ b/internal/repository/index_test.go @@ -301,7 +301,11 @@ var ( func initBenchmarkIndexJSON() { idx, _ := createRandomIndex(rand.New(rand.NewSource(0)), 200000) var buf bytes.Buffer - idx.Encode(&buf) + err := idx.Encode(&buf) + if err != nil { + panic(err) + } + benchmarkIndexJSON = buf.Bytes() } diff --git a/internal/repository/master_index_test.go b/internal/repository/master_index_test.go index 3c279696e..167cad2f8 100644 --- a/internal/repository/master_index_test.go +++ b/internal/repository/master_index_test.go @@ -338,7 +338,10 @@ func TestIndexSave(t *testing.T) { repo, cleanup := createFilledRepo(t, 3, 0) defer cleanup() - repo.LoadIndex(context.TODO()) + err := repo.LoadIndex(context.TODO()) + if err != nil { + t.Fatal(err) + } obsoletes, err := repo.Index().(*repository.MasterIndex).Save(context.TODO(), repo, nil, nil, nil) if err != nil { diff --git a/internal/repository/repack_test.go b/internal/repository/repack_test.go index a4505d4d2..108c167d9 100644 --- a/internal/repository/repack_test.go +++ b/internal/repository/repack_test.go @@ -201,7 +201,11 @@ func rebuildIndex(t *testing.T, repo restic.Repository) { } func reloadIndex(t *testing.T, repo restic.Repository) { - repo.SetIndex(repository.NewMasterIndex()) + err := repo.SetIndex(repository.NewMasterIndex()) + if err != nil { + t.Fatal(err) + } + if err := repo.LoadIndex(context.TODO()); err != nil { t.Fatalf("error loading new index: %v", err) } diff --git a/internal/repository/repository.go b/internal/repository/repository.go index a0d27cc3f..298800008 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -781,16 +781,19 @@ func DownloadAndHash(ctx context.Context, be Loader, h restic.Handle) (tmpfile * hash = restic.IDFromHash(hrd.Sum(nil)) return ierr }) + if err != nil { - tmpfile.Close() - os.Remove(tmpfile.Name()) + // ignore subsequent errors + _ = tmpfile.Close() + _ = os.Remove(tmpfile.Name()) return nil, restic.ID{}, -1, errors.Wrap(err, "Load") } _, err = tmpfile.Seek(0, io.SeekStart) if err != nil { - tmpfile.Close() - os.Remove(tmpfile.Name()) + // ignore subsequent errors + _ = tmpfile.Close() + _ = os.Remove(tmpfile.Name()) return nil, restic.ID{}, -1, errors.Wrap(err, "Seek") } From 16313bfcc9c30babad30144e94eeba653dfe69ab Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:35:05 +0100 Subject: [PATCH 05/14] errcheck: Add error check for MergeFinalIndexes() --- internal/checker/checker.go | 6 ++++- internal/repository/master_index.go | 10 +++++-- internal/repository/master_index_test.go | 33 ++++++++++++++++-------- internal/repository/repository.go | 11 ++++++-- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/internal/checker/checker.go b/internal/checker/checker.go index 38b3c75c4..537a44d12 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -111,7 +111,11 @@ func (c *Checker) LoadIndex(ctx context.Context) (hints []error, errs []error) { } // Merge index before computing pack sizes, as this needs removed duplicates - c.masterIndex.MergeFinalIndexes() + err = c.masterIndex.MergeFinalIndexes() + if err != nil { + // abort if an error occurs merging the indexes + return hints, append(errs, err) + } // compute pack size using index entries c.packs = c.masterIndex.PackSize(ctx, false) diff --git a/internal/repository/master_index.go b/internal/repository/master_index.go index f24589e3d..5aa0ceccb 100644 --- a/internal/repository/master_index.go +++ b/internal/repository/master_index.go @@ -2,6 +2,7 @@ package repository import ( "context" + "fmt" "sync" "github.com/restic/restic/internal/debug" @@ -271,7 +272,7 @@ func (mi *MasterIndex) Each(ctx context.Context) <-chan restic.PackedBlob { // Indexes that are not final are left untouched. // This merging can only be called after all index files are loaded - as // removing of superseded index contents is only possible for unmerged indexes. -func (mi *MasterIndex) MergeFinalIndexes() { +func (mi *MasterIndex) MergeFinalIndexes() error { mi.idxMutex.Lock() defer mi.idxMutex.Unlock() @@ -284,10 +285,15 @@ func (mi *MasterIndex) MergeFinalIndexes() { if !idx.Final() { newIdx = append(newIdx, idx) } else { - mi.idx[0].merge(idx) + err := mi.idx[0].merge(idx) + if err != nil { + return fmt.Errorf("MergeFinalIndexes: %w", err) + } } } mi.idx = newIdx + + return nil } const saveIndexParallelism = 4 diff --git a/internal/repository/master_index_test.go b/internal/repository/master_index_test.go index 167cad2f8..f1fe9af7e 100644 --- a/internal/repository/master_index_test.go +++ b/internal/repository/master_index_test.go @@ -163,7 +163,11 @@ func TestMasterMergeFinalIndexes(t *testing.T) { finalIndexes := mIdx.FinalizeNotFinalIndexes() rtest.Equals(t, []*repository.Index{idx1, idx2}, finalIndexes) - mIdx.MergeFinalIndexes() + err := mIdx.MergeFinalIndexes() + if err != nil { + t.Fatal(err) + } + allIndexes := mIdx.All() rtest.Equals(t, 1, len(allIndexes)) @@ -191,7 +195,11 @@ func TestMasterMergeFinalIndexes(t *testing.T) { finalIndexes = mIdx.FinalizeNotFinalIndexes() rtest.Equals(t, []*repository.Index{idx3}, finalIndexes) - mIdx.MergeFinalIndexes() + err = mIdx.MergeFinalIndexes() + if err != nil { + t.Fatal(err) + } + allIndexes = mIdx.All() rtest.Equals(t, 1, len(allIndexes)) @@ -209,7 +217,7 @@ func TestMasterMergeFinalIndexes(t *testing.T) { rtest.Equals(t, 2, blobCount) } -func createRandomMasterIndex(rng *rand.Rand, num, size int) (*repository.MasterIndex, restic.BlobHandle) { +func createRandomMasterIndex(t testing.TB, rng *rand.Rand, num, size int) (*repository.MasterIndex, restic.BlobHandle) { mIdx := repository.NewMasterIndex() for i := 0; i < num-1; i++ { idx, _ := createRandomIndex(rng, size) @@ -219,7 +227,10 @@ func createRandomMasterIndex(rng *rand.Rand, num, size int) (*repository.MasterI mIdx.Insert(idx1) mIdx.FinalizeNotFinalIndexes() - mIdx.MergeFinalIndexes() + err := mIdx.MergeFinalIndexes() + if err != nil { + t.Fatal(err) + } return mIdx, lookupBh } @@ -229,12 +240,12 @@ func BenchmarkMasterIndexAlloc(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - createRandomMasterIndex(rng, 10000, 5) + createRandomMasterIndex(b, rng, 10000, 5) } } func BenchmarkMasterIndexLookupSingleIndex(b *testing.B) { - mIdx, lookupBh := createRandomMasterIndex(rand.New(rand.NewSource(0)), 1, 200000) + mIdx, lookupBh := createRandomMasterIndex(b, rand.New(rand.NewSource(0)), 1, 200000) b.ResetTimer() @@ -244,7 +255,7 @@ func BenchmarkMasterIndexLookupSingleIndex(b *testing.B) { } func BenchmarkMasterIndexLookupMultipleIndex(b *testing.B) { - mIdx, lookupBh := createRandomMasterIndex(rand.New(rand.NewSource(0)), 100, 10000) + mIdx, lookupBh := createRandomMasterIndex(b, rand.New(rand.NewSource(0)), 100, 10000) b.ResetTimer() @@ -256,7 +267,7 @@ func BenchmarkMasterIndexLookupMultipleIndex(b *testing.B) { func BenchmarkMasterIndexLookupSingleIndexUnknown(b *testing.B) { lookupBh := restic.NewRandomBlobHandle() - mIdx, _ := createRandomMasterIndex(rand.New(rand.NewSource(0)), 1, 200000) + mIdx, _ := createRandomMasterIndex(b, rand.New(rand.NewSource(0)), 1, 200000) b.ResetTimer() @@ -267,7 +278,7 @@ func BenchmarkMasterIndexLookupSingleIndexUnknown(b *testing.B) { func BenchmarkMasterIndexLookupMultipleIndexUnknown(b *testing.B) { lookupBh := restic.NewRandomBlobHandle() - mIdx, _ := createRandomMasterIndex(rand.New(rand.NewSource(0)), 100, 10000) + mIdx, _ := createRandomMasterIndex(b, rand.New(rand.NewSource(0)), 100, 10000) b.ResetTimer() @@ -284,7 +295,7 @@ func BenchmarkMasterIndexLookupParallel(b *testing.B) { b.StopTimer() rng := rand.New(rand.NewSource(0)) - mIdx, lookupBh = createRandomMasterIndex(rng, numindices, 10000) + mIdx, lookupBh = createRandomMasterIndex(b, rng, numindices, 10000) b.StartTimer() name := fmt.Sprintf("known,indices=%d", numindices) @@ -310,7 +321,7 @@ func BenchmarkMasterIndexLookupParallel(b *testing.B) { func BenchmarkMasterIndexLookupBlobSize(b *testing.B) { rng := rand.New(rand.NewSource(0)) - mIdx, lookupBh := createRandomMasterIndex(rand.New(rng), 5, 200000) + mIdx, lookupBh := createRandomMasterIndex(b, rand.New(rng), 5, 200000) b.ResetTimer() diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 298800008..c21bd520c 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -419,7 +419,11 @@ func (r *Repository) saveIndex(ctx context.Context, indexes ...*Index) error { debug.Log("Saved index %d as %v", i, sid) } - r.idx.MergeFinalIndexes() + + err := r.idx.MergeFinalIndexes() + if err != nil { + return err + } return nil } @@ -461,7 +465,10 @@ func (r *Repository) LoadIndex(ctx context.Context) error { return errors.Fatal(err.Error()) } - r.idx.MergeFinalIndexes() + err = r.idx.MergeFinalIndexes() + if err != nil { + return err + } // remove index files from the cache which have been removed in the repo return r.PrepareCache(validIndex) From 3c753c071c2a3d2a81d65834957c8e9cd654278c Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:46:34 +0100 Subject: [PATCH 06/14] errcheck: More error handling --- internal/backend/rclone/backend.go | 18 +++++++++++------- internal/backend/rclone/internal_test.go | 4 +++- internal/checker/checker.go | 7 ++++++- internal/ui/json/backup.go | 5 ++++- internal/walker/walker_test.go | 10 ++++++++-- 5 files changed, 32 insertions(+), 12 deletions(-) diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go index 2f730f238..25169fd58 100644 --- a/internal/backend/rclone/backend.go +++ b/internal/backend/rclone/backend.go @@ -63,9 +63,9 @@ func run(command string, args ...string) (*StdioConn, *exec.Cmd, *sync.WaitGroup stdout, w, err := os.Pipe() if err != nil { - // close first pipe - r.Close() - stdin.Close() + // close first pipe and ignore subsequent errors + _ = r.Close() + _ = stdin.Close() return nil, nil, nil, nil, err } @@ -197,8 +197,8 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) { err := cmd.Wait() debug.Log("Wait returned %v", err) be.waitResult = err - // close our side of the pipes to rclone - stdioConn.CloseAll() + // close our side of the pipes to rclone, ignore errors + _ = stdioConn.CloseAll() close(waitCh) }() @@ -237,13 +237,17 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) { res, err := ctxhttp.Do(ctx, client, req) if err != nil { - bg() + // ignore subsequent errors + _ = bg() _ = cmd.Process.Kill() return nil, errors.Errorf("error talking HTTP to rclone: %v", err) } debug.Log("HTTP status %q returned, moving instance to background", res.Status) - bg() + err = bg() + if err != nil { + return nil, fmt.Errorf("error moving process to background: %w", err) + } return be, nil } diff --git a/internal/backend/rclone/internal_test.go b/internal/backend/rclone/internal_test.go index b4bd01996..a192a37bf 100644 --- a/internal/backend/rclone/internal_test.go +++ b/internal/backend/rclone/internal_test.go @@ -23,7 +23,9 @@ func TestRcloneExit(t *testing.T) { return } rtest.OK(t, err) - defer be.Close() + defer func() { + rtest.OK(t, be.Close()) + }() err = be.cmd.Process.Kill() rtest.OK(t, err) diff --git a/internal/checker/checker.go b/internal/checker/checker.go index 537a44d12..e842a08be 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -327,7 +327,12 @@ func (c *Checker) Structure(ctx context.Context, p *progress.Counter, errChan ch }) } - wg.Wait() + // the wait group should not return an error because no worker returns an + // error, so panic if that has changed somehow. + err := wg.Wait() + if err != nil { + panic(err) + } } func (c *Checker) checkTree(id restic.ID, tree *restic.Tree) (errs []error) { diff --git a/internal/ui/json/backup.go b/internal/ui/json/backup.go index d9be31b24..b6a2ed03d 100644 --- a/internal/ui/json/backup.go +++ b/internal/ui/json/backup.go @@ -79,7 +79,10 @@ func NewBackup(term *termstatus.Terminal, verbosity uint) *Backup { func toJSONString(status interface{}) string { buf := new(bytes.Buffer) - json.NewEncoder(buf).Encode(status) + err := json.NewEncoder(buf).Encode(status) + if err != nil { + panic(err) + } return buf.String() } diff --git a/internal/walker/walker_test.go b/internal/walker/walker_test.go index 0c8086956..a4e820940 100644 --- a/internal/walker/walker_test.go +++ b/internal/walker/walker_test.go @@ -28,17 +28,23 @@ func buildTreeMap(tree TestTree, m TreeMap) restic.ID { for name, item := range tree { switch elem := item.(type) { case TestFile: - res.Insert(&restic.Node{ + err := res.Insert(&restic.Node{ Name: name, Type: "file", }) + if err != nil { + panic(err) + } case TestTree: id := buildTreeMap(elem, m) - res.Insert(&restic.Node{ + err := res.Insert(&restic.Node{ Name: name, Subtree: &id, Type: "dir", }) + if err != nil { + panic(err) + } default: panic(fmt.Sprintf("invalid type %T", elem)) } From 1a0eb05bfaaaa3c4cc7c8a293cca6b17ee4d5ea3 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:48:09 +0100 Subject: [PATCH 07/14] errcheck: Add more error checks --- cmd/restic/cmd_backup.go | 6 +++++- cmd/restic/cmd_forget.go | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index d8500b263..e769fef1f 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -119,7 +119,11 @@ func init() { f.StringVarP(&backupOptions.Host, "host", "H", "", "set the `hostname` for the snapshot manually. To prevent an expensive rescan use the \"parent\" flag") f.StringVar(&backupOptions.Host, "hostname", "", "set the `hostname` for the snapshot manually") - f.MarkDeprecated("hostname", "use --host") + err := f.MarkDeprecated("hostname", "use --host") + if err != nil { + // MarkDeprecated only returns an error when the flag could not be found + panic(err) + } f.StringArrayVar(&backupOptions.FilesFrom, "files-from", nil, "read the files to backup from `file` (can be combined with file args; can be specified multiple times)") f.StringArrayVar(&backupOptions.FilesFromVerbatim, "files-from-verbatim", nil, "read the files to backup from `file` (can be combined with file args; can be specified multiple times)") diff --git a/cmd/restic/cmd_forget.go b/cmd/restic/cmd_forget.go index ec90879ec..83b0ac91d 100644 --- a/cmd/restic/cmd_forget.go +++ b/cmd/restic/cmd_forget.go @@ -68,7 +68,11 @@ func init() { f.Var(&forgetOptions.KeepTags, "keep-tag", "keep snapshots with this `taglist` (can be specified multiple times)") f.StringArrayVar(&forgetOptions.Hosts, "host", nil, "only consider snapshots with the given `host` (can be specified multiple times)") f.StringArrayVar(&forgetOptions.Hosts, "hostname", nil, "only consider snapshots with the given `hostname` (can be specified multiple times)") - f.MarkDeprecated("hostname", "use --host") + err := f.MarkDeprecated("hostname", "use --host") + if err != nil { + // MarkDeprecated only returns an error when the flag is not found + panic(err) + } f.Var(&forgetOptions.Tags, "tag", "only consider snapshots which include this `taglist` in the format `tag[,tag,...]` (can be specified multiple times)") From cbd88c457a97d6383dd82099dc20cb06c50dca95 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 16:49:15 +0100 Subject: [PATCH 08/14] backup: Improve error handling --- cmd/restic/cmd_backup.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index e769fef1f..bd8b4da8c 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -205,10 +205,21 @@ func readFilenamesFromFileRaw(filename string) (names []string, err error) { if f, err = os.Open(filename); err != nil { return nil, err } - defer f.Close() } - return readFilenamesRaw(f) + names, err = readFilenamesRaw(f) + if err != nil { + // ignore subsequent errors + _ = f.Close() + return nil, err + } + + err = f.Close() + if err != nil { + return nil, err + } + + return names, nil } func readFilenamesRaw(r io.Reader) (names []string, err error) { From 200f09522dfe034ced28075705c41de9a5fefebf Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 17:25:10 +0100 Subject: [PATCH 09/14] Add more error checks --- cmd/restic/cmd_backup_test.go | 8 ++++---- cmd/restic/cmd_cache.go | 2 +- cmd/restic/cmd_find.go | 5 ++++- cmd/restic/cmd_ls.go | 6 ++++-- cmd/restic/cmd_recover.go | 5 ++++- cmd/restic/cmd_snapshots.go | 5 ++++- cmd/restic/cmd_snapshots_test.go | 3 ++- cmd/restic/delete.go | 2 +- cmd/restic/exclude.go | 4 +++- cmd/restic/integration_test.go | 30 +++++++++++++++++++++++------- internal/archiver/archiver.go | 17 +++++++++++++---- 11 files changed, 63 insertions(+), 24 deletions(-) diff --git a/cmd/restic/cmd_backup_test.go b/cmd/restic/cmd_backup_test.go index 90e9a5a4b..49642b872 100644 --- a/cmd/restic/cmd_backup_test.go +++ b/cmd/restic/cmd_backup_test.go @@ -32,7 +32,7 @@ func TestCollectTargets(t *testing.T) { // All mentioned files must exist for collectTargets. f, err := os.Create(filepath.Join(dir, filename)) rtest.OK(t, err) - f.Close() + rtest.OK(t, f.Close()) expect = append(expect, f.Name()) } @@ -41,7 +41,7 @@ func TestCollectTargets(t *testing.T) { rtest.OK(t, err) // Empty lines should be ignored. A line starting with '#' is a comment. fmt.Fprintf(f1, "\n%s*\n # here's a comment\n", f1.Name()) - f1.Close() + rtest.OK(t, f1.Close()) f2, err := os.Create(filepath.Join(dir, "fromfile-verbatim")) rtest.OK(t, err) @@ -49,7 +49,7 @@ func TestCollectTargets(t *testing.T) { // Empty lines should be ignored. CR+LF is allowed. fmt.Fprintf(f2, "%s\r\n\n", filepath.Join(dir, filename)) } - f2.Close() + rtest.OK(t, f2.Close()) f3, err := os.Create(filepath.Join(dir, "fromfile-raw")) rtest.OK(t, err) @@ -57,7 +57,7 @@ func TestCollectTargets(t *testing.T) { fmt.Fprintf(f3, "%s\x00", filepath.Join(dir, filename)) } rtest.OK(t, err) - f3.Close() + rtest.OK(t, f3.Close()) opts := BackupOptions{ FilesFrom: []string{f1.Name()}, diff --git a/cmd/restic/cmd_cache.go b/cmd/restic/cmd_cache.go index 89fb2c0df..1b36219eb 100644 --- a/cmd/restic/cmd_cache.go +++ b/cmd/restic/cmd_cache.go @@ -148,7 +148,7 @@ func runCache(opts CacheOptions, gopts GlobalOptions, args []string) error { }) } - tab.Write(gopts.stdout) + _ = tab.Write(gopts.stdout) Printf("%d cache dirs in %s\n", len(dirs), cachedir) return nil diff --git a/cmd/restic/cmd_find.go b/cmd/restic/cmd_find.go index 0986b5a13..378d6cbdc 100644 --- a/cmd/restic/cmd_find.go +++ b/cmd/restic/cmd_find.go @@ -563,7 +563,10 @@ func runFind(opts FindOptions, gopts GlobalOptions, args []string) error { } if opts.PackID { - f.packsToBlobs(ctx, []string{f.pat.pattern[0]}) // TODO: support multiple packs + err := f.packsToBlobs(ctx, []string{f.pat.pattern[0]}) // TODO: support multiple packs + if err != nil { + return err + } } for sn := range FindFilteredSnapshots(ctx, repo, opts.Hosts, opts.Tags, opts.Paths, opts.Snapshots) { diff --git a/cmd/restic/cmd_ls.go b/cmd/restic/cmd_ls.go index db908a72c..90106e8a5 100644 --- a/cmd/restic/cmd_ls.go +++ b/cmd/restic/cmd_ls.go @@ -159,16 +159,17 @@ func runLs(opts LsOptions, gopts GlobalOptions, args []string) error { enc := json.NewEncoder(gopts.stdout) printSnapshot = func(sn *restic.Snapshot) { - enc.Encode(lsSnapshot{ + err = enc.Encode(lsSnapshot{ Snapshot: sn, ID: sn.ID(), ShortID: sn.ID().Str(), StructType: "snapshot", }) + panic(err) } printNode = func(path string, node *restic.Node) { - enc.Encode(lsNode{ + err = enc.Encode(lsNode{ Name: node.Name, Type: node.Type, Path: path, @@ -181,6 +182,7 @@ func runLs(opts LsOptions, gopts GlobalOptions, args []string) error { ChangeTime: node.ChangeTime, StructType: "node", }) + panic(err) } } else { printSnapshot = func(sn *restic.Snapshot) { diff --git a/cmd/restic/cmd_recover.go b/cmd/restic/cmd_recover.go index 8f11b6860..c6b111d22 100644 --- a/cmd/restic/cmd_recover.go +++ b/cmd/restic/cmd_recover.go @@ -117,7 +117,10 @@ func runRecover(gopts GlobalOptions) error { ModTime: time.Now(), ChangeTime: time.Now(), } - tree.Insert(&node) + err = tree.Insert(&node) + if err != nil { + return err + } } treeID, err := repo.SaveTree(gopts.ctx, tree) diff --git a/cmd/restic/cmd_snapshots.go b/cmd/restic/cmd_snapshots.go index 300ba99ee..091861bfa 100644 --- a/cmd/restic/cmd_snapshots.go +++ b/cmd/restic/cmd_snapshots.go @@ -243,7 +243,10 @@ func PrintSnapshots(stdout io.Writer, list restic.Snapshots, reasons []restic.Ke } } - tab.Write(stdout) + err := tab.Write(stdout) + if err != nil { + Warnf("error printing: %v\n", err) + } } // PrintSnapshotGroupHeader prints which group of the group-by option the diff --git a/cmd/restic/cmd_snapshots_test.go b/cmd/restic/cmd_snapshots_test.go index 04af1a3c1..777c4272f 100644 --- a/cmd/restic/cmd_snapshots_test.go +++ b/cmd/restic/cmd_snapshots_test.go @@ -11,7 +11,8 @@ import ( func TestEmptySnapshotGroupJSON(t *testing.T) { for _, grouped := range []bool{false, true} { var w strings.Builder - printSnapshotGroupJSON(&w, nil, grouped) + err := printSnapshotGroupJSON(&w, nil, grouped) + rtest.OK(t, err) rtest.Equals(t, "[]", strings.TrimSpace(w.String())) } diff --git a/cmd/restic/delete.go b/cmd/restic/delete.go index e9b2481e0..26ca08196 100644 --- a/cmd/restic/delete.go +++ b/cmd/restic/delete.go @@ -9,7 +9,7 @@ import ( // DeleteFiles deletes the given fileList of fileType in parallel // it will print a warning if there is an error, but continue deleting the remaining files func DeleteFiles(gopts GlobalOptions, repo restic.Repository, fileList restic.IDSet, fileType restic.FileType) { - deleteFiles(gopts, true, repo, fileList, fileType) + _ = deleteFiles(gopts, true, repo, fileList, fileType) } // DeleteFilesChecked deletes the given fileList of fileType in parallel diff --git a/cmd/restic/exclude.go b/cmd/restic/exclude.go index e63c689ec..1ddd8932c 100644 --- a/cmd/restic/exclude.go +++ b/cmd/restic/exclude.go @@ -180,7 +180,9 @@ func isDirExcludedByFile(dir, tagFilename, header string) bool { Warnf("could not open exclusion tagfile: %v", err) return false } - defer f.Close() + defer func() { + _ = f.Close() + }() buf := make([]byte, len(header)) _, err = io.ReadFull(f, buf) // EOF is handled with a dedicated message, otherwise the warning were too cryptic diff --git a/cmd/restic/integration_test.go b/cmd/restic/integration_test.go index d17602d0b..1e39301f3 100644 --- a/cmd/restic/integration_test.go +++ b/cmd/restic/integration_test.go @@ -566,9 +566,9 @@ func TestBackupErrors(t *testing.T) { // Assume failure inaccessibleFile := filepath.Join(env.testdata, "0", "0", "9", "0") - os.Chmod(inaccessibleFile, 0000) + rtest.OK(t, os.Chmod(inaccessibleFile, 0000)) defer func() { - os.Chmod(inaccessibleFile, 0644) + rtest.OK(t, os.Chmod(inaccessibleFile, 0644)) }() opts := BackupOptions{} gopts := env.gopts @@ -933,7 +933,7 @@ func testRunKeyAddNewKeyUserHost(t testing.TB, gopts GlobalOptions) { keyHostname = "" }() - cmdKey.Flags().Parse([]string{"--user=john", "--host=example.com"}) + rtest.OK(t, cmdKey.Flags().Parse([]string{"--user=john", "--host=example.com"})) t.Log("adding key for john@example.com") rtest.OK(t, runKey(gopts, []string{"add"})) @@ -1106,7 +1106,7 @@ func TestRestoreLatest(t *testing.T) { testRunBackup(t, "", []string{filepath.Base(env.testdata)}, opts, env.gopts) testRunCheck(t, env.gopts) - os.Remove(p) + rtest.OK(t, os.Remove(p)) rtest.OK(t, appendRandomData(p, 101)) testRunBackup(t, "", []string{filepath.Base(env.testdata)}, opts, env.gopts) testRunCheck(t, env.gopts) @@ -1747,16 +1747,32 @@ func copyFile(dst string, src string) error { if err != nil { return err } - defer srcFile.Close() dstFile, err := os.Create(dst) if err != nil { + // ignore subsequent errors + _ = srcFile.Close() return err } - defer dstFile.Close() _, err = io.Copy(dstFile, srcFile) - return err + if err != nil { + // ignore subsequent errors + _ = srcFile.Close() + _ = dstFile.Close() + } + + err = srcFile.Close() + if err != nil { + return err + } + + err = dstFile.Close() + if err != nil { + return err + } + + return nil } var diffOutputRegexPatterns = []string{ diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 2b0ac4ada..235898df0 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -405,7 +405,10 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous debug.Log("%v hasn't changed, but contents are missing!", target) // There are contents missing - inform user! err := errors.Errorf("parts of %v not found in the repository index; storing the file again", target) - arch.error(abstarget, fi, err) + err = arch.error(abstarget, fi, err) + if err != nil { + return FutureNode{}, false, err + } } // reopen file and do an fstat() on the open file to check it is still @@ -457,7 +460,10 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous start := time.Now() oldSubtree, err := arch.loadSubtree(ctx, previous) if err != nil { - arch.error(abstarget, fi, err) + err = arch.error(abstarget, fi, err) + } + if err != nil { + return FutureNode{}, false, err } fn.isTree = true @@ -592,7 +598,10 @@ func (arch *Archiver) SaveTree(ctx context.Context, snPath string, atree *Tree, oldNode := previous.Find(name) oldSubtree, err := arch.loadSubtree(ctx, oldNode) if err != nil { - arch.error(join(snPath, name), nil, err) + err = arch.error(join(snPath, name), nil, err) + } + if err != nil { + return nil, err } // not a leaf node, archive subtree @@ -751,7 +760,7 @@ func (arch *Archiver) loadParentTree(ctx context.Context, snapshotID restic.ID) tree, err := arch.Repo.LoadTree(ctx, *sn.Tree) if err != nil { debug.Log("unable to load tree %v: %v", *sn.Tree, err) - arch.error("/", nil, arch.wrapLoadTreeError(*sn.Tree, err)) + _ = arch.error("/", nil, arch.wrapLoadTreeError(*sn.Tree, err)) return nil } return tree From aef3658a5f6a91dab0191ca745deb189405c3c7a Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 18:49:30 +0100 Subject: [PATCH 10/14] Address review comments --- cmd/restic/integration_test.go | 3 +++ internal/backend/rclone/internal_test.go | 3 ++- internal/repository/repository.go | 7 +------ 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/cmd/restic/integration_test.go b/cmd/restic/integration_test.go index 1e39301f3..b32d2dc39 100644 --- a/cmd/restic/integration_test.go +++ b/cmd/restic/integration_test.go @@ -1760,10 +1760,13 @@ func copyFile(dst string, src string) error { // ignore subsequent errors _ = srcFile.Close() _ = dstFile.Close() + return err } err = srcFile.Close() if err != nil { + // ignore subsequent errors + _ = dstFile.Close() return err } diff --git a/internal/backend/rclone/internal_test.go b/internal/backend/rclone/internal_test.go index a192a37bf..8bc661ab7 100644 --- a/internal/backend/rclone/internal_test.go +++ b/internal/backend/rclone/internal_test.go @@ -24,7 +24,8 @@ func TestRcloneExit(t *testing.T) { } rtest.OK(t, err) defer func() { - rtest.OK(t, be.Close()) + // ignore the error as the test will kill rclone (see below) + _ = be.Close() }() err = be.cmd.Process.Kill() diff --git a/internal/repository/repository.go b/internal/repository/repository.go index c21bd520c..a13a0d784 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -420,12 +420,7 @@ func (r *Repository) saveIndex(ctx context.Context, indexes ...*Index) error { debug.Log("Saved index %d as %v", i, sid) } - err := r.idx.MergeFinalIndexes() - if err != nil { - return err - } - - return nil + return r.idx.MergeFinalIndexes() } // SaveIndex saves all new indexes in the backend. From 0858fbf6aa2019b5414ee6d9e07ce975e5b6db75 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 19:35:46 +0100 Subject: [PATCH 11/14] Add more error handling --- helpers/prepare-release/main.go | 4 +++- internal/backend/backend_retry_test.go | 6 +++--- internal/backend/local/local.go | 16 ++++++++++++++-- internal/backend/local/local_internal_test.go | 4 +++- internal/backend/utils.go | 2 +- internal/cache/backend.go | 2 +- internal/cache/dir_test.go | 7 ++++++- internal/dump/acl.go | 15 +++++++++++++-- internal/dump/common.go | 3 ++- internal/dump/zip_test.go | 8 +++++++- internal/fs/fs_local_vss.go | 4 ++-- internal/mock/backend.go | 2 +- internal/pack/pack_internal_test.go | 12 ++++++------ internal/pack/pack_test.go | 3 ++- internal/restic/lock_unix.go | 4 +++- internal/restic/node_linux.go | 5 +++-- internal/restic/node_test.go | 3 ++- internal/restorer/fileswriter.go | 3 ++- internal/restorer/preallocate_test.go | 4 +++- internal/restorer/restorer_test.go | 6 ++++-- internal/test/helpers.go | 8 ++++++-- internal/textfile/read_test.go | 3 ++- internal/ui/termstatus/background_linux_test.go | 3 ++- 23 files changed, 91 insertions(+), 36 deletions(-) diff --git a/helpers/prepare-release/main.go b/helpers/prepare-release/main.go index 70dc31afe..774397b0b 100644 --- a/helpers/prepare-release/main.go +++ b/helpers/prepare-release/main.go @@ -237,7 +237,9 @@ func preCheckChangelogVersion() { if err != nil { die("unable to open CHANGELOG.md: %v", err) } - defer f.Close() + defer func() { + _ = f.Close() + }() sc := bufio.NewScanner(f) for sc.Scan() { diff --git a/internal/backend/backend_retry_test.go b/internal/backend/backend_retry_test.go index 9c9a6b708..a746032c7 100644 --- a/internal/backend/backend_retry_test.go +++ b/internal/backend/backend_retry_test.go @@ -62,11 +62,11 @@ func TestBackendListRetry(t *testing.T) { // fail during first retry, succeed during second retry++ if retry == 1 { - fn(restic.FileInfo{Name: ID1}) + _ = fn(restic.FileInfo{Name: ID1}) return errors.New("test list error") } - fn(restic.FileInfo{Name: ID1}) - fn(restic.FileInfo{Name: ID2}) + _ = fn(restic.FileInfo{Name: ID1}) + _ = fn(restic.FileInfo{Name: ID2}) return nil }, } diff --git a/internal/backend/local/local.go b/internal/backend/local/local.go index f1b658864..8e9625734 100644 --- a/internal/backend/local/local.go +++ b/internal/backend/local/local.go @@ -265,9 +265,15 @@ func visitDirs(ctx context.Context, dir string, fn func(restic.FileInfo) error) if err != nil { return err } - defer d.Close() sub, err := d.Readdirnames(-1) + if err != nil { + // ignore subsequent errors + _ = d.Close() + return err + } + + err = d.Close() if err != nil { return err } @@ -286,9 +292,15 @@ func visitFiles(ctx context.Context, dir string, fn func(restic.FileInfo) error) if err != nil { return err } - defer d.Close() sub, err := d.Readdir(-1) + if err != nil { + // ignore subsequent errors + _ = d.Close() + return err + } + + err = d.Close() if err != nil { return err } diff --git a/internal/backend/local/local_internal_test.go b/internal/backend/local/local_internal_test.go index a67233903..030099488 100644 --- a/internal/backend/local/local_internal_test.go +++ b/internal/backend/local/local_internal_test.go @@ -30,7 +30,9 @@ func TestNoSpacePermanent(t *testing.T) { be, err := Open(context.Background(), Config{Path: dir}) rtest.OK(t, err) - defer be.Close() + defer func() { + rtest.OK(t, be.Close()) + }() h := restic.Handle{Type: restic.ConfigFile} err = be.Save(context.Background(), h, nil) diff --git a/internal/backend/utils.go b/internal/backend/utils.go index e6dc7a549..39c68b4ce 100644 --- a/internal/backend/utils.go +++ b/internal/backend/utils.go @@ -52,7 +52,7 @@ func DefaultLoad(ctx context.Context, h restic.Handle, length int, offset int64, } err = fn(rd) if err != nil { - rd.Close() // ignore secondary errors closing the reader + _ = rd.Close() // ignore secondary errors closing the reader return err } return rd.Close() diff --git a/internal/cache/backend.go b/internal/cache/backend.go index a58c695f7..876f30110 100644 --- a/internal/cache/backend.go +++ b/internal/cache/backend.go @@ -166,7 +166,7 @@ func (b *Backend) Load(ctx context.Context, h restic.Handle, length int, offset if err == nil { err = consumer(rd) if err != nil { - rd.Close() // ignore secondary errors + _ = rd.Close() // ignore secondary errors return err } return rd.Close() diff --git a/internal/cache/dir_test.go b/internal/cache/dir_test.go index 73a08eb4d..3076bea3d 100644 --- a/internal/cache/dir_test.go +++ b/internal/cache/dir_test.go @@ -17,7 +17,12 @@ func TestCacheDirEnv(t *testing.T) { if err != nil { t.Fatal(err) } - defer os.Unsetenv("RESTIC_CACHE_DIR") + defer func() { + err := os.Unsetenv("RESTIC_CACHE_DIR") + if err != nil { + t.Fatal(err) + } + }() } dir, err := DefaultDir() diff --git a/internal/dump/acl.go b/internal/dump/acl.go index 9c5fd95de..48ce28a4c 100644 --- a/internal/dump/acl.go +++ b/internal/dump/acl.go @@ -101,12 +101,23 @@ func (a *acl) decode(xattr []byte) { func (a *acl) encode() []byte { buf := new(bytes.Buffer) ae := new(aclElem) - binary.Write(buf, binary.LittleEndian, &a.Version) + + err := binary.Write(buf, binary.LittleEndian, &a.Version) + // write to a bytes.Buffer always returns a nil error + if err != nil { + panic(err) + } + for _, elem := range a.List { ae.Tag = uint16(elem.getType()) ae.Perm = elem.Perm ae.ID = elem.getID() - binary.Write(buf, binary.LittleEndian, ae) + + err := binary.Write(buf, binary.LittleEndian, ae) + // write to a bytes.Buffer always returns a nil error + if err != nil { + panic(err) + } } return buf.Bytes() } diff --git a/internal/dump/common.go b/internal/dump/common.go index fd74a4a07..eb02b1c2e 100644 --- a/internal/dump/common.go +++ b/internal/dump/common.go @@ -25,7 +25,8 @@ func writeDump(ctx context.Context, repo restic.Repository, tree *restic.Tree, r rootNode.Path = rootPath err := dumpTree(ctx, repo, rootNode, rootPath, dmp) if err != nil { - dmp.Close() + // ignore subsequent errors + _ = dmp.Close() return err } diff --git a/internal/dump/zip_test.go b/internal/dump/zip_test.go index 84e0d0487..3b482244c 100644 --- a/internal/dump/zip_test.go +++ b/internal/dump/zip_test.go @@ -23,10 +23,16 @@ func readZipFile(f *zip.File) ([]byte, error) { if err != nil { return nil, err } - defer rc.Close() b := &bytes.Buffer{} _, err = b.ReadFrom(rc) + if err != nil { + // ignore subsequent errors + _ = rc.Close() + return nil, err + } + + err = rc.Close() if err != nil { return nil, err } diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index 7deef8f05..b3e08fed9 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -50,7 +50,7 @@ func (fs *LocalVss) DeleteSnapshots() { for volumeName, snapshot := range fs.snapshots { if err := snapshot.Delete(); err != nil { - fs.msgError(volumeName, errors.Errorf("failed to delete VSS snapshot: %s", err)) + _ = fs.msgError(volumeName, errors.Errorf("failed to delete VSS snapshot: %s", err)) activeSnapshots[volumeName] = snapshot } } @@ -117,7 +117,7 @@ func (fs *LocalVss) snapshotPath(path string) string { fs.msgMessage("creating VSS snapshot for [%s]\n", vssVolume) if snapshot, err := NewVssSnapshot(vssVolume, 120, fs.msgError); err != nil { - fs.msgError(vssVolume, errors.Errorf("failed to create snapshot for [%s]: %s\n", + _ = fs.msgError(vssVolume, errors.Errorf("failed to create snapshot for [%s]: %s\n", vssVolume, err)) fs.failedSnapshots[volumeNameLower] = struct{}{} } else { diff --git a/internal/mock/backend.go b/internal/mock/backend.go index 930fdb3ee..e3759acbf 100644 --- a/internal/mock/backend.go +++ b/internal/mock/backend.go @@ -73,7 +73,7 @@ func (m *Backend) Load(ctx context.Context, h restic.Handle, length int, offset } err = fn(rd) if err != nil { - rd.Close() // ignore secondary errors closing the reader + _ = rd.Close() // ignore secondary errors closing the reader return err } return rd.Close() diff --git a/internal/pack/pack_internal_test.go b/internal/pack/pack_internal_test.go index 6502e4a35..b0a5d2862 100644 --- a/internal/pack/pack_internal_test.go +++ b/internal/pack/pack_internal_test.go @@ -61,9 +61,9 @@ func TestReadHeaderEagerLoad(t *testing.T) { expectedHeader := rtest.Random(0, entryCount*int(EntrySize)+crypto.Extension) buf := &bytes.Buffer{} - buf.Write(rtest.Random(0, dataSize)) // pack blobs data - buf.Write(expectedHeader) // pack header - binary.Write(buf, binary.LittleEndian, uint32(len(expectedHeader))) // pack header length + buf.Write(rtest.Random(0, dataSize)) // pack blobs data + buf.Write(expectedHeader) // pack header + rtest.OK(t, binary.Write(buf, binary.LittleEndian, uint32(len(expectedHeader)))) // pack header length rd := &countingReaderAt{delegate: bytes.NewReader(buf.Bytes())} @@ -104,9 +104,9 @@ func TestReadRecords(t *testing.T) { expectedHeader := totalHeader[off:] buf := &bytes.Buffer{} - buf.Write(rtest.Random(0, dataSize)) // pack blobs data - buf.Write(totalHeader) // pack header - binary.Write(buf, binary.LittleEndian, uint32(len(totalHeader))) // pack header length + buf.Write(rtest.Random(0, dataSize)) // pack blobs data + buf.Write(totalHeader) // pack header + rtest.OK(t, binary.Write(buf, binary.LittleEndian, uint32(len(totalHeader)))) // pack header length rd := bytes.NewReader(buf.Bytes()) diff --git a/internal/pack/pack_test.go b/internal/pack/pack_test.go index 471e901c3..02413247f 100644 --- a/internal/pack/pack_test.go +++ b/internal/pack/pack_test.go @@ -39,7 +39,8 @@ func newPack(t testing.TB, k *crypto.Key, lengths []int) ([]Buf, []byte, uint) { var buf bytes.Buffer p := pack.NewPacker(k, &buf) for _, b := range bufs { - p.Add(restic.TreeBlob, b.id, b.data) + _, err := p.Add(restic.TreeBlob, b.id, b.data) + rtest.OK(t, err) } _, err := p.Finalize() diff --git a/internal/restic/lock_unix.go b/internal/restic/lock_unix.go index 019cbbfa5..266f55580 100644 --- a/internal/restic/lock_unix.go +++ b/internal/restic/lock_unix.go @@ -38,7 +38,9 @@ func (l Lock) processExists() bool { debug.Log("error searching for process %d: %v\n", l.PID, err) return false } - defer proc.Release() + defer func() { + _ = proc.Release() + }() debug.Log("sending SIGHUP to process %d\n", l.PID) err = proc.Signal(syscall.SIGHUP) diff --git a/internal/restic/node_linux.go b/internal/restic/node_linux.go index 1e45a6ed2..bdb03651f 100644 --- a/internal/restic/node_linux.go +++ b/internal/restic/node_linux.go @@ -15,7 +15,6 @@ func (node Node) restoreSymlinkTimestamps(path string, utimes [2]syscall.Timespe if err != nil { return errors.Wrap(err, "Open") } - defer dir.Close() times := []unix.Timespec{ {Sec: utimes[0].Sec, Nsec: utimes[0].Nsec}, @@ -25,10 +24,12 @@ func (node Node) restoreSymlinkTimestamps(path string, utimes [2]syscall.Timespe err = unix.UtimesNanoAt(int(dir.Fd()), filepath.Base(path), times, unix.AT_SYMLINK_NOFOLLOW) if err != nil { + // ignore subsequent errors + _ = dir.Close() return errors.Wrap(err, "UtimesNanoAt") } - return nil + return dir.Close() } func (node Node) device() int { diff --git a/internal/restic/node_test.go b/internal/restic/node_test.go index dc2449bca..5390a90ad 100644 --- a/internal/restic/node_test.go +++ b/internal/restic/node_test.go @@ -29,7 +29,8 @@ func BenchmarkNodeFillUser(t *testing.B) { t.ResetTimer() for i := 0; i < t.N; i++ { - restic.NodeFromFileInfo(path, fi) + _, err := restic.NodeFromFileInfo(path, fi) + rtest.OK(t, err) } rtest.OK(t, tempfile.Close()) diff --git a/internal/restorer/fileswriter.go b/internal/restorer/fileswriter.go index 1a73055c4..542a15f02 100644 --- a/internal/restorer/fileswriter.go +++ b/internal/restorer/fileswriter.go @@ -97,7 +97,8 @@ func (w *filesWriter) writeToFile(path string, blob []byte, offset int64, create _, err = wr.WriteAt(blob, offset) if err != nil { - releaseWriter(wr) + // ignore subsequent errors + _ = releaseWriter(wr) return err } diff --git a/internal/restorer/preallocate_test.go b/internal/restorer/preallocate_test.go index 05b3a8efd..fe77c183b 100644 --- a/internal/restorer/preallocate_test.go +++ b/internal/restorer/preallocate_test.go @@ -19,7 +19,9 @@ func TestPreallocate(t *testing.T) { flags := os.O_CREATE | os.O_TRUNC | os.O_WRONLY wr, err := os.OpenFile(path.Join(dirpath, "test"), flags, 0600) test.OK(t, err) - defer wr.Close() + defer func() { + test.OK(t, wr.Close()) + }() err = preallocateFile(wr, i) test.OK(t, err) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 247f2ac4d..32be0f51d 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -74,7 +74,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode if mode == 0 { mode = 0644 } - tree.Insert(&restic.Node{ + err := tree.Insert(&restic.Node{ Type: "file", Mode: mode, ModTime: node.ModTime, @@ -86,6 +86,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode Inode: fi, Links: lc, }) + rtest.OK(t, err) case Dir: id := saveDir(t, repo, node.Nodes, inode) @@ -94,7 +95,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode mode = 0755 } - tree.Insert(&restic.Node{ + err := tree.Insert(&restic.Node{ Type: "dir", Mode: mode, ModTime: node.ModTime, @@ -103,6 +104,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode GID: uint32(os.Getgid()), Subtree: &id, }) + rtest.OK(t, err) default: t.Fatalf("unknown node type %T", node) } diff --git a/internal/test/helpers.go b/internal/test/helpers.go index f01c030f3..1e127f786 100644 --- a/internal/test/helpers.go +++ b/internal/test/helpers.go @@ -95,7 +95,9 @@ func Random(seed, count int) []byte { func SetupTarTestFixture(t testing.TB, outputDir, tarFile string) { input, err := os.Open(tarFile) OK(t, err) - defer input.Close() + defer func() { + OK(t, input.Close()) + }() var rd io.Reader switch filepath.Ext(tarFile) { @@ -103,7 +105,9 @@ func SetupTarTestFixture(t testing.TB, outputDir, tarFile string) { r, err := gzip.NewReader(input) OK(t, err) - defer r.Close() + defer func() { + OK(t, r.Close()) + }() rd = r case ".bzip2": rd = bzip2.NewReader(input) diff --git a/internal/textfile/read_test.go b/internal/textfile/read_test.go index 8e8e659dc..d4de03513 100644 --- a/internal/textfile/read_test.go +++ b/internal/textfile/read_test.go @@ -25,7 +25,8 @@ func writeTempfile(t testing.TB, data []byte) (string, func()) { err = closeErr } if err != nil { - os.Remove(name) + // ignore subsequent errors + _ = os.Remove(name) t.Fatal(err) } }() diff --git a/internal/ui/termstatus/background_linux_test.go b/internal/ui/termstatus/background_linux_test.go index 0880d2bc0..f5796b23b 100644 --- a/internal/ui/termstatus/background_linux_test.go +++ b/internal/ui/termstatus/background_linux_test.go @@ -12,8 +12,9 @@ func TestIsProcessBackground(t *testing.T) { if err != nil { t.Skipf("can't open terminal: %v", err) } - defer tty.Close() _, err = isProcessBackground(tty.Fd()) rtest.OK(t, err) + + _ = tty.Close() } From a00e27adf6cebd5f8045ff5fe350bf1bfbb25613 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 20:22:25 +0100 Subject: [PATCH 12/14] Add entry to changelog --- changelog/unreleased/pr-3250 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/pr-3250 diff --git a/changelog/unreleased/pr-3250 b/changelog/unreleased/pr-3250 new file mode 100644 index 000000000..f4aa555e5 --- /dev/null +++ b/changelog/unreleased/pr-3250 @@ -0,0 +1,6 @@ +Enhancement: Add more error checks + +We've added a lot more error checks in places where errors were ignored before +(as hinted by the static analysis programm `errcheck` via `golangci-lint`). + +https://github.com/restic/restic/pull/3250 From f867e65bcd83451277273113d69d29af1d951e7b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 20:43:53 +0100 Subject: [PATCH 13/14] Fix issues reported by staticcheck --- cmd/restic/global_test.go | 2 +- cmd/restic/integration_test.go | 39 +++++++++++++++++++++++------- internal/backend/rclone/backend.go | 3 +-- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/cmd/restic/global_test.go b/cmd/restic/global_test.go index 2a0b05ceb..fee5294b5 100644 --- a/cmd/restic/global_test.go +++ b/cmd/restic/global_test.go @@ -54,7 +54,7 @@ func TestReadRepo(t *testing.T) { var opts3 GlobalOptions opts3.RepositoryFile = foo + "-invalid" - repo, err = ReadRepo(opts3) + _, err = ReadRepo(opts3) if err == nil { t.Fatal("must not read repository path from invalid file path") } diff --git a/cmd/restic/integration_test.go b/cmd/restic/integration_test.go index b32d2dc39..7d198d336 100644 --- a/cmd/restic/integration_test.go +++ b/cmd/restic/integration_test.go @@ -166,7 +166,7 @@ func testRunDiffOutput(gopts GlobalOptions, firstSnapshotID string, secondSnapsh ShowMetadata: false, } err := runDiff(opts, gopts, []string{firstSnapshotID, secondSnapshotID}) - return string(buf.Bytes()), err + return buf.String(), err } func testRunRebuildIndex(t testing.TB, gopts GlobalOptions) { @@ -657,7 +657,11 @@ func TestBackupTags(t *testing.T) { testRunBackup(t, "", []string{env.testdata}, opts, env.gopts) testRunCheck(t, env.gopts) newest, _ := testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + + if newest == nil { + t.Fatal("expected a backup, got nil") + } + rtest.Assert(t, len(newest.Tags) == 0, "expected no tags, got %v", newest.Tags) parent := newest @@ -666,7 +670,11 @@ func TestBackupTags(t *testing.T) { testRunBackup(t, "", []string{env.testdata}, opts, env.gopts) testRunCheck(t, env.gopts) newest, _ = testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + + if newest == nil { + t.Fatal("expected a backup, got nil") + } + rtest.Assert(t, len(newest.Tags) == 1 && newest.Tags[0] == "NL", "expected one NL tag, got %v", newest.Tags) // Tagged backup should have untagged backup as parent. @@ -833,7 +841,10 @@ func TestTag(t *testing.T) { testRunBackup(t, "", []string{env.testdata}, BackupOptions{}, env.gopts) testRunCheck(t, env.gopts) newest, _ := testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + if newest == nil { + t.Fatal("expected a new backup, got nil") + } + rtest.Assert(t, len(newest.Tags) == 0, "expected no tags, got %v", newest.Tags) rtest.Assert(t, newest.Original == nil, @@ -843,7 +854,9 @@ func TestTag(t *testing.T) { testRunTag(t, TagOptions{SetTags: restic.TagLists{[]string{"NL"}}}, env.gopts) testRunCheck(t, env.gopts) newest, _ = testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + if newest == nil { + t.Fatal("expected a backup, got nil") + } rtest.Assert(t, len(newest.Tags) == 1 && newest.Tags[0] == "NL", "set failed, expected one NL tag, got %v", newest.Tags) rtest.Assert(t, newest.Original != nil, "expected original snapshot id, got nil") @@ -853,7 +866,9 @@ func TestTag(t *testing.T) { testRunTag(t, TagOptions{AddTags: restic.TagLists{[]string{"CH"}}}, env.gopts) testRunCheck(t, env.gopts) newest, _ = testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + if newest == nil { + t.Fatal("expected a backup, got nil") + } rtest.Assert(t, len(newest.Tags) == 2 && newest.Tags[0] == "NL" && newest.Tags[1] == "CH", "add failed, expected CH,NL tags, got %v", newest.Tags) rtest.Assert(t, newest.Original != nil, "expected original snapshot id, got nil") @@ -863,7 +878,9 @@ func TestTag(t *testing.T) { testRunTag(t, TagOptions{RemoveTags: restic.TagLists{[]string{"NL"}}}, env.gopts) testRunCheck(t, env.gopts) newest, _ = testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + if newest == nil { + t.Fatal("expected a backup, got nil") + } rtest.Assert(t, len(newest.Tags) == 1 && newest.Tags[0] == "CH", "remove failed, expected one CH tag, got %v", newest.Tags) rtest.Assert(t, newest.Original != nil, "expected original snapshot id, got nil") @@ -874,7 +891,9 @@ func TestTag(t *testing.T) { testRunTag(t, TagOptions{RemoveTags: restic.TagLists{[]string{"CH", "US", "RU"}}}, env.gopts) testRunCheck(t, env.gopts) newest, _ = testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + if newest == nil { + t.Fatal("expected a backup, got nil") + } rtest.Assert(t, len(newest.Tags) == 0, "expected no tags, got %v", newest.Tags) rtest.Assert(t, newest.Original != nil, "expected original snapshot id, got nil") @@ -885,7 +904,9 @@ func TestTag(t *testing.T) { testRunTag(t, TagOptions{SetTags: restic.TagLists{[]string{""}}}, env.gopts) testRunCheck(t, env.gopts) newest, _ = testRunSnapshots(t, env.gopts) - rtest.Assert(t, newest != nil, "expected a new backup, got nil") + if newest == nil { + t.Fatal("expected a backup, got nil") + } rtest.Assert(t, len(newest.Tags) == 0, "expected no tags, got %v", newest.Tags) rtest.Assert(t, newest.Original != nil, "expected original snapshot id, got nil") diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go index 25169fd58..9aa36c9d3 100644 --- a/internal/backend/rclone/backend.go +++ b/internal/backend/rclone/backend.go @@ -228,12 +228,11 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) { // rclone is able to accept HTTP requests. url := fmt.Sprintf("http://localhost/file-%d", rand.Uint64()) - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, err } req.Header.Set("Accept", rest.ContentTypeV2) - req.Cancel = ctx.Done() res, err := ctxhttp.Do(ctx, client, req) if err != nil { From 04ca69cc78d8f7253b048efecd633967f2f6ddd3 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 30 Jan 2021 20:45:57 +0100 Subject: [PATCH 14/14] Address issues reported by golint --- internal/checker/checker_test.go | 8 ++++---- internal/fuse/fuse_test.go | 8 ++++---- internal/pack/pack.go | 2 +- internal/restorer/filerestorer_test.go | 8 -------- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/internal/checker/checker_test.go b/internal/checker/checker_test.go index ad1b15f1a..ade2813c7 100644 --- a/internal/checker/checker_test.go +++ b/internal/checker/checker_test.go @@ -489,7 +489,7 @@ func TestCheckerBlobTypeConfusion(t *testing.T) { Nodes: []*restic.Node{malNode, dirNode}, } - rootId, err := repo.SaveTree(ctx, rootTree) + rootID, err := repo.SaveTree(ctx, rootTree) test.OK(t, err) test.OK(t, repo.Flush(ctx)) @@ -498,12 +498,12 @@ func TestCheckerBlobTypeConfusion(t *testing.T) { snapshot, err := restic.NewSnapshot([]string{"/damaged"}, []string{"test"}, "foo", time.Now()) test.OK(t, err) - snapshot.Tree = &rootId + snapshot.Tree = &rootID - snapId, err := repo.SaveJSONUnpacked(ctx, restic.SnapshotFile, snapshot) + snapID, err := repo.SaveJSONUnpacked(ctx, restic.SnapshotFile, snapshot) test.OK(t, err) - t.Logf("saved snapshot %v", snapId.Str()) + t.Logf("saved snapshot %v", snapID.Str()) delayRepo := &delayRepository{ Repository: repo, diff --git a/internal/fuse/fuse_test.go b/internal/fuse/fuse_test.go index 50128a9c0..24d8cf03d 100644 --- a/internal/fuse/fuse_test.go +++ b/internal/fuse/fuse_test.go @@ -219,17 +219,17 @@ func TestFuseDir(t *testing.T) { } // Test top-level directories for their UID and GID. -func TestTopUidGid(t *testing.T) { +func TestTopUIDGID(t *testing.T) { repo, cleanup := repository.TestRepository(t) defer cleanup() restic.TestCreateSnapshot(t, repo, time.Unix(1460289341, 207401672), 0, 0) - testTopUidGid(t, Config{}, repo, uint32(os.Getuid()), uint32(os.Getgid())) - testTopUidGid(t, Config{OwnerIsRoot: true}, repo, 0, 0) + testTopUIDGID(t, Config{}, repo, uint32(os.Getuid()), uint32(os.Getgid())) + testTopUIDGID(t, Config{OwnerIsRoot: true}, repo, 0, 0) } -func testTopUidGid(t *testing.T, cfg Config, repo restic.Repository, uid, gid uint32) { +func testTopUIDGID(t *testing.T, cfg Config, repo restic.Repository, uid, gid uint32) { t.Helper() ctx := context.Background() diff --git a/internal/pack/pack.go b/internal/pack/pack.go index c22f434d1..d679c658b 100644 --- a/internal/pack/pack.go +++ b/internal/pack/pack.go @@ -157,7 +157,7 @@ var ( const ( // size of the header-length field at the end of the file; it is a uint32 headerLengthSize = 4 - // constant overhead of the header independent of #entries + // HeaderSize is the header's constant overhead (independent of #entries) HeaderSize = headerLengthSize + crypto.Extension maxHeaderSize = 16 * 1024 * 1024 diff --git a/internal/restorer/filerestorer_test.go b/internal/restorer/filerestorer_test.go index 75433cdef..28bcd5e28 100644 --- a/internal/restorer/filerestorer_test.go +++ b/internal/restorer/filerestorer_test.go @@ -44,14 +44,6 @@ func (i *TestRepo) Lookup(bh restic.BlobHandle) []restic.PackedBlob { return packs } -func (i *TestRepo) packName(pack *packInfo) string { - return i.packsIDToName[pack.id] -} - -func (i *TestRepo) packID(name string) restic.ID { - return i.packsNameToID[name] -} - func (i *TestRepo) fileContent(file *fileInfo) string { return i.filesPathToContent[file.location] }