Assorted clean ups, optimizations, and a small bug fix

copy/sign.go

@@ -106,7 +106,7 @@ func (c *copier) createSignatures(ctx context.Context, manifest []byte, identity
 			if len(c.signers) == 1 {
 				return nil, fmt.Errorf("creating signature: %w", err)
 			} else {
-				return nil, fmt.Errorf("creating signature %d: %w", signerIndex, err)
+				return nil, fmt.Errorf("creating signature %d: %w", signerIndex+1, err)
 			}
 		}
 		res = append(res, newSig)

copy/single.go

@@ -323,10 +323,7 @@ func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.Syst
 		if err != nil {
 			return fmt.Errorf("parsing image configuration: %w", err)
 		}
-		wantedPlatforms, err := platform.WantedPlatforms(sys)
-		if err != nil {
-			return fmt.Errorf("getting current platform information %#v: %w", sys, err)
-		}
+		wantedPlatforms := platform.WantedPlatforms(sys)
 
 		options := newOrderedSet()
 		match := false

docker/docker_image_src.go

@@ -116,10 +116,10 @@ func newImageSource(ctx context.Context, sys *types.SystemContext, ref dockerRef
 		// Don’t just build a string, try to preserve the typed error.
 		primary := &attempts[len(attempts)-1]
 		extras := []string{}
-		for i := 0; i < len(attempts)-1; i++ {
+		for _, attempt := range attempts[:len(attempts)-1] {
 			// This is difficult to fit into a single-line string, when the error can contain arbitrary strings including any metacharacters we decide to use.
 			// The paired [] at least have some chance of being unambiguous.
-			extras = append(extras, fmt.Sprintf("[%s: %v]", attempts[i].ref.String(), attempts[i].err))
+			extras = append(extras, fmt.Sprintf("[%s: %v]", attempt.ref.String(), attempt.err))
 		}
 		return nil, fmt.Errorf("(Mirrors also failed: %s): %s: %w", strings.Join(extras, "\n"), primary.ref.String(), primary.err)
 	}
@@ -464,26 +464,20 @@ func (s *dockerImageSource) GetSignaturesWithFormat(ctx context.Context, instanc
 	var res []signature.Signature
 	switch {
 	case s.c.supportsSignatures:
-		sigs, err := s.getSignaturesFromAPIExtension(ctx, instanceDigest)
-		if err != nil {
+		if err := s.appendSignaturesFromAPIExtension(ctx, &res, instanceDigest); err != nil {
 			return nil, err
 		}
-		res = append(res, sigs...)
 	case s.c.signatureBase != nil:
-		sigs, err := s.getSignaturesFromLookaside(ctx, instanceDigest)
-		if err != nil {
+		if err := s.appendSignaturesFromLookaside(ctx, &res, instanceDigest); err != nil {
 			return nil, err
 		}
-		res = append(res, sigs...)
 	default:
 		return nil, errors.New("Internal error: X-Registry-Supports-Signatures extension not supported, and lookaside should not be empty configuration")
 	}
 
-	sigstoreSigs, err := s.getSignaturesFromSigstoreAttachments(ctx, instanceDigest)
-	if err != nil {
+	if err := s.appendSignaturesFromSigstoreAttachments(ctx, &res, instanceDigest); err != nil {
 		return nil, err
 	}
-	res = append(res, sigstoreSigs...)
 	return res, nil
 }
 
@@ -505,35 +499,35 @@ func (s *dockerImageSource) manifestDigest(ctx context.Context, instanceDigest *
 	return manifest.Digest(s.cachedManifest)
 }
 
-// getSignaturesFromLookaside implements GetSignaturesWithFormat() from the lookaside location configured in s.c.signatureBase,
-// which is not nil.
-func (s *dockerImageSource) getSignaturesFromLookaside(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
+// appendSignaturesFromLookaside implements GetSignaturesWithFormat() from the lookaside location configured in s.c.signatureBase,
+// which is not nil, storing the signatures to *dest.
+// On error, the contents of *dest are undefined.
+func (s *dockerImageSource) appendSignaturesFromLookaside(ctx context.Context, dest *[]signature.Signature, instanceDigest *digest.Digest) error {
 	manifestDigest, err := s.manifestDigest(ctx, instanceDigest)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	// NOTE: Keep this in sync with docs/signature-protocols.md!
-	signatures := []signature.Signature{}
 	for i := 0; ; i++ {
 		if i >= maxLookasideSignatures {
-			return nil, fmt.Errorf("server provided %d signatures, assuming that's unreasonable and a server error", maxLookasideSignatures)
+			return fmt.Errorf("server provided %d signatures, assuming that's unreasonable and a server error", maxLookasideSignatures)
 		}
 
 		sigURL, err := lookasideStorageURL(s.c.signatureBase, manifestDigest, i)
 		if err != nil {
-			return nil, err
+			return err
 		}
 		signature, missing, err := s.getOneSignature(ctx, sigURL)
 		if err != nil {
-			return nil, err
+			return err
 		}
 		if missing {
 			break
 		}
-		signatures = append(signatures, signature)
+		*dest = append(*dest, signature)
 	}
-	return signatures, nil
+	return nil
 }
 
 // getOneSignature downloads one signature from sigURL, and returns (signature, false, nil)
@@ -596,48 +590,51 @@ func (s *dockerImageSource) getOneSignature(ctx context.Context, sigURL *url.URL
 	}
 }
 
-// getSignaturesFromAPIExtension implements GetSignaturesWithFormat() using the X-Registry-Supports-Signatures API extension.
-func (s *dockerImageSource) getSignaturesFromAPIExtension(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
+// appendSignaturesFromAPIExtension implements GetSignaturesWithFormat() using the X-Registry-Supports-Signatures API extension,
+// storing the signatures to *dest.
+// On error, the contents of *dest are undefined.
+func (s *dockerImageSource) appendSignaturesFromAPIExtension(ctx context.Context, dest *[]signature.Signature, instanceDigest *digest.Digest) error {
 	manifestDigest, err := s.manifestDigest(ctx, instanceDigest)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	parsedBody, err := s.c.getExtensionsSignatures(ctx, s.physicalRef, manifestDigest)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	var sigs []signature.Signature
 	for _, sig := range parsedBody.Signatures {
 		if sig.Version == extensionSignatureSchemaVersion && sig.Type == extensionSignatureTypeAtomic {
-			sigs = append(sigs, signature.SimpleSigningFromBlob(sig.Content))
+			*dest = append(*dest, signature.SimpleSigningFromBlob(sig.Content))
 		}
 	}
-	return sigs, nil
+	return nil
 }
 
-func (s *dockerImageSource) getSignaturesFromSigstoreAttachments(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
+// appendSignaturesFromSigstoreAttachments implements GetSignaturesWithFormat() using the sigstore tag convention,
+// storing the signatures to *dest.
+// On error, the contents of *dest are undefined.
+func (s *dockerImageSource) appendSignaturesFromSigstoreAttachments(ctx context.Context, dest *[]signature.Signature, instanceDigest *digest.Digest) error {
 	if !s.c.useSigstoreAttachments {
 		logrus.Debugf("Not looking for sigstore attachments: disabled by configuration")
-		return nil, nil
+		return nil
 	}
 
 	manifestDigest, err := s.manifestDigest(ctx, instanceDigest)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	ociManifest, err := s.c.getSigstoreAttachmentManifest(ctx, s.physicalRef, manifestDigest)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	if ociManifest == nil {
-		return nil, nil
+		return nil
 	}
 
 	logrus.Debugf("Found a sigstore attachment manifest with %d layers", len(ociManifest.Layers))
-	res := []signature.Signature{}
 	for layerIndex, layer := range ociManifest.Layers {
 		// Note that this copies all kinds of attachments: attestations, and whatever else is there,
 		// not just signatures. We leave the signature consumers to decide based on the MIME type.
@@ -648,11 +645,11 @@ func (s *dockerImageSource) getSignaturesFromSigstoreAttachments(ctx context.Con
 		payload, err := s.c.getOCIDescriptorContents(ctx, s.physicalRef, layer, iolimits.MaxSignatureBodySize,
 			none.NoCache)
 		if err != nil {
-			return nil, err
+			return err
 		}
-		res = append(res, signature.SigstoreFromComponents(layer.MediaType, payload, layer.Annotations))
+		*dest = append(*dest, signature.SigstoreFromComponents(layer.MediaType, payload, layer.Annotations))
 	}
-	return res, nil
+	return nil
 }
 
 // deleteImage deletes the named image from the registry, if supported.

internal/manifest/docker_schema2_list.go

@@ -152,10 +152,7 @@ func (list *Schema2ListPublic) ChooseInstanceByCompression(ctx *types.SystemCont
 // ChooseInstance parses blob as a schema2 manifest list, and returns the digest
 // of the image which is appropriate for the current environment.
 func (list *Schema2ListPublic) ChooseInstance(ctx *types.SystemContext) (digest.Digest, error) {
-	wantedPlatforms, err := platform.WantedPlatforms(ctx)
-	if err != nil {
-		return "", fmt.Errorf("getting platform information %#v: %w", ctx, err)
-	}
+	wantedPlatforms := platform.WantedPlatforms(ctx)
 	for _, wantedPlatform := range wantedPlatforms {
 		for _, d := range list.Manifests {
 			imagePlatform := ociPlatformFromSchema2PlatformSpec(d.Platform)

internal/manifest/oci_index.go

@@ -236,10 +236,7 @@ func (index *OCI1IndexPublic) chooseInstance(ctx *types.SystemContext, preferGzi
 	if preferGzip == types.OptionalBoolTrue {
 		didPreferGzip = true
 	}
-	wantedPlatforms, err := platform.WantedPlatforms(ctx)
-	if err != nil {
-		return "", fmt.Errorf("getting platform information %#v: %w", ctx, err)
-	}
+	wantedPlatforms := platform.WantedPlatforms(ctx)
 	var bestMatch *instanceCandidate
 	bestMatch = nil
 	for manifestIndex, d := range index.Manifests {

internal/pkg/platform/platform_matcher.go

@@ -153,7 +153,7 @@ var compatibility = map[string][]string{
 // WantedPlatforms returns all compatible platforms with the platform specifics possibly overridden by user,
 // the most compatible platform is first.
 // If some option (arch, os, variant) is not present, a value from current platform is detected.
-func WantedPlatforms(ctx *types.SystemContext) ([]imgspecv1.Platform, error) {
+func WantedPlatforms(ctx *types.SystemContext) []imgspecv1.Platform {
 	// Note that this does not use Platform.OSFeatures and Platform.OSVersion at all.
 	// The fields are not specified by the OCI specification, as of version 1.1, usefully enough
 	// to be interoperable, anyway.
@@ -211,7 +211,7 @@ func WantedPlatforms(ctx *types.SystemContext) ([]imgspecv1.Platform, error) {
 			Variant:      v,
 		})
 	}
-	return res, nil
+	return res
 }
 
 // MatchesPlatform returns true if a platform descriptor from a multi-arch image matches

internal/pkg/platform/platform_matcher_test.go

@@ -54,8 +54,7 @@ func TestWantedPlatforms(t *testing.T) {
 		},
 	} {
 		testName := fmt.Sprintf("%q/%q/%q", c.ctx.ArchitectureChoice, c.ctx.OSChoice, c.ctx.VariantChoice)
-		platforms, err := WantedPlatforms(&c.ctx)
-		assert.Nil(t, err, testName)
+		platforms := WantedPlatforms(&c.ctx)
 		assert.Equal(t, c.expected, platforms, testName)
 	}
 }

pkg/blobinfocache/internal/prioritize/prioritize.go

@@ -200,8 +200,6 @@ func (css *candidateSortState) compare(xi, xj CandidateWithTime) int {
 
 // destructivelyPrioritizeReplacementCandidatesWithMax is destructivelyPrioritizeReplacementCandidates with parameters for the
 // number of entries to limit for known and unknown location separately, only to make testing simpler.
-// TODO: following function is not destructive any more in the nature instead prioritized result is actually copies of the original
-// candidate set, so In future we might wanna re-name this public API and remove the destructive prefix.
 func destructivelyPrioritizeReplacementCandidatesWithMax(cs []CandidateWithTime, primaryDigest, uncompressedDigest digest.Digest, totalLimit int, noLocationLimit int) []blobinfocache.BICReplacementCandidate2 {
 	// split unknown candidates and known candidates
 	// and limit them separately.

storage/storage_src.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"slices"
 	"sync"
 
 	"github.com/containers/image/v5/docker/reference"
@@ -300,7 +301,7 @@ func (s *storageImageSource) LayerInfosForCopy(ctx context.Context, instanceDige
 		uncompressedLayerType = manifest.DockerV2SchemaLayerMediaTypeUncompressed
 	}
 
-	physicalBlobInfos := []types.BlobInfo{}
+	physicalBlobInfos := []types.BlobInfo{} // Built reversed
 	layerID := s.image.TopLayer
 	for layerID != "" {
 		layer, err := s.imageRef.transport.store.Layer(layerID)
@@ -340,9 +341,10 @@ func (s *storageImageSource) LayerInfosForCopy(ctx context.Context, instanceDige
 			Size:      size,
 			MediaType: uncompressedLayerType,
 		}
-		physicalBlobInfos = append([]types.BlobInfo{blobInfo}, physicalBlobInfos...)
+		physicalBlobInfos = append(physicalBlobInfos, blobInfo)
 		layerID = layer.Parent
 	}
+	slices.Reverse(physicalBlobInfos)
 
 	res, err := buildLayerInfosForCopy(man.LayerInfos(), physicalBlobInfos)
 	if err != nil {