Merge pull request #21977 from mtrmac/close-error-handling

Close error handling

pkg/machine/compression/decompress.go

@@ -1,7 +1,6 @@
 package compression
 
 import (
-	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -25,7 +24,7 @@ type decompressor interface {
 	compressedFileSize() int64
 	compressedFileMode() os.FileMode
 	compressedFileReader() (io.ReadCloser, error)
-	decompress(w WriteSeekCloser, r io.Reader) error
+	decompress(w io.WriteSeeker, r io.Reader) error
 	close()
 }
 
@@ -72,7 +71,7 @@ func newDecompressor(compressedFilePath string, compressedFileContent []byte) (d
 	}
 }
 
-func runDecompression(d decompressor, decompressedFilePath string) error {
+func runDecompression(d decompressor, decompressedFilePath string) (retErr error) {
 	compressedFileReader, err := d.compressedFileReader()
 	if err != nil {
 		return err
@@ -99,8 +98,11 @@ func runDecompression(d decompressor, decompressedFilePath string) error {
 		return err
 	}
 	defer func() {
-		if err := decompressedFileWriter.Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+		if err := decompressedFileWriter.Close(); err != nil {
 			logrus.Warnf("Unable to to close destination file %s: %q", decompressedFilePath, err)
+			if retErr == nil {
+				retErr = err
+			}
 		}
 	}()
 

pkg/machine/compression/generic.go

@@ -43,7 +43,7 @@ func (d *genericDecompressor) compressedFileReader() (io.ReadCloser, error) {
 	return compressedFile, nil
 }
 
-func (d *genericDecompressor) decompress(w WriteSeekCloser, r io.Reader) error {
+func (d *genericDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	decompressedFileReader, _, err := compression.AutoDecompress(r)
 	if err != nil {
 		return err
@@ -64,7 +64,7 @@ func (d *genericDecompressor) close() {
 	}
 }
 
-func (d *genericDecompressor) sparseOptimizedCopy(w WriteSeekCloser, r io.Reader) error {
+func (d *genericDecompressor) sparseOptimizedCopy(w io.WriteSeeker, r io.Reader) error {
 	var err error
 	sparseWriter := NewSparseWriter(w)
 	defer func() {

pkg/machine/compression/gzip.go

@@ -16,7 +16,7 @@ func newGzipDecompressor(compressedFilePath string) (*gzipDecompressor, error) {
 	return &gzipDecompressor{*d}, err
 }
 
-func (d *gzipDecompressor) decompress(w WriteSeekCloser, r io.Reader) error {
+func (d *gzipDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	gzReader, err := image.GzipDecompressor(r)
 	if err != nil {
 		return err

pkg/machine/compression/sparse_file_writer.go

@@ -14,14 +14,18 @@ type WriteSeekCloser interface {
 }
 
 type sparseWriter struct {
-	file WriteSeekCloser
+	file io.WriteSeeker
 	// Invariant between method calls:
 	// The contents of the file match the contents passed to Write, except that pendingZeroes trailing zeroes have not been written.
 	// Also, the data that _has_ been written does not end with a zero byte (i.e. pendingZeroes is the largest possible value.
 	pendingZeroes int64
 }
 
-func NewSparseWriter(file WriteSeekCloser) *sparseWriter {
+// NewSparseWriter returns a WriteCloser for underlying file which creates
+// holes where appropriate.
+// NOTE: The caller must .Close() both the returned sparseWriter AND the underlying file,
+// in that order.
+func NewSparseWriter(file io.WriteSeeker) *sparseWriter {
 	return &sparseWriter{
 		file:          file,
 		pendingZeroes: 0,
@@ -121,18 +125,15 @@ func (sw *sparseWriter) Close() error {
 	if sw.pendingZeroes != 0 {
 		if holeSize := sw.pendingZeroes - 1; holeSize >= zerosThreshold {
 			if err := sw.createHole(holeSize); err != nil {
-				sw.file.Close()
 				return err
 			}
 			sw.pendingZeroes -= holeSize
 		}
 		var zeroArray [zerosThreshold]byte
 		if _, err := sw.file.Write(zeroArray[:sw.pendingZeroes]); err != nil {
-			sw.file.Close()
 			return err
 		}
 	}
-	err := sw.file.Close()
 	sw.file = nil
-	return err
+	return nil
 }

pkg/machine/compression/sparse_file_writer_test.go

@@ -58,10 +58,6 @@ func (m *memorySparseFile) Write(b []byte) (n int, err error) {
 	return n, err
 }
 
-func (m *memorySparseFile) Close() error {
-	return nil
-}
-
 func testInputWithWriteLen(t *testing.T, input []byte, minSparse int64, chunkSize int) {
 	m := &memorySparseFile{}
 	sparseWriter := NewSparseWriter(m)

pkg/machine/compression/uncompressed.go

@@ -13,6 +13,6 @@ func newUncompressedDecompressor(compressedFilePath string) (*uncompressedDecomp
 	return &uncompressedDecompressor{*d}, err
 }
 
-func (d *uncompressedDecompressor) decompress(w WriteSeekCloser, r io.Reader) error {
+func (d *uncompressedDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	return d.sparseOptimizedCopy(w, r)
 }

pkg/machine/compression/xz.go

@@ -22,7 +22,7 @@ func newXzDecompressor(compressedFilePath string) (*xzDecompressor, error) {
 // Will error out if file without .Xz already exists
 // Maybe extracting then renaming is a good idea here..
 // depends on Xz: not pre-installed on mac, so it becomes a brew dependency
-func (*xzDecompressor) decompress(w WriteSeekCloser, r io.Reader) error {
+func (*xzDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	var cmd *exec.Cmd
 	var read io.Reader
 

pkg/machine/compression/zip.go

@@ -40,7 +40,7 @@ func (d *zipDecompressor) compressedFileReader() (io.ReadCloser, error) {
 	return z, nil
 }
 
-func (*zipDecompressor) decompress(w WriteSeekCloser, r io.Reader) error {
+func (*zipDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	_, err := io.Copy(w, r)
 	return err
 }

pkg/machine/compression/zstd.go

@@ -15,7 +15,7 @@ func newZstdDecompressor(compressedFilePath string) (*zstdDecompressor, error) {
 	return &zstdDecompressor{*d}, err
 }
 
-func (d *zstdDecompressor) decompress(w WriteSeekCloser, r io.Reader) error {
+func (d *zstdDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	zstdReader, err := zstd.NewReader(r)
 	if err != nil {
 		return err

pkg/machine/e2e/machine_test.go

@@ -1,10 +1,8 @@
 package e2e_test
 
 import (
-	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -121,9 +119,8 @@ func setup() (string, *machineTestBuilder) {
 		Fail(fmt.Sprintf("failed to create file %s: %q", mb.imagePath, err))
 	}
 	defer func() {
-		closeErr := dest.Close()
+		if err := dest.Close(); err != nil {
-		if err != nil || !errors.Is(closeErr, fs.ErrClosed) {
+			Fail(fmt.Sprintf("failed to close destination file %q: %q\n", dest.Name(), err))
-			fmt.Printf("failed to close destination file %q: %q\n", dest.Name(), err)
 		}
 	}()
 	fmt.Printf("--> copying %q to %q\n", src.Name(), dest.Name())
@@ -161,26 +158,14 @@ func teardown(origHomeDir string, testDir string, mb *machineTestBuilder) {
 	}
 }
 
-// copySparseFile is a helper method for tests only.  copies a file sparsely
-// between two string inputs
-func copySparseFile(src, dst string) error { //nolint:unused
-	dstWriter, err := os.OpenFile(dst, os.O_CREATE|os.O_RDWR, 0600)
-	if err != nil {
-		return err
-	}
-	dstWriter.Close()
-	fSrc, err := os.Open(src)
-	if err != nil {
-		return err
-	}
-	defer fSrc.Close()
-	return copySparse(dstWriter, fSrc)
-}
-
 // copySparse is a helper method for tests only; caller is responsible for closures
-func copySparse(dst compression.WriteSeekCloser, src io.Reader) error {
+func copySparse(dst io.WriteSeeker, src io.Reader) (retErr error) {
 	spWriter := compression.NewSparseWriter(dst)
-	defer spWriter.Close()
+	defer func() {
+		if err := spWriter.Close(); err != nil && retErr == nil {
+			retErr = err
+		}
+	}()
 	_, err := io.Copy(spWriter, src)
 	return err
 }