From d4aadfa3893baa90c1730eb9b7dc3e46e3655ed8 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sun, 9 Oct 2022 10:21:30 +0200 Subject: [PATCH 1/4] all: Drop ctxhttp This package is no longer needed, since we can use the stdlib's http.NewRequestWithContext. backend/rclone already did, but it needed a different error check due to a difference between net/http and ctxhttp. Also, store the http.Client by value in the REST backend (changed to a pointer when ctxhttp was introduced) and use errors.WithStack instead of errors.Wrap where the message was no longer accurate. Errors from http.NewRequestWithContext will start with "net/http" or "net/url", so they're easy to identify. --- internal/backend/rclone/backend.go | 7 +++-- internal/backend/rest/rest.go | 43 ++++++++++++++---------------- internal/selfupdate/github.go | 9 +++---- 3 files changed, 27 insertions(+), 32 deletions(-) diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go index f4e2770a1..97ed417a3 100644 --- a/internal/backend/rclone/backend.go +++ b/internal/backend/rclone/backend.go @@ -22,7 +22,6 @@ import ( "github.com/restic/restic/internal/backend/rest" "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" - "golang.org/x/net/context/ctxhttp" "golang.org/x/net/http2" ) @@ -216,7 +215,7 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) { }() // send an HTTP request to the base URL, see if the server is there - client := &http.Client{ + client := http.Client{ Transport: debug.RoundTripper(tr), Timeout: cfg.Timeout, } @@ -231,7 +230,7 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) { } req.Header.Set("Accept", rest.ContentTypeV2) - res, err := ctxhttp.Do(ctx, client, req) + res, err := client.Do(req) if err != nil { // ignore subsequent errors _ = bg() @@ -240,7 +239,7 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) { // wait for rclone to exit wg.Wait() // try to return the program exit code if communication with rclone has failed - if be.waitResult != nil && (err == context.Canceled || errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, syscall.EPIPE)) { + if be.waitResult != nil && (errors.Is(err, context.Canceled) || errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, syscall.EPIPE)) { err = be.waitResult } diff --git a/internal/backend/rest/rest.go b/internal/backend/rest/rest.go index cd41bc0ce..6444e8655 100644 --- a/internal/backend/rest/rest.go +++ b/internal/backend/rest/rest.go @@ -14,8 +14,6 @@ import ( "strconv" "strings" - "golang.org/x/net/context/ctxhttp" - "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/backend/sema" "github.com/restic/restic/internal/debug" @@ -33,7 +31,7 @@ type Backend struct { url *url.URL connections uint sem sema.Semaphore - client *http.Client + client http.Client backend.Layout } @@ -45,8 +43,6 @@ const ( // Open opens the REST backend with the given config. func Open(cfg Config, rt http.RoundTripper) (*Backend, error) { - client := &http.Client{Transport: rt} - sem, err := sema.New(cfg.Connections) if err != nil { return nil, err @@ -60,7 +56,7 @@ func Open(cfg Config, rt http.RoundTripper) (*Backend, error) { be := &Backend{ url: cfg.URL, - client: client, + client: http.Client{Transport: rt}, Layout: &backend.RESTLayout{URL: url, Join: path.Join}, connections: cfg.Connections, sem: sem, @@ -138,9 +134,10 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea defer cancel() // make sure that client.Post() cannot close the reader by wrapping it - req, err := http.NewRequest(http.MethodPost, b.Filename(h), ioutil.NopCloser(rd)) + req, err := http.NewRequestWithContext(ctx, + http.MethodPost, b.Filename(h), ioutil.NopCloser(rd)) if err != nil { - return errors.Wrap(err, "NewRequest") + return errors.WithStack(err) } req.Header.Set("Content-Type", "application/octet-stream") req.Header.Set("Accept", ContentTypeV2) @@ -150,7 +147,7 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea req.ContentLength = rd.Length() b.sem.GetToken() - resp, err := ctxhttp.Do(ctx, b.client, req) + resp, err := b.client.Do(req) b.sem.ReleaseToken() var cerr error @@ -160,7 +157,7 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea } if err != nil { - return errors.Wrap(err, "client.Post") + return errors.WithStack(err) } if resp.StatusCode != 200 { @@ -269,9 +266,9 @@ func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, o return nil, errors.Errorf("invalid length %d", length) } - req, err := http.NewRequest("GET", b.Filename(h), nil) + req, err := http.NewRequestWithContext(ctx, "GET", b.Filename(h), nil) if err != nil { - return nil, errors.Wrap(err, "http.NewRequest") + return nil, errors.WithStack(err) } byteRange := fmt.Sprintf("bytes=%d-", offset) @@ -283,7 +280,7 @@ func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, o debug.Log("Load(%v) send range %v", h, byteRange) b.sem.GetToken() - resp, err := ctxhttp.Do(ctx, b.client, req) + resp, err := b.client.Do(req) b.sem.ReleaseToken() if err != nil { @@ -321,17 +318,17 @@ func (b *Backend) Stat(ctx context.Context, h restic.Handle) (restic.FileInfo, e return restic.FileInfo{}, backoff.Permanent(err) } - req, err := http.NewRequest(http.MethodHead, b.Filename(h), nil) + req, err := http.NewRequestWithContext(ctx, http.MethodHead, b.Filename(h), nil) if err != nil { - return restic.FileInfo{}, errors.Wrap(err, "NewRequest") + return restic.FileInfo{}, errors.WithStack(err) } req.Header.Set("Accept", ContentTypeV2) b.sem.GetToken() - resp, err := ctxhttp.Do(ctx, b.client, req) + resp, err := b.client.Do(req) b.sem.ReleaseToken() if err != nil { - return restic.FileInfo{}, errors.Wrap(err, "client.Head") + return restic.FileInfo{}, errors.WithStack(err) } _, _ = io.Copy(ioutil.Discard, resp.Body) @@ -376,14 +373,14 @@ func (b *Backend) Remove(ctx context.Context, h restic.Handle) error { return backoff.Permanent(err) } - req, err := http.NewRequest("DELETE", b.Filename(h), nil) + req, err := http.NewRequestWithContext(ctx, "DELETE", b.Filename(h), nil) if err != nil { - return errors.Wrap(err, "http.NewRequest") + return errors.WithStack(err) } req.Header.Set("Accept", ContentTypeV2) b.sem.GetToken() - resp, err := ctxhttp.Do(ctx, b.client, req) + resp, err := b.client.Do(req) b.sem.ReleaseToken() if err != nil { @@ -415,14 +412,14 @@ func (b *Backend) List(ctx context.Context, t restic.FileType, fn func(restic.Fi url += "/" } - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { - return errors.Wrap(err, "NewRequest") + return errors.WithStack(err) } req.Header.Set("Accept", ContentTypeV2) b.sem.GetToken() - resp, err := ctxhttp.Do(ctx, b.client, req) + resp, err := b.client.Do(req) b.sem.ReleaseToken() if err != nil { diff --git a/internal/selfupdate/github.go b/internal/selfupdate/github.go index 637f6765e..c5cc77286 100644 --- a/internal/selfupdate/github.go +++ b/internal/selfupdate/github.go @@ -10,7 +10,6 @@ import ( "time" "github.com/pkg/errors" - "golang.org/x/net/context/ctxhttp" ) // Release collects data about a single release on GitHub. @@ -53,7 +52,7 @@ func GitHubLatestRelease(ctx context.Context, owner, repo string) (Release, erro defer cancel() url := fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/latest", owner, repo) - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return Release{}, err } @@ -61,7 +60,7 @@ func GitHubLatestRelease(ctx context.Context, owner, repo string) (Release, erro // pin API version 3 req.Header.Set("Accept", "application/vnd.github.v3+json") - res, err := ctxhttp.Do(ctx, http.DefaultClient, req) + res, err := http.DefaultClient.Do(req) if err != nil { return Release{}, err } @@ -112,7 +111,7 @@ func GitHubLatestRelease(ctx context.Context, owner, repo string) (Release, erro } func getGithubData(ctx context.Context, url string) ([]byte, error) { - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, err } @@ -120,7 +119,7 @@ func getGithubData(ctx context.Context, url string) ([]byte, error) { // request binary data req.Header.Set("Accept", "application/octet-stream") - res, err := ctxhttp.Do(ctx, http.DefaultClient, req) + res, err := http.DefaultClient.Do(req) if err != nil { return nil, err } From 69223601798f7907c68fe7d5d59520bf2a6b693b Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sun, 9 Oct 2022 14:13:37 +0200 Subject: [PATCH 2/4] ui/backup: Remove unused ProgressReporter type, Progress field --- internal/ui/backup/progress.go | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/internal/ui/backup/progress.go b/internal/ui/backup/progress.go index a4b641fe9..d57a5308b 100644 --- a/internal/ui/backup/progress.go +++ b/internal/ui/backup/progress.go @@ -39,18 +39,6 @@ type fileWorkerMessage struct { done bool } -type ProgressReporter interface { - CompleteItem(item string, previous, current *restic.Node, s archiver.ItemStats, d time.Duration) - StartFile(filename string) - CompleteBlob(filename string, bytes uint64) - ScannerError(item string, err error) error - ReportTotal(item string, s archiver.ScanStats) - SetMinUpdatePause(d time.Duration) - Run(ctx context.Context) error - Error(item string, err error) error - Finish(snapshotID restic.ID) -} - type Summary struct { sync.Mutex Files, Dirs struct { @@ -69,8 +57,6 @@ type Progress struct { start time.Time dry bool - totalBytes uint64 - totalCh chan Counter processedCh chan Counter errCh chan struct{} @@ -130,7 +116,6 @@ func (p *Progress) Run(ctx context.Context) error { } else { // scan has finished p.totalCh = nil - p.totalBytes = total.Bytes } case s := <-p.processedCh: processed.Files += s.Files @@ -312,8 +297,7 @@ func (p *Progress) Finish(snapshotID restic.ID) { p.printer.Finish(snapshotID, p.start, p.summary, p.dry) } -// SetMinUpdatePause sets b.MinUpdatePause. It satisfies the -// ArchiveProgressReporter interface. +// SetMinUpdatePause sets b.MinUpdatePause. func (p *Progress) SetMinUpdatePause(d time.Duration) { p.MinUpdatePause = d } From e0b743c64d87ef9dbb10fec690055f947a6e9c44 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Fri, 14 Oct 2022 18:07:58 +0200 Subject: [PATCH 3/4] internal/restic: Remove unused ID.EqualString --- internal/restic/id.go | 13 ------------- internal/restic/id_test.go | 8 -------- 2 files changed, 21 deletions(-) diff --git a/internal/restic/id.go b/internal/restic/id.go index 6d85ed68a..5a25e0ebe 100644 --- a/internal/restic/id.go +++ b/internal/restic/id.go @@ -82,19 +82,6 @@ func (id ID) Equal(other ID) bool { return id == other } -// EqualString compares this ID to another one, given as a string. -func (id ID) EqualString(other string) (bool, error) { - s, err := hex.DecodeString(other) - if err != nil { - return false, errors.Wrap(err, "hex.DecodeString") - } - - id2 := ID{} - copy(id2[:], s) - - return id == id2, nil -} - // MarshalJSON returns the JSON encoding of id. func (id ID) MarshalJSON() ([]byte, error) { buf := make([]byte, 2+hex.EncodedLen(len(id))) diff --git a/internal/restic/id_test.go b/internal/restic/id_test.go index ff1dc54e0..9a9fddcda 100644 --- a/internal/restic/id_test.go +++ b/internal/restic/id_test.go @@ -30,14 +30,6 @@ func TestID(t *testing.T) { t.Errorf("ID.Equal() does not work as expected") } - ret, err := id.EqualString(test.id) - if err != nil { - t.Error(err) - } - if !ret { - t.Error("ID.EqualString() returned wrong value") - } - // test json marshalling buf, err := id.MarshalJSON() if err != nil { From 0e155fd9a65998c42afd43ec331dd21d3466d247 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Fri, 14 Oct 2022 17:40:49 +0200 Subject: [PATCH 4/4] internal/restic: Fix UID/GID parsing The helper function uidGidInt used strconv.ParseInt instead of ParseUint, so it silently ignored some invalid user/group IDs. Also, improve the error message. "Invalid UID" is more informative than having "ParseInt" twice (*strconv.NumError displays the function name). Finally, the user.User struct can be passed by pointer to get reduce code size. --- internal/restic/lock.go | 2 +- internal/restic/lock_unix.go | 18 +++++++----------- internal/restic/lock_windows.go | 2 +- internal/restic/snapshot.go | 2 +- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/internal/restic/lock.go b/internal/restic/lock.go index 13a422e71..e27990aed 100644 --- a/internal/restic/lock.go +++ b/internal/restic/lock.go @@ -126,7 +126,7 @@ func (l *Lock) fillUserInfo() error { } l.Username = usr.Username - l.UID, l.GID, err = uidGidInt(*usr) + l.UID, l.GID, err = uidGidInt(usr) return err } diff --git a/internal/restic/lock_unix.go b/internal/restic/lock_unix.go index dbf23fc6c..c11bc4ca7 100644 --- a/internal/restic/lock_unix.go +++ b/internal/restic/lock_unix.go @@ -9,25 +9,21 @@ import ( "strconv" "syscall" - "github.com/restic/restic/internal/errors" - "github.com/restic/restic/internal/debug" + "github.com/restic/restic/internal/errors" ) // uidGidInt returns uid, gid of the user as a number. -func uidGidInt(u user.User) (uid, gid uint32, err error) { - var ui, gi int64 - ui, err = strconv.ParseInt(u.Uid, 10, 32) +func uidGidInt(u *user.User) (uid, gid uint32, err error) { + ui, err := strconv.ParseUint(u.Uid, 10, 32) if err != nil { - return uid, gid, errors.Wrap(err, "ParseInt") + return 0, 0, errors.Errorf("invalid UID %q", u.Uid) } - gi, err = strconv.ParseInt(u.Gid, 10, 32) + gi, err := strconv.ParseUint(u.Gid, 10, 32) if err != nil { - return uid, gid, errors.Wrap(err, "ParseInt") + return 0, 0, errors.Errorf("invalid GID %q", u.Gid) } - uid = uint32(ui) - gid = uint32(gi) - return + return uint32(ui), uint32(gi), nil } // checkProcess will check if the process retaining the lock diff --git a/internal/restic/lock_windows.go b/internal/restic/lock_windows.go index 5697b6efb..ee24e3bca 100644 --- a/internal/restic/lock_windows.go +++ b/internal/restic/lock_windows.go @@ -8,7 +8,7 @@ import ( ) // uidGidInt always returns 0 on Windows, since uid isn't numbers -func uidGidInt(u user.User) (uid, gid uint32, err error) { +func uidGidInt(u *user.User) (uid, gid uint32, err error) { return 0, 0, nil } diff --git a/internal/restic/snapshot.go b/internal/restic/snapshot.go index 9eb7ab3ab..7f8d4164b 100644 --- a/internal/restic/snapshot.go +++ b/internal/restic/snapshot.go @@ -154,7 +154,7 @@ func (sn *Snapshot) fillUserInfo() error { sn.Username = usr.Username // set userid and groupid - sn.UID, sn.GID, err = uidGidInt(*usr) + sn.UID, sn.GID, err = uidGidInt(usr) return err }