directory: simplify newImageDestination

The current code is unnecessarily complicated:
 - it does unnecessary checks if a file/directory exists before opening it;
 - it uses three helper functions not used anywhere else;
 - directory contents is read twice.

Untangle all this, making the code simpler.

Also, move the logic of initializing a destination directory to a
separate function.

Now, since the logic of handing version file is now encapsulated in a
single function, remove versionPath method and const version, as well
as the corresponding test.

This also fixes the subtle issue of using ref.path for constructing the
version file path, but ref.resolvedPath for all other operations.

Signed-off-by: Kir Kolyshkin <[email protected]>

directory/directory_dest.go

@@ -1,6 +1,7 @@
 package directory
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -15,13 +16,10 @@ import (
 	"github.com/containers/image/v5/internal/putblobdigest"
 	"github.com/containers/image/v5/internal/signature"
 	"github.com/containers/image/v5/types"
-	"github.com/containers/storage/pkg/fileutils"
 	"github.com/opencontainers/go-digest"
 	"github.com/sirupsen/logrus"
 )
 
-const version = "Directory Transport Version: 1.1\n"
-
 // ErrNotContainerImageDir indicates that the directory doesn't match the expected contents of a directory created
 // using the 'dir' transport
 var ErrNotContainerImageDir = errors.New("not a containers image directory, don't want to overwrite important data")
@@ -51,52 +49,8 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (private.Im
 		}
 	}
 
-	// If directory exists check if it is empty
-	// if not empty, check whether the contents match that of a container image directory and overwrite the contents
-	// if the contents don't match throw an error
-	dirExists, err := pathExists(ref.resolvedPath)
-	if err != nil {
-		return nil, fmt.Errorf("checking for path %q: %w", ref.resolvedPath, err)
-	}
-	if dirExists {
-		isEmpty, err := isDirEmpty(ref.resolvedPath)
-		if err != nil {
-			return nil, err
-		}
-
-		if !isEmpty {
-			versionExists, err := pathExists(ref.versionPath())
-			if err != nil {
-				return nil, fmt.Errorf("checking if path exists %q: %w", ref.versionPath(), err)
-			}
-			if versionExists {
-				contents, err := os.ReadFile(ref.versionPath())
-				if err != nil {
-					return nil, err
-				}
-				// check if contents of version file is what we expect it to be
-				if string(contents) != version {
-					return nil, ErrNotContainerImageDir
-				}
-			} else {
-				return nil, ErrNotContainerImageDir
-			}
-			// delete directory contents so that only one image is in the directory at a time
-			if err = removeDirContents(ref.resolvedPath); err != nil {
-				return nil, fmt.Errorf("erasing contents in %q: %w", ref.resolvedPath, err)
-			}
-			logrus.Debugf("overwriting existing container image directory %q", ref.resolvedPath)
-		}
-	} else {
-		// create directory if it doesn't exist
-		if err := os.MkdirAll(ref.resolvedPath, 0755); err != nil {
-			return nil, fmt.Errorf("unable to create directory %q: %w", ref.resolvedPath, err)
-		}
-	}
-	// create version file
-	err = os.WriteFile(ref.versionPath(), []byte(version), 0644)
-	if err != nil {
-		return nil, fmt.Errorf("creating version file %q: %w", ref.versionPath(), err)
+	if err := initDestDir(ref.resolvedPath); err != nil {
+		return nil, err
 	}
 
 	d := &dirImageDestination{
@@ -116,6 +70,63 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (private.Im
 	return d, nil
 }
 
+func initDestDir(dir string) error {
+	const versionContents = "Directory Transport Version: 1.1\n"
+	versionFile := filepath.Join(dir, "version")
+
+	// If dir exists, checks if it is empty.
+	// If not empty, check whether the contents match that of a container
+	// image directory and overwrite the contents.
+	// If the contents don't match, throw ErrNotContainerImageDir.
+	fd, err := os.Open(dir)
+	switch {
+	case err == nil: // Directory exists.
+		contents, err := fd.Readdirnames(-1)
+		_ = fd.Close()
+		if err != nil { // Unexpected error.
+			return fmt.Errorf("%q: %w", dir, err)
+		}
+		if len(contents) == 0 {
+			break
+		}
+		// Check if contents of version file is what we expect it to be.
+		ver, err := os.ReadFile(versionFile)
+		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				return ErrNotContainerImageDir
+			}
+			return fmt.Errorf("%q: %w", versionFile, err)
+		}
+		if !bytes.Equal(ver, []byte(versionContents)) {
+			return ErrNotContainerImageDir
+		}
+		// Indeed an image directory. Reuse by removing all the old contents
+		// (including the versionFile, which will be recreated below).
+		logrus.Debugf("overwriting existing container image directory %q", dir)
+		for _, name := range contents {
+			rm := filepath.Join(dir, name)
+			err := os.RemoveAll(rm)
+			if err != nil {
+				return fmt.Errorf("%q: %w", rm, err)
+			}
+		}
+	case errors.Is(err, os.ErrNotExist): // Directory does not exist; create it.
+		if err := os.MkdirAll(dir, 0o755); err != nil {
+			return fmt.Errorf("%q: %w", dir, err)
+		}
+	default:
+		// Unexpected error.
+		return fmt.Errorf("%q: %w", dir, err)
+	}
+
+	err = os.WriteFile(versionFile, []byte(versionContents), 0o644)
+	if err != nil {
+		return fmt.Errorf("%q: %w", versionFile, err)
+	}
+
+	return nil
+}
+
 // Reference returns the reference used to set up this destination.  Note that this should directly correspond to user's intent,
 // e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects.
 func (d *dirImageDestination) Reference() types.ImageReference {
@@ -261,39 +272,3 @@ func (d *dirImageDestination) PutSignaturesWithFormat(ctx context.Context, signa
 func (d *dirImageDestination) Commit(context.Context, types.UnparsedImage) error {
 	return nil
 }
-
-// returns true if path exists
-func pathExists(path string) (bool, error) {
-	err := fileutils.Exists(path)
-	if err == nil {
-		return true, nil
-	}
-	if os.IsNotExist(err) {
-		return false, nil
-	}
-	return false, err
-}
-
-// returns true if directory is empty
-func isDirEmpty(path string) (bool, error) {
-	files, err := os.ReadDir(path)
-	if err != nil {
-		return false, err
-	}
-	return len(files) == 0, nil
-}
-
-// deletes the contents of a directory
-func removeDirContents(path string) error {
-	files, err := os.ReadDir(path)
-	if err != nil {
-		return err
-	}
-
-	for _, file := range files {
-		if err := os.RemoveAll(filepath.Join(path, file.Name())); err != nil {
-			return err
-		}
-	}
-	return nil
-}

directory/directory_transport.go

@@ -190,8 +190,3 @@ func (ref dirReference) signaturePath(index int, instanceDigest *digest.Digest)
 	}
 	return filepath.Join(ref.path, fmt.Sprintf("signature-%d", index+1)), nil
 }
-
-// versionPath returns a path for the version file within a directory using our conventions.
-func (ref dirReference) versionPath() string {
-	return filepath.Join(ref.path, "version")
-}

directory/directory_transport_test.go

@@ -243,10 +243,3 @@ func TestReferenceSignaturePath(t *testing.T) {
 	_, err = dirRef.signaturePath(0, &invalidDigest)
 	assert.Error(t, err)
 }
-
-func TestReferenceVersionPath(t *testing.T) {
-	ref, tmpDir := refToTempDir(t)
-	dirRef, ok := ref.(dirReference)
-	require.True(t, ok)
-	assert.Equal(t, tmpDir+"/version", dirRef.versionPath())
-}