Introduce noteCloseFailure, use it for reporting of cleanup errors

Note that this is a behavior change: we used to do
    retErr = errors.Wrapf(retErr, ..., closeErr)
which doesn't record closeErr if retErr was nil.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

cmd/skopeo/copy.go

@@ -130,7 +130,7 @@ func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) {
 	}
 	defer func() {
 		if err := policyContext.Destroy(); err != nil {
-			retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr)
+			retErr = noteCloseFailure(retErr, "error tearing down policy context", err)
 		}
 	}()
 

cmd/skopeo/inspect.go

@@ -105,7 +105,7 @@ func (opts *inspectOptions) run(args []string, stdout io.Writer) (retErr error)
 
 	defer func() {
 		if err := src.Close(); err != nil {
-			retErr = errors.Wrapf(retErr, fmt.Sprintf("(could not close image: %v) ", err))
+			retErr = noteCloseFailure(retErr, "could not close image", err)
 		}
 	}()
 

cmd/skopeo/layers.go

@@ -86,7 +86,7 @@ func (opts *layersOptions) run(args []string, stdout io.Writer) (retErr error) {
 	}
 	defer func() {
 		if err := src.Close(); err != nil {
-			retErr = errors.Wrapf(retErr, " (close error: %v)", err)
+			retErr = noteCloseFailure(retErr, "close error", err)
 		}
 	}()
 
@@ -136,7 +136,7 @@ func (opts *layersOptions) run(args []string, stdout io.Writer) (retErr error) {
 
 	defer func() {
 		if err := dest.Close(); err != nil {
-			retErr = errors.Wrapf(retErr, " (close error: %v)", err)
+			retErr = noteCloseFailure(retErr, "close error", err)
 		}
 	}()
 

cmd/skopeo/sync.go

@@ -514,7 +514,7 @@ func (opts *syncOptions) run(args []string, stdout io.Writer) (retErr error) {
 	}
 	defer func() {
 		if err := policyContext.Destroy(); err != nil {
-			retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr)
+			retErr = noteCloseFailure(retErr, "error tearing down policy context", err)
 		}
 	}()
 

cmd/skopeo/utils.go

@@ -25,6 +25,27 @@ type errorShouldDisplayUsage struct {
 	error
 }
 
+// noteCloseFailure returns (possibly-nil) err modified to account for (non-nil) closeErr.
+// The error for closeErr is annotated with description (which is not a format string)
+// Typical usage:
+//
+// defer func() {
+//     if err := something.Close(); err != nil {
+//         returnedErr = noteCloseFailure(returnedErr, "closing something", err)
+//     }
+// }
+func noteCloseFailure(err error, description string, closeErr error) error {
+	// We don’t accept a Closer() and close it ourselves because signature.PolicyContext has .Destroy(), not .Close().
+	// This also makes it harder for a caller to do
+	//     defer noteCloseFailure(returnedErr, …)
+	// which doesn’t use the right value of returnedErr, and doesn’t update it.
+	if err == nil {
+		return fmt.Errorf("%s: %w", description, closeErr)
+	}
+	// In this case we prioritize the primary error for use with %w; closeErr is usually less relevant, or might be a consequence of the primary erorr.
+	return fmt.Errorf("%w (%s: %v)", err, description, closeErr)
+}
+
 // commandAction intermediates between the RunE interface and the real handler,
 // primarily to ensure that cobra.Command is not available to the handler, which in turn
 // makes sure that the cmd.Flags() etc. flag access functions are not used,

cmd/skopeo/utils_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/containers/image/v5/manifest"
@@ -12,6 +13,27 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestNoteCloseFailure(t *testing.T) {
+	const description = "description"
+
+	mainErr := errors.New("main")
+	closeErr := errors.New("closing")
+
+	// Main success, closing failed
+	res := noteCloseFailure(nil, description, closeErr)
+	require.NotNil(t, res)
+	assert.Contains(t, res.Error(), description)
+	assert.Contains(t, res.Error(), closeErr.Error())
+
+	// Both main and closing failed
+	res = noteCloseFailure(mainErr, description, closeErr)
+	require.NotNil(t, res)
+	assert.Contains(t, res.Error(), mainErr.Error())
+	assert.Contains(t, res.Error(), description)
+	assert.Contains(t, res.Error(), closeErr.Error())
+	assert.ErrorIs(t, res, mainErr)
+}
+
 // fakeGlobalOptions creates globalOptions and sets it according to flags.
 func fakeGlobalOptions(t *testing.T, flags []string) (*globalOptions, *cobra.Command) {
 	app, opts := createApp()