From 693de29e37303f9e70bcc8b40e741235da0d2643 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 15 Mar 2022 21:31:51 +0100 Subject: [PATCH] Add various missing error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... as found by (golangci-lint run). Note: this does not add (golangci-lint run) to the Makefile to ensure the coding standard. (BTW golangci-lint currently fails on structcheck, which doesn't handle embedded structs, and that's a years-long known unfixed limitation.) Signed-off-by: Miloslav Trmač --- cmd/skopeo/copy.go | 8 ++++++-- cmd/skopeo/proxy.go | 8 ++++++-- cmd/skopeo/sync.go | 8 ++++++-- cmd/skopeo/utils.go | 2 +- integration/check_test.go | 4 ++-- integration/copy_test.go | 4 ++-- integration/openshift.go | 10 +++++++--- integration/proxy_test.go | 3 +++ integration/registry.go | 9 ++++++--- integration/sync_test.go | 20 +++++++++++++------- 10 files changed, 52 insertions(+), 24 deletions(-) diff --git a/cmd/skopeo/copy.go b/cmd/skopeo/copy.go index 52b269f8..6f1452a4 100644 --- a/cmd/skopeo/copy.go +++ b/cmd/skopeo/copy.go @@ -110,7 +110,7 @@ func parseMultiArch(multiArch string) (copy.ImageListSelection, error) { } } -func (opts *copyOptions) run(args []string, stdout io.Writer) error { +func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) { if len(args) != 2 { return errorShouldDisplayUsage{errors.New("Exactly two arguments expected")} } @@ -125,7 +125,11 @@ func (opts *copyOptions) run(args []string, stdout io.Writer) error { if err != nil { return fmt.Errorf("Error loading trust policy: %v", err) } - defer policyContext.Destroy() + defer func() { + if err := policyContext.Destroy(); err != nil { + retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr) + } + }() srcRef, err := alltransports.ParseImageName(imageNames[0]) if err != nil { diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 8556e7d7..0a0cc5ab 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -722,13 +722,17 @@ func (opts *proxyOptions) run(args []string, stdout io.Writer) error { var req request if err := json.Unmarshal(readbuf, &req); err != nil { rb := replyBuf{} - rb.send(conn, fmt.Errorf("invalid request: %v", err)) + if err := rb.send(conn, fmt.Errorf("invalid request: %v", err)); err != nil { + return fmt.Errorf("writing to socket: %w", err) + } } rb, terminate, err := handler.processRequest(req) if terminate { return nil } - rb.send(conn, err) + if err := rb.send(conn, err); err != nil { + return fmt.Errorf("writing to socket: %w", err) + } } } diff --git a/cmd/skopeo/sync.go b/cmd/skopeo/sync.go index 159dfecd..a7dd4814 100644 --- a/cmd/skopeo/sync.go +++ b/cmd/skopeo/sync.go @@ -500,7 +500,7 @@ func imagesToCopy(source string, transport string, sourceCtx *types.SystemContex return descriptors, nil } -func (opts *syncOptions) run(args []string, stdout io.Writer) error { +func (opts *syncOptions) run(args []string, stdout io.Writer) (retErr error) { if len(args) != 2 { return errorShouldDisplayUsage{errors.New("Exactly two arguments expected")} } @@ -510,7 +510,11 @@ func (opts *syncOptions) run(args []string, stdout io.Writer) error { if err != nil { return errors.Wrapf(err, "Error loading trust policy") } - defer policyContext.Destroy() + defer func() { + if err := policyContext.Destroy(); err != nil { + retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr) + } + }() // validate source and destination options contains := func(val string, list []string) (_ bool) { diff --git a/cmd/skopeo/utils.go b/cmd/skopeo/utils.go index 15450562..2e7b65b6 100644 --- a/cmd/skopeo/utils.go +++ b/cmd/skopeo/utils.go @@ -34,7 +34,7 @@ func commandAction(handler func(args []string, stdout io.Writer) error) func(cmd return func(c *cobra.Command, args []string) error { err := handler(args, c.OutOrStdout()) if _, ok := err.(errorShouldDisplayUsage); ok { - c.Help() + return c.Help() } return err } diff --git a/integration/check_test.go b/integration/check_test.go index 64046004..1ff25649 100644 --- a/integration/check_test.go +++ b/integration/check_test.go @@ -36,12 +36,12 @@ func (s *SkopeoSuite) SetUpSuite(c *check.C) { func (s *SkopeoSuite) TearDownSuite(c *check.C) { if s.regV2 != nil { - s.regV2.Close() + s.regV2.tearDown(c) } if s.regV2WithAuth != nil { //cmd := exec.Command("docker", "logout", s.regV2WithAuth) //c.Assert(cmd.Run(), check.IsNil) - s.regV2WithAuth.Close() + s.regV2WithAuth.tearDown(c) } } diff --git a/integration/copy_test.go b/integration/copy_test.go index 13f2020f..c3a55a5c 100644 --- a/integration/copy_test.go +++ b/integration/copy_test.go @@ -86,10 +86,10 @@ func (s *CopySuite) TearDownSuite(c *check.C) { os.RemoveAll(s.gpgHome) } if s.registry != nil { - s.registry.Close() + s.registry.tearDown(c) } if s.s1Registry != nil { - s.s1Registry.Close() + s.s1Registry.tearDown(c) } if s.cluster != nil { s.cluster.tearDown(c) diff --git a/integration/openshift.go b/integration/openshift.go index 4c7d9e6c..0264e9f1 100644 --- a/integration/openshift.go +++ b/integration/openshift.go @@ -258,12 +258,16 @@ func (cluster *openshiftCluster) relaxImageSignerPermissions(c *check.C) { // tearDown stops the cluster services and deletes (only some!) of the state. func (cluster *openshiftCluster) tearDown(c *check.C) { for i := len(cluster.processes) - 1; i >= 0; i-- { - cluster.processes[i].Process.Kill() + // It’s undocumented what Kill() returns if the process has terminated, + // so we couldn’t check just for that. This is running in a container anyway… + _ = cluster.processes[i].Process.Kill() } if cluster.workingDir != "" { - os.RemoveAll(cluster.workingDir) + err := os.RemoveAll(cluster.workingDir) + c.Assert(err, check.IsNil) } if cluster.dockerDir != "" { - os.RemoveAll(cluster.dockerDir) + err := os.RemoveAll(cluster.dockerDir) + c.Assert(err, check.IsNil) } } diff --git a/integration/proxy_test.go b/integration/proxy_test.go index e37e2767..3a7fc11c 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -285,6 +285,9 @@ func runTestGetManifestAndConfig(p *proxy, img string) error { } _, err = p.callNoFd("CloseImage", []interface{}{imgid}) + if err != nil { + return err + } return nil } diff --git a/integration/registry.go b/integration/registry.go index 2a510351..e2ec22c5 100644 --- a/integration/registry.go +++ b/integration/registry.go @@ -126,7 +126,10 @@ func (t *testRegistryV2) Ping() error { return nil } -func (t *testRegistryV2) Close() { - t.cmd.Process.Kill() - os.RemoveAll(t.dir) +func (t *testRegistryV2) tearDown(c *check.C) { + // It’s undocumented what Kill() returns if the process has terminated, + // so we couldn’t check just for that. This is running in a container anyway… + _ = t.cmd.Process.Kill() + err := os.RemoveAll(t.dir) + c.Assert(err, check.IsNil) } diff --git a/integration/sync_test.go b/integration/sync_test.go index 2465a2ff..d7102c17 100644 --- a/integration/sync_test.go +++ b/integration/sync_test.go @@ -100,7 +100,7 @@ func (s *SyncSuite) TearDownSuite(c *check.C) { os.RemoveAll(s.gpgHome) } if s.registry != nil { - s.registry.Close() + s.registry.tearDown(c) } if s.cluster != nil { s.cluster.tearDown(c) @@ -278,7 +278,8 @@ func (s *SyncSuite) TestYamlUntagged(c *check.C) { // sync to the local registry yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "docker", "--dest-tls-verify=false", yamlFile, v2DockerRegistryURL) // sync back from local registry to a folder os.Remove(yamlFile) @@ -289,7 +290,8 @@ func (s *SyncSuite) TestYamlUntagged(c *check.C) { %s: [] `, v2DockerRegistryURL, imagePath) - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) sysCtx = types.SystemContext{ @@ -334,7 +336,8 @@ k8s.gcr.io: c.Assert(nTags, check.Not(check.Equals), 0) yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) nManifests := 0 @@ -365,7 +368,8 @@ k8s.gcr.io: - sha256:59eec8837a4d942cc19a52b8c09ea75121acc38114a2c68b98983ce9356b8610 ` yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) nManifests := 0 @@ -417,7 +421,8 @@ quay.io: c.Assert(nTags, check.Not(check.Equals), 0) yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) nManifests := 0 @@ -481,7 +486,8 @@ func (s *SyncSuite) TestYamlTLSVerify(c *check.C) { for _, cfg := range testCfg { yamlConfig := fmt.Sprintf(yamlTemplate, v2DockerRegistryURL, cfg.tlsVerify, image, tag) yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) cfg.checker(c, cfg.msg, "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) os.Remove(yamlFile)