From 33ad61a09048a88ded42d7bd8bb9db7800ce5adf Mon Sep 17 00:00:00 2001
From: Erik Hollensbe <github@hollensbe.org>
Date: Sun, 5 Feb 2017 23:13:00 -0800
Subject: [PATCH] Close with error in all interfaces

This allows us to provide in the image interfaces a method of providing
an error at close time. This is only currently used in a few situations.

Signed-off-by: Erik Hollensbe <github@hollensbe.org>
---
 copy/copy.go                             | 45 ++++++++++++++++--------
 directory/directory_dest.go              |  3 +-
 directory/directory_src.go               |  3 +-
 docker/daemon/daemon_dest.go             |  8 +++--
 docker/daemon/daemon_src.go              |  4 +--
 docker/docker_image_dest.go              |  3 +-
 docker/docker_image_src.go               |  3 +-
 image/docker_schema2_test.go             |  4 +--
 image/memory.go                          |  3 +-
 image/unparsed.go                        |  4 +--
 oci/layout/oci_dest.go                   |  3 +-
 oci/layout/oci_src.go                    |  3 +-
 openshift/openshift.go                   | 12 ++++---
 signature/policy_reference_match_test.go |  4 +--
 storage/storage_image.go                 |  6 ++--
 types/types.go                           |  6 ++--
 16 files changed, 74 insertions(+), 40 deletions(-)

diff --git a/copy/copy.go b/copy/copy.go
index e03fd5c987..e13152c3d3 100644
--- a/copy/copy.go
+++ b/copy/copy.go
@@ -13,7 +13,6 @@ import (
 	"github.com/Sirupsen/logrus"
 	"github.com/containers/image/image"
 	"github.com/containers/image/manifest"
-	"github.com/containers/image/pkg/compression"
 	"github.com/containers/image/signature"
 	"github.com/containers/image/transports"
 	"github.com/containers/image/types"
@@ -95,8 +94,15 @@ type Options struct {
 	DestinationCtx   *types.SystemContext
 }
 
-// Image copies image from srcRef to destRef, using policyContext to validate source image admissibility.
-func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageReference, options *Options) error {
+// Image copies image from srcRef to destRef, using policyContext to validate
+// source image admissibility.
+func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageReference, options *Options) (retErr error) {
+	// NOTE this function uses an output parameter for the error return value.
+	// Setting this and returning is the ideal way to return an error.
+	//
+	// the defers in this routine will wrap the error return with its own errors
+	// which can be valuable context in the middle of a multi-streamed copy.
+
 	reportWriter := ioutil.Discard
 	if options != nil && options.ReportWriter != nil {
 		reportWriter = options.ReportWriter
@@ -109,7 +115,12 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 	if err != nil {
 		return errors.Wrapf(err, "Error initializing destination %s", transports.ImageName(destRef))
 	}
-	defer dest.Close()
+	defer func() {
+		if err := dest.Close(); err != nil {
+			retErr = errors.Wrapf(retErr, " (dest: %v)", err)
+		}
+	}()
+
 	destSupportedManifestMIMETypes := dest.SupportedManifestMIMETypes()
 
 	rawSource, err := srcRef.NewImageSource(options.SourceCtx, destSupportedManifestMIMETypes)
@@ -119,7 +130,9 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 	unparsedImage := image.UnparsedFromSource(rawSource)
 	defer func() {
 		if unparsedImage != nil {
-			unparsedImage.Close()
+			if err := unparsedImage.Close(); err != nil {
+				retErr = errors.Wrapf(retErr, " (unparsed: %v)", err)
+			}
 		}
 	}()
 
@@ -129,10 +142,14 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 	}
 	src, err := image.FromUnparsedImage(unparsedImage)
 	if err != nil {
-		return errors.Wrapf(err, "Error initializing image from source %s", transports.ImageName(srcRef))
+		retErr = errors.Wrapf(err, "Error initializing image from source %s", transports.ImageName(srcRef))
 	}
 	unparsedImage = nil
-	defer src.Close()
+	defer func() {
+		if err := src.Close(); err != nil {
+			retErr = errors.Wrapf(retErr, " (source: %v)", err)
+		}
+	}()
 
 	if src.IsMultiImage() {
 		return errors.Errorf("can not copy %s: manifest contains multiple images", transports.ImageName(srcRef))
@@ -371,7 +388,7 @@ func (ic *imageCopier) copyLayer(srcInfo types.BlobInfo) (types.BlobInfo, digest
 // and returns a complete blobInfo of the copied blob and perhaps a <-chan diffIDResult if diffIDIsNeeded, to be read by the caller.
 func (ic *imageCopier) copyLayerFromStream(srcStream io.Reader, srcInfo types.BlobInfo,
 	diffIDIsNeeded bool) (types.BlobInfo, <-chan diffIDResult, error) {
-	var getDiffIDRecorder func(compression.DecompressorFunc) io.Writer // = nil
+	var getDiffIDRecorder func(decompressorFunc) io.Writer // = nil
 	var diffIDChan chan diffIDResult
 
 	err := errors.New("Internal error: unexpected panic in copyLayer") // For pipeWriter.CloseWithError below
@@ -382,7 +399,7 @@ func (ic *imageCopier) copyLayerFromStream(srcStream io.Reader, srcInfo types.Bl
 			pipeWriter.CloseWithError(err) // CloseWithError(nil) is equivalent to Close()
 		}()
 
-		getDiffIDRecorder = func(decompressor compression.DecompressorFunc) io.Writer {
+		getDiffIDRecorder = func(decompressor decompressorFunc) io.Writer {
 			// If this fails, e.g. because we have exited and due to pipeWriter.CloseWithError() above further
 			// reading from the pipe has failed, we don’t really care.
 			// We only read from diffIDChan if the rest of the flow has succeeded, and when we do read from it,
@@ -400,7 +417,7 @@ func (ic *imageCopier) copyLayerFromStream(srcStream io.Reader, srcInfo types.Bl
 }
 
 // diffIDComputationGoroutine reads all input from layerStream, uncompresses using decompressor if necessary, and sends its digest, and status, if any, to dest.
-func diffIDComputationGoroutine(dest chan<- diffIDResult, layerStream io.ReadCloser, decompressor compression.DecompressorFunc) {
+func diffIDComputationGoroutine(dest chan<- diffIDResult, layerStream io.ReadCloser, decompressor decompressorFunc) {
 	result := diffIDResult{
 		digest: "",
 		err:    errors.New("Internal error: unexpected panic in diffIDComputationGoroutine"),
@@ -412,7 +429,7 @@ func diffIDComputationGoroutine(dest chan<- diffIDResult, layerStream io.ReadClo
 }
 
 // computeDiffID reads all input from layerStream, uncompresses it using decompressor if necessary, and returns its digest.
-func computeDiffID(stream io.Reader, decompressor compression.DecompressorFunc) (digest.Digest, error) {
+func computeDiffID(stream io.Reader, decompressor decompressorFunc) (digest.Digest, error) {
 	if decompressor != nil {
 		s, err := decompressor(stream)
 		if err != nil {
@@ -429,7 +446,7 @@ func computeDiffID(stream io.Reader, decompressor compression.DecompressorFunc)
 // perhaps compressing it if canCompress,
 // and returns a complete blobInfo of the copied blob.
 func (ic *imageCopier) copyBlobFromStream(srcStream io.Reader, srcInfo types.BlobInfo,
-	getOriginalLayerCopyWriter func(decompressor compression.DecompressorFunc) io.Writer,
+	getOriginalLayerCopyWriter func(decompressor decompressorFunc) io.Writer,
 	canCompress bool) (types.BlobInfo, error) {
 	// The copying happens through a pipeline of connected io.Readers.
 	// === Input: srcStream
@@ -447,8 +464,8 @@ func (ic *imageCopier) copyBlobFromStream(srcStream io.Reader, srcInfo types.Blo
 	var destStream io.Reader = digestingReader
 
 	// === Detect compression of the input stream.
-	// This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression.
-	decompressor, destStream, err := compression.DetectCompression(destStream) // We could skip this in some cases, but let's keep the code path uniform
+	// This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by detectCompression.
+	decompressor, destStream, err := detectCompression(destStream) // We could skip this in some cases, but let's keep the code path uniform
 	if err != nil {
 		return types.BlobInfo{}, errors.Wrapf(err, "Error reading blob %s", srcInfo.Digest)
 	}
diff --git a/directory/directory_dest.go b/directory/directory_dest.go
index e0b0fe452e..c310d6624b 100644
--- a/directory/directory_dest.go
+++ b/directory/directory_dest.go
@@ -26,7 +26,8 @@ func (d *dirImageDestination) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageDestination, if any.
-func (d *dirImageDestination) Close() {
+func (d *dirImageDestination) Close() error {
+	return nil
 }
 
 func (d *dirImageDestination) SupportedManifestMIMETypes() []string {
diff --git a/directory/directory_src.go b/directory/directory_src.go
index 8053c1896e..d432fb72ed 100644
--- a/directory/directory_src.go
+++ b/directory/directory_src.go
@@ -28,7 +28,8 @@ func (s *dirImageSource) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageSource, if any.
-func (s *dirImageSource) Close() {
+func (s *dirImageSource) Close() error {
+	return nil
 }
 
 // GetManifest returns the image's manifest along with its MIME type (which may be empty when it can't be determined but the manifest is available).
diff --git a/docker/daemon/daemon_dest.go b/docker/daemon/daemon_dest.go
index dd2d725bca..a6cee88a57 100644
--- a/docker/daemon/daemon_dest.go
+++ b/docker/daemon/daemon_dest.go
@@ -91,9 +91,11 @@ func imageLoadGoroutine(ctx context.Context, c *client.Client, reader *io.PipeRe
 }
 
 // Close removes resources associated with an initialized ImageDestination, if any.
-func (d *daemonImageDestination) Close() {
+func (d *daemonImageDestination) Close() error {
+	var err error
 	if !d.committed {
 		logrus.Debugf("docker-daemon: Closing tar stream to abort loading")
+		err = errors.New("Aborting upload, daemonImageDestination closed without a previous .Commit()")
 		// In principle, goroutineCancel() should abort the HTTP request and stop the process from continuing.
 		// In practice, though, various HTTP implementations used by client.Client.ImageLoad() (including
 		// https://github.com/golang/net/blob/master/context/ctxhttp/ctxhttp_pre17.go and the
@@ -104,9 +106,11 @@ func (d *daemonImageDestination) Close() {
 		// immediately, and hopefully, through terminating the sending which uses "Transfer-Encoding: chunked"" without sending
 		// the terminating zero-length chunk, prevent the docker daemon from processing the tar stream at all.
 		// Whether that works or not, closing the PipeWriter seems desirable in any case.
-		d.writer.CloseWithError(errors.New("Aborting upload, daemonImageDestination closed without a previous .Commit()"))
+		d.writer.CloseWithError(err)
 	}
 	d.goroutineCancel()
+
+	return err
 }
 
 // Reference returns the reference used to set up this destination.  Note that this should directly correspond to user's intent,
diff --git a/docker/daemon/daemon_src.go b/docker/daemon/daemon_src.go
index 79042852ae..256fd2da49 100644
--- a/docker/daemon/daemon_src.go
+++ b/docker/daemon/daemon_src.go
@@ -92,8 +92,8 @@ func (s *daemonImageSource) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageSource, if any.
-func (s *daemonImageSource) Close() {
-	_ = os.Remove(s.tarCopyPath)
+func (s *daemonImageSource) Close() error {
+	return os.Remove(s.tarCopyPath)
 }
 
 // tarReadCloser is a way to close the backing file of a tar.Reader when the user no longer needs the tar component.
diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go
index 4c5ee1436c..f19e8cfd54 100644
--- a/docker/docker_image_dest.go
+++ b/docker/docker_image_dest.go
@@ -59,7 +59,8 @@ func (d *dockerImageDestination) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageDestination, if any.
-func (d *dockerImageDestination) Close() {
+func (d *dockerImageDestination) Close() error {
+	return nil
 }
 
 func (d *dockerImageDestination) SupportedManifestMIMETypes() []string {
diff --git a/docker/docker_image_src.go b/docker/docker_image_src.go
index e26d0f0b53..d84c31afcb 100644
--- a/docker/docker_image_src.go
+++ b/docker/docker_image_src.go
@@ -65,7 +65,8 @@ func (s *dockerImageSource) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageSource, if any.
-func (s *dockerImageSource) Close() {
+func (s *dockerImageSource) Close() error {
+	return nil
 }
 
 // simplifyContentType drops parameters from a HTTP media type (see https://tools.ietf.org/html/rfc7231#section-3.1.1.1)
diff --git a/image/docker_schema2_test.go b/image/docker_schema2_test.go
index 473e51e246..9d1ab7ee33 100644
--- a/image/docker_schema2_test.go
+++ b/image/docker_schema2_test.go
@@ -25,7 +25,7 @@ type unusedImageSource struct{}
 func (f unusedImageSource) Reference() types.ImageReference {
 	panic("Unexpected call to a mock function")
 }
-func (f unusedImageSource) Close() {
+func (f unusedImageSource) Close() error {
 	panic("Unexpected call to a mock function")
 }
 func (f unusedImageSource) GetManifest() ([]byte, string, error) {
@@ -346,7 +346,7 @@ type memoryImageDest struct {
 func (d *memoryImageDest) Reference() types.ImageReference {
 	return refImageReferenceMock{d.ref}
 }
-func (d *memoryImageDest) Close() {
+func (d *memoryImageDest) Close() error {
 	panic("Unexpected call to a mock function")
 }
 func (d *memoryImageDest) SupportedManifestMIMETypes() []string {
diff --git a/image/memory.go b/image/memory.go
index 1a3faa0207..304274d142 100644
--- a/image/memory.go
+++ b/image/memory.go
@@ -32,7 +32,8 @@ func (i *memoryImage) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized UnparsedImage, if any.
-func (i *memoryImage) Close() {
+func (i *memoryImage) Close() error {
+	return nil
 }
 
 // Size returns the size of the image as stored, if known, or -1 if not.
diff --git a/image/unparsed.go b/image/unparsed.go
index 0feb1101c5..446652e79f 100644
--- a/image/unparsed.go
+++ b/image/unparsed.go
@@ -36,8 +36,8 @@ func (i *UnparsedImage) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized UnparsedImage, if any.
-func (i *UnparsedImage) Close() {
-	i.src.Close()
+func (i *UnparsedImage) Close() error {
+	return i.src.Close()
 }
 
 // Manifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need.
diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go
index 06ae40ffa2..b665f87d8e 100644
--- a/oci/layout/oci_dest.go
+++ b/oci/layout/oci_dest.go
@@ -31,7 +31,8 @@ func (d *ociImageDestination) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageDestination, if any.
-func (d *ociImageDestination) Close() {
+func (d *ociImageDestination) Close() error {
+	return nil
 }
 
 func (d *ociImageDestination) SupportedManifestMIMETypes() []string {
diff --git a/oci/layout/oci_src.go b/oci/layout/oci_src.go
index 89acff0b57..6ae47c26b4 100644
--- a/oci/layout/oci_src.go
+++ b/oci/layout/oci_src.go
@@ -26,7 +26,8 @@ func (s *ociImageSource) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageSource, if any.
-func (s *ociImageSource) Close() {
+func (s *ociImageSource) Close() error {
+	return nil
 }
 
 // GetManifest returns the image's manifest along with its MIME type (which may be empty when it can't be determined but the manifest is available).
diff --git a/openshift/openshift.go b/openshift/openshift.go
index c1de090d63..9ab905e2bc 100644
--- a/openshift/openshift.go
+++ b/openshift/openshift.go
@@ -191,11 +191,15 @@ func (s *openshiftImageSource) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageSource, if any.
-func (s *openshiftImageSource) Close() {
+func (s *openshiftImageSource) Close() error {
 	if s.docker != nil {
-		s.docker.Close()
+		err := s.docker.Close()
 		s.docker = nil
+
+		return err
 	}
+
+	return nil
 }
 
 func (s *openshiftImageSource) GetTargetManifest(digest digest.Digest) ([]byte, string, error) {
@@ -329,8 +333,8 @@ func (d *openshiftImageDestination) Reference() types.ImageReference {
 }
 
 // Close removes resources associated with an initialized ImageDestination, if any.
-func (d *openshiftImageDestination) Close() {
-	d.docker.Close()
+func (d *openshiftImageDestination) Close() error {
+	return d.docker.Close()
 }
 
 func (d *openshiftImageDestination) SupportedManifestMIMETypes() []string {
diff --git a/signature/policy_reference_match_test.go b/signature/policy_reference_match_test.go
index 295e8339a0..da934f3d56 100644
--- a/signature/policy_reference_match_test.go
+++ b/signature/policy_reference_match_test.go
@@ -57,7 +57,7 @@ type refImageMock struct{ reference.Named }
 func (ref refImageMock) Reference() types.ImageReference {
 	return refImageReferenceMock{ref.Named}
 }
-func (ref refImageMock) Close() {
+func (ref refImageMock) Close() error {
 	panic("unexpected call to a mock function")
 }
 func (ref refImageMock) Manifest() ([]byte, string, error) {
@@ -322,7 +322,7 @@ type forbiddenImageMock struct{}
 func (ref forbiddenImageMock) Reference() types.ImageReference {
 	panic("unexpected call to a mock function")
 }
-func (ref forbiddenImageMock) Close() {
+func (ref forbiddenImageMock) Close() error {
 	panic("unexpected call to a mock function")
 }
 func (ref forbiddenImageMock) Manifest() ([]byte, string, error) {
diff --git a/storage/storage_image.go b/storage/storage_image.go
index fb7fe86f55..7930cb32eb 100644
--- a/storage/storage_image.go
+++ b/storage/storage_image.go
@@ -118,10 +118,12 @@ func (s storageImageDestination) Reference() types.ImageReference {
 	return s.imageRef
 }
 
-func (s storageImageSource) Close() {
+func (s storageImageSource) Close() error {
+	return nil
 }
 
-func (s storageImageDestination) Close() {
+func (s storageImageDestination) Close() error {
+	return nil
 }
 
 func (s storageImageDestination) ShouldCompressLayers() bool {
diff --git a/types/types.go b/types/types.go
index 2154f306f7..e05809a3d2 100644
--- a/types/types.go
+++ b/types/types.go
@@ -110,7 +110,7 @@ type ImageSource interface {
 	// (not as the image itself, or its underlying storage, claims).  This can be used e.g. to determine which public keys are trusted for this image.
 	Reference() ImageReference
 	// Close removes resources associated with an initialized ImageSource, if any.
-	Close()
+	Close() error
 	// GetManifest returns the image's manifest along with its MIME type (which may be empty when it can't be determined but the manifest is available).
 	// It may use a remote (= slow) service.
 	GetManifest() ([]byte, string, error)
@@ -138,7 +138,7 @@ type ImageDestination interface {
 	// e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects.
 	Reference() ImageReference
 	// Close removes resources associated with an initialized ImageDestination, if any.
-	Close()
+	Close() error
 
 	// SupportedManifestMIMETypes tells which manifest mime types the destination supports
 	// If an empty slice or nil it's returned, then any mime type can be tried to upload
@@ -184,7 +184,7 @@ type UnparsedImage interface {
 	// (not as the image itself, or its underlying storage, claims).  This can be used e.g. to determine which public keys are trusted for this image.
 	Reference() ImageReference
 	// Close removes resources associated with an initialized UnparsedImage, if any.
-	Close()
+	Close() error
 	// Manifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need.
 	Manifest() ([]byte, string, error)
 	// Signatures is like ImageSource.GetSignatures, but the result is cached; it is OK to call this however often you need.