Remove duplication and make consistent usage of the progress bar

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>

pkg/machine/compression/decompress.go

@@ -1,6 +1,7 @@
 package compression
 
 import (
+	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -14,19 +15,39 @@ import (
 )
 
 const (
-	zipExt            = ".zip"
-	progressBarPrefix = "Extracting compressed file"
-	macOs             = "darwin"
+	decompressedFileFlag = os.O_CREATE | os.O_TRUNC | os.O_WRONLY
+	macOs                = "darwin"
+	progressBarPrefix    = "Extracting compressed file"
+	zipExt               = ".zip"
 )
 
 type decompressor interface {
-	srcFilePath() string
-	reader() (io.Reader, error)
-	copy(w *os.File, r io.Reader) error
+	compressedFileSize() int64
+	compressedFileMode() os.FileMode
+	compressedFileReader() (io.ReadCloser, error)
+	decompress(w io.WriteSeeker, r io.Reader) error
 	close()
 }
 
-func newDecompressor(compressedFilePath string, compressedFileContent []byte) decompressor {
+func Decompress(compressedVMFile *define.VMFile, decompressedFilePath string) error {
+	compressedFilePath := compressedVMFile.GetPath()
+	// Are we reading full image file?
+	// Only few bytes are read to detect
+	// the compression type
+	compressedFileContent, err := compressedVMFile.Read()
+	if err != nil {
+		return err
+	}
+
+	var d decompressor
+	if d, err = newDecompressor(compressedFilePath, compressedFileContent); err != nil {
+		return err
+	}
+
+	return runDecompression(d, decompressedFilePath)
+}
+
+func newDecompressor(compressedFilePath string, compressedFileContent []byte) (decompressor, error) {
 	compressionType := archive.DetectCompression(compressedFileContent)
 	os := runtime.GOOS
 	hasZipSuffix := strings.HasSuffix(compressedFilePath, zipExt)
@@ -40,6 +61,10 @@ func newDecompressor(compressedFilePath string, compressedFileContent []byte) de
 		return newZipDecompressor(compressedFilePath)
 	case compressionType == archive.Uncompressed:
 		return newUncompressedDecompressor(compressedFilePath)
+	// macOS gzipped VM images are sparse. As a result a
+	// special decompressor is required: it uses crc os.CopySparse
+	// instead of io.Copy and std lib gzip instead of klauspost/pgzip
+	// (even if it's slower).
 	case compressionType == archive.Gzip && os == macOs:
 		return newGzipDecompressor(compressedFilePath)
 	default:
@@ -47,70 +72,42 @@ func newDecompressor(compressedFilePath string, compressedFileContent []byte) de
 	}
 }
 
-func Decompress(srcVMFile *define.VMFile, dstFilePath string) error {
-	srcFilePath := srcVMFile.GetPath()
-	// Are we reading full image file?
-	// Only few bytes are read to detect
-	// the compression type
-	srcFileContent, err := srcVMFile.Read()
-	if err != nil {
-		return err
-	}
-
-	d := newDecompressor(srcFilePath, srcFileContent)
-	return runDecompression(d, dstFilePath)
-}
-
-func runDecompression(d decompressor, dstFilePath string) error {
-	decompressorReader, err := d.reader()
+func runDecompression(d decompressor, decompressedFilePath string) error {
+	compressedFileReader, err := d.compressedFileReader()
 	if err != nil {
 		return err
 	}
 	defer d.close()
 
-	stat, err := os.Stat(d.srcFilePath())
-	if err != nil {
-		return err
-	}
-
-	initMsg := progressBarPrefix + ": " + filepath.Base(dstFilePath)
+	initMsg := progressBarPrefix + ": " + filepath.Base(decompressedFilePath)
 	finalMsg := initMsg + ": done"
 
-	// We are getting the compressed file size but
-	// the progress bar needs the full size of the
-	// decompressed file.
-	// As a result the progress bar shows 100%
-	// before the decompression completes.
-	// A workaround is to set the size to -1 but the
-	// side effect is that we won't see any advancment in
-	// the bar.
-	// An update in utils.ProgressBar to handle is needed
-	// to improve the case of size=-1 (i.e. unkwonw size).
-	p, bar := utils.ProgressBar(initMsg, stat.Size(), finalMsg)
+	p, bar := utils.ProgressBar(initMsg, d.compressedFileSize(), finalMsg)
 	// Wait for bars to complete and then shut down the bars container
 	defer p.Wait()
 
-	readProxy := bar.ProxyReader(decompressorReader)
+	compressedFileReaderProxy := bar.ProxyReader(compressedFileReader)
 	// Interrupts the bar goroutine. It's important that
 	// bar.Abort(false) is called before p.Wait(), otherwise
 	// can hang.
 	defer bar.Abort(false)
 
-	dstFileWriter, err := os.OpenFile(dstFilePath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, stat.Mode())
-	if err != nil {
-		logrus.Errorf("Unable to open destination file %s for writing: %q", dstFilePath, err)
+	var decompressedFileWriter *os.File
+
+	if decompressedFileWriter, err = os.OpenFile(decompressedFilePath, decompressedFileFlag, d.compressedFileMode()); err != nil {
+		logrus.Errorf("Unable to open destination file %s for writing: %q", decompressedFilePath, err)
 		return err
 	}
 	defer func() {
-		if err := dstFileWriter.Close(); err != nil {
-			logrus.Errorf("Unable to to close destination file %s: %q", dstFilePath, err)
+		if err := decompressedFileWriter.Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+			logrus.Warnf("Unable to to close destination file %s: %q", decompressedFilePath, err)
 		}
 	}()
 
-	err = d.copy(dstFileWriter, readProxy)
-	if err != nil {
+	if err = d.decompress(decompressedFileWriter, compressedFileReaderProxy); err != nil {
 		logrus.Errorf("Error extracting compressed file: %q", err)
 		return err
 	}
+
 	return nil
 }

pkg/machine/compression/generic.go

@@ -2,6 +2,7 @@ package compression
 
 import (
 	"io"
+	"io/fs"
 	"os"
 
 	"github.com/containers/image/v5/pkg/compression"
@@ -9,38 +10,48 @@ import (
 )
 
 type genericDecompressor struct {
-	compressedFilePath string
-	compressedFile     *os.File
-	uncompressStream   io.ReadCloser
+	compressedFilePath     string
+	compressedFile         *os.File
+	decompressedFileReader io.ReadCloser
+	compressedFileInfo     os.FileInfo
 }
 
-func newGenericDecompressor(compressedFilePath string) decompressor {
-	return &genericDecompressor{
-		compressedFilePath: compressedFilePath,
-	}
-}
-
-func (d *genericDecompressor) srcFilePath() string {
-	return d.compressedFilePath
-}
-
-func (d *genericDecompressor) reader() (io.Reader, error) {
-	srcFile, err := os.Open(d.compressedFilePath)
+func newGenericDecompressor(compressedFilePath string) (*genericDecompressor, error) {
+	d := &genericDecompressor{}
+	d.compressedFilePath = compressedFilePath
+	stat, err := os.Stat(d.compressedFilePath)
 	if err != nil {
 		return nil, err
 	}
-	d.compressedFile = srcFile
-	return srcFile, nil
+	d.compressedFileInfo = stat
+	return d, nil
 }
 
-func (d *genericDecompressor) copy(w *os.File, r io.Reader) error {
-	uncompressStream, _, err := compression.AutoDecompress(r)
+func (d *genericDecompressor) compressedFileSize() int64 {
+	return d.compressedFileInfo.Size()
+}
+
+func (d *genericDecompressor) compressedFileMode() fs.FileMode {
+	return d.compressedFileInfo.Mode()
+}
+
+func (d *genericDecompressor) compressedFileReader() (io.ReadCloser, error) {
+	compressedFile, err := os.Open(d.compressedFilePath)
+	if err != nil {
+		return nil, err
+	}
+	d.compressedFile = compressedFile
+	return compressedFile, nil
+}
+
+func (d *genericDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
+	decompressedFileReader, _, err := compression.AutoDecompress(r)
 	if err != nil {
 		return err
 	}
-	d.uncompressStream = uncompressStream
+	d.decompressedFileReader = decompressedFileReader
 
-	_, err = io.Copy(w, uncompressStream)
+	_, err = io.Copy(w, decompressedFileReader)
 	return err
 }
 
@@ -48,7 +59,10 @@ func (d *genericDecompressor) close() {
 	if err := d.compressedFile.Close(); err != nil {
 		logrus.Errorf("Unable to close compressed file: %q", err)
 	}
-	if err := d.uncompressStream.Close(); err != nil {
-		logrus.Errorf("Unable to close uncompressed stream: %q", err)
+
+	if d.decompressedFileReader != nil {
+		if err := d.decompressedFileReader.Close(); err != nil {
+			logrus.Errorf("Unable to close uncompressed stream: %q", err)
+		}
 	}
 }

pkg/machine/compression/gzip.go

@@ -3,54 +3,34 @@ package compression
 import (
 	"compress/gzip"
 	"io"
-	"os"
 
 	crcOs "github.com/crc-org/crc/v2/pkg/os"
 	"github.com/sirupsen/logrus"
 )
 
-type gzDecompressor struct {
-	compressedFilePath string
-	compressedFile     *os.File
-	gzReader           *gzip.Reader
+type gzipDecompressor struct {
+	genericDecompressor
+	gzReader io.ReadCloser
 }
 
-func newGzipDecompressor(compressedFilePath string) decompressor {
-	return &gzDecompressor{
-		compressedFilePath: compressedFilePath,
-	}
+func newGzipDecompressor(compressedFilePath string) (*gzipDecompressor, error) {
+	d, err := newGenericDecompressor(compressedFilePath)
+	return &gzipDecompressor{*d, nil}, err
 }
 
-func (d *gzDecompressor) srcFilePath() string {
-	return d.compressedFilePath
-}
-
-func (d *gzDecompressor) reader() (io.Reader, error) {
-	srcFile, err := os.Open(d.compressedFilePath)
+func (d *gzipDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
+	gzReader, err := gzip.NewReader(r)
 	if err != nil {
-		return nil, err
-	}
-	d.compressedFile = srcFile
-
-	gzReader, err := gzip.NewReader(srcFile)
-	if err != nil {
-		return gzReader, err
+		return err
 	}
 	d.gzReader = gzReader
-
-	return gzReader, nil
-}
-
-func (*gzDecompressor) copy(w *os.File, r io.Reader) error {
-	_, err := crcOs.CopySparse(w, r)
+	_, err = crcOs.CopySparse(w, gzReader)
 	return err
 }
 
-func (d *gzDecompressor) close() {
-	if err := d.compressedFile.Close(); err != nil {
-		logrus.Errorf("Unable to close gz file: %q", err)
-	}
+func (d *gzipDecompressor) close() {
 	if err := d.gzReader.Close(); err != nil {
 		logrus.Errorf("Unable to close gz file: %q", err)
 	}
+	d.genericDecompressor.close()
 }

pkg/machine/compression/uncompressed.go

@@ -2,44 +2,20 @@ package compression
 
 import (
 	"io"
-	"os"
 
 	crcOs "github.com/crc-org/crc/v2/pkg/os"
-	"github.com/sirupsen/logrus"
 )
 
 type uncompressedDecompressor struct {
-	compressedFilePath string
-	compressedFile     *os.File
+	genericDecompressor
 }
 
-func newUncompressedDecompressor(compressedFilePath string) decompressor {
-	return &uncompressedDecompressor{
-		compressedFilePath: compressedFilePath,
-	}
+func newUncompressedDecompressor(compressedFilePath string) (*uncompressedDecompressor, error) {
+	d, err := newGenericDecompressor(compressedFilePath)
+	return &uncompressedDecompressor{*d}, err
 }
 
-func (d *uncompressedDecompressor) srcFilePath() string {
-	return d.compressedFilePath
-}
-
-func (d *uncompressedDecompressor) reader() (io.Reader, error) {
-	srcFile, err := os.Open(d.compressedFilePath)
-	if err != nil {
-		return nil, err
-	}
-	d.compressedFile = srcFile
-
-	return srcFile, nil
-}
-
-func (*uncompressedDecompressor) copy(w *os.File, r io.Reader) error {
+func (*uncompressedDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	_, err := crcOs.CopySparse(w, r)
 	return err
 }
-
-func (d *uncompressedDecompressor) close() {
-	if err := d.compressedFile.Close(); err != nil {
-		logrus.Errorf("Unable to close gz file: %q", err)
-	}
-}

pkg/machine/compression/xz.go

@@ -11,33 +11,18 @@ import (
 )
 
 type xzDecompressor struct {
-	compressedFilePath string
-	compressedFile     *os.File
+	genericDecompressor
 }
 
-func newXzDecompressor(compressedFilePath string) decompressor {
-	return &xzDecompressor{
-		compressedFilePath: compressedFilePath,
-	}
-}
-
-func (d *xzDecompressor) srcFilePath() string {
-	return d.compressedFilePath
-}
-
-func (d *xzDecompressor) reader() (io.Reader, error) {
-	srcFile, err := os.Open(d.compressedFilePath)
-	if err != nil {
-		return nil, err
-	}
-	d.compressedFile = srcFile
-	return srcFile, nil
+func newXzDecompressor(compressedFilePath string) (*xzDecompressor, error) {
+	d, err := newGenericDecompressor(compressedFilePath)
+	return &xzDecompressor{*d}, err
 }
 
 // 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) copy(w *os.File, r io.Reader) error {
+func (*xzDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	var cmd *exec.Cmd
 	var read io.Reader
 
@@ -79,9 +64,3 @@ func (*xzDecompressor) copy(w *os.File, r io.Reader) error {
 	<-done
 	return nil
 }
-
-func (d *xzDecompressor) close() {
-	if err := d.compressedFile.Close(); err != nil {
-		logrus.Errorf("Unable to close xz file: %q", err)
-	}
-}

pkg/machine/compression/zip.go

@@ -4,28 +4,26 @@ import (
 	"archive/zip"
 	"errors"
 	"io"
-	"os"
 
 	"github.com/sirupsen/logrus"
 )
 
 type zipDecompressor struct {
-	compressedFilePath string
-	zipReader          *zip.ReadCloser
-	fileReader         io.ReadCloser
+	genericDecompressor
+	zipReader  *zip.ReadCloser
+	fileReader io.ReadCloser
 }
 
-func newZipDecompressor(compressedFilePath string) decompressor {
-	return &zipDecompressor{
-		compressedFilePath: compressedFilePath,
-	}
+func newZipDecompressor(compressedFilePath string) (*zipDecompressor, error) {
+	d, err := newGenericDecompressor(compressedFilePath)
+	return &zipDecompressor{*d, nil, nil}, err
 }
 
-func (d *zipDecompressor) srcFilePath() string {
-	return d.compressedFilePath
-}
-
-func (d *zipDecompressor) reader() (io.Reader, error) {
+// This is the only compressor that doesn't return the compressed file
+// stream (zip.OpenReader() provides access to the decompressed file).
+// As a result the progress bar shows the decompressed file stream
+// but the final size is the compressed file size.
+func (d *zipDecompressor) compressedFileReader() (io.ReadCloser, error) {
 	zipReader, err := zip.OpenReader(d.compressedFilePath)
 	if err != nil {
 		return nil, err
@@ -42,7 +40,7 @@ func (d *zipDecompressor) reader() (io.Reader, error) {
 	return z, nil
 }
 
-func (*zipDecompressor) copy(w *os.File, r io.Reader) error {
+func (*zipDecompressor) decompress(w io.WriteSeeker, r io.Reader) error {
 	_, err := io.Copy(w, r)
 	return err
 }