From bfa18ee8ecbafe0c6d81e82bb8b810aa857713f0 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 28 Oct 2018 21:12:15 +0100 Subject: [PATCH 1/2] DownloadAndHash: Check error returned by Load() --- internal/checker/checker.go | 9 +-- internal/repository/repack.go | 5 +- internal/repository/repository.go | 23 ++++-- internal/repository/repository_test.go | 106 +++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 15 deletions(-) diff --git a/internal/checker/checker.go b/internal/checker/checker.go index faafdc6b7..7255e990d 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -7,13 +7,12 @@ import ( "os" "sync" - "github.com/restic/restic/internal/errors" - "github.com/restic/restic/internal/restic" - "golang.org/x/sync/errgroup" - "github.com/restic/restic/internal/debug" + "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/pack" "github.com/restic/restic/internal/repository" + "github.com/restic/restic/internal/restic" + "golang.org/x/sync/errgroup" ) // Checker runs various checks on a repository. It is advisable to create an @@ -652,7 +651,7 @@ func checkPack(ctx context.Context, r restic.Repository, id restic.ID) error { debug.Log("checking pack %v", id) h := restic.Handle{Type: restic.DataFile, Name: id.String()} - packfile, hash, size, err := repository.DownloadAndHash(ctx, r, h) + packfile, hash, size, err := repository.DownloadAndHash(ctx, r.Backend(), h) if err != nil { return errors.Wrap(err, "checkPack") } diff --git a/internal/repository/repack.go b/internal/repository/repack.go index 8f4c2caea..d0119c204 100644 --- a/internal/repository/repack.go +++ b/internal/repository/repack.go @@ -6,11 +6,10 @@ import ( "os" "github.com/restic/restic/internal/debug" + "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/pack" "github.com/restic/restic/internal/restic" - - "github.com/restic/restic/internal/errors" ) // Repack takes a list of packs together with a list of blobs contained in @@ -24,7 +23,7 @@ func Repack(ctx context.Context, repo restic.Repository, packs restic.IDSet, kee // load the complete pack into a temp file h := restic.Handle{Type: restic.DataFile, Name: packID.String()} - tempfile, hash, packLength, err := DownloadAndHash(ctx, repo, h) + tempfile, hash, packLength, err := DownloadAndHash(ctx, repo.Backend(), h) if err != nil { return nil, errors.Wrap(err, "Repack") } diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 4a76f4025..b44de7c6f 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -9,16 +9,15 @@ import ( "io" "os" + "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/cache" + "github.com/restic/restic/internal/crypto" + "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/hashing" - "github.com/restic/restic/internal/restic" - - "github.com/restic/restic/internal/backend" - "github.com/restic/restic/internal/crypto" - "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/pack" + "github.com/restic/restic/internal/restic" ) // Repository is used to access a repository in a backend. @@ -694,16 +693,21 @@ func (r *Repository) SaveTree(ctx context.Context, t *restic.Tree) (restic.ID, e return id, err } +// Loader allows loading data from a backend. +type Loader interface { + Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error +} + // DownloadAndHash is all-in-one helper to download content of the file at h to a temporary filesystem location // and calculate ID of the contents. Returned (temporary) file is positioned at the beginning of the file; // it is reponsibility of the caller to close and delete the file. -func DownloadAndHash(ctx context.Context, repo restic.Repository, h restic.Handle) (tmpfile *os.File, hash restic.ID, size int64, err error) { +func DownloadAndHash(ctx context.Context, be Loader, h restic.Handle) (tmpfile *os.File, hash restic.ID, size int64, err error) { tmpfile, err = fs.TempFile("", "restic-temp-") if err != nil { return nil, restic.ID{}, -1, errors.Wrap(err, "TempFile") } - err = repo.Backend().Load(ctx, h, 0, 0, func(rd io.Reader) (ierr error) { + err = be.Load(ctx, h, 0, 0, func(rd io.Reader) (ierr error) { _, ierr = tmpfile.Seek(0, io.SeekStart) if ierr == nil { ierr = tmpfile.Truncate(0) @@ -716,6 +720,11 @@ func DownloadAndHash(ctx context.Context, repo restic.Repository, h restic.Handl hash = restic.IDFromHash(hrd.Sum(nil)) return ierr }) + if err != nil { + tmpfile.Close() + os.Remove(tmpfile.Name()) + return nil, restic.ID{}, -1, errors.Wrap(err, "Load") + } _, err = tmpfile.Seek(0, io.SeekStart) if err != nil { diff --git a/internal/repository/repository_test.go b/internal/repository/repository_test.go index 60c1190ce..8ea203d59 100644 --- a/internal/repository/repository_test.go +++ b/internal/repository/repository_test.go @@ -11,6 +11,8 @@ import ( "time" "github.com/restic/restic/internal/archiver" + "github.com/restic/restic/internal/errors" + "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" rtest "github.com/restic/restic/internal/test" @@ -392,3 +394,107 @@ func TestRepositoryIncrementalIndex(t *testing.T) { } } } + +type backend struct { + rd io.Reader +} + +func (be backend) Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error { + return fn(be.rd) +} + +type retryBackend struct { + buf []byte +} + +func (be retryBackend) Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error { + err := fn(bytes.NewReader(be.buf[:len(be.buf)/2])) + if err != nil { + return err + } + + return fn(bytes.NewReader(be.buf)) +} + +func TestDownloadAndHash(t *testing.T) { + buf := make([]byte, 5*1024*1024+881) + _, err := io.ReadFull(rnd, buf) + if err != nil { + t.Fatal(err) + } + + var tests = []struct { + be repository.Loader + want []byte + }{ + { + be: backend{rd: bytes.NewReader(buf)}, + want: buf, + }, + { + be: retryBackend{buf: buf}, + want: buf, + }, + } + + for _, test := range tests { + t.Run("", func(t *testing.T) { + f, id, size, err := repository.DownloadAndHash(context.TODO(), test.be, restic.Handle{}) + if err != nil { + t.Error(err) + } + + want := restic.Hash(test.want) + if !want.Equal(id) { + t.Errorf("wrong hash returned, want %v, got %v", want.Str(), id.Str()) + } + + if size != int64(len(test.want)) { + t.Errorf("wrong size returned, want %v, got %v", test.want, size) + } + + err = f.Close() + if err != nil { + t.Error(err) + } + + err = fs.RemoveIfExists(f.Name()) + if err != nil { + t.Fatal(err) + } + }) + } +} + +type errorReader struct { + err error +} + +func (er errorReader) Read(p []byte) (n int, err error) { + return 0, er.err +} + +func TestDownloadAndHashErrors(t *testing.T) { + var tests = []struct { + be repository.Loader + err string + }{ + { + be: backend{rd: errorReader{errors.New("test error 1")}}, + err: "test error 1", + }, + } + + for _, test := range tests { + t.Run("", func(t *testing.T) { + _, _, _, err := repository.DownloadAndHash(context.TODO(), test.be, restic.Handle{}) + if err == nil { + t.Fatalf("wanted error %q, got nil", test.err) + } + + if errors.Cause(err).Error() != test.err { + t.Fatalf("wanted error %q, got %q", test.err, err) + } + }) + } +} From 157d36589494d7e8e75c5482e2feda1d2fbb5bad Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 28 Oct 2018 21:38:04 +0100 Subject: [PATCH 2/2] Add entry to CHANGELOG --- changelog/unreleased/pull-2068 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/pull-2068 diff --git a/changelog/unreleased/pull-2068 b/changelog/unreleased/pull-2068 new file mode 100644 index 000000000..1b8853943 --- /dev/null +++ b/changelog/unreleased/pull-2068 @@ -0,0 +1,6 @@ +Bugfix: Correctly return error loading data + +In one case during `prune` and `check`, an error loading data from the backend is not returned properly. This is now corrected. + +https://github.com/restic/restic/pull/2068 +https://github.com/restic/restic/issues/1999#issuecomment-433737921