Merge pull request #30 from thaJeztah/platforms_refactor

refactor, optimize FormatAll, ParseAll

Author: estesp

cpuinfo_linux.go

@@ -45,7 +45,6 @@ func getMachineArch() (string, error) {
 // So we don't need to access the ARM registers to detect platform information
 // by ourselves. We can just parse these information from /proc/cpuinfo
 func getCPUInfo(pattern string) (info string, err error) {
-
 	cpuinfo, err := os.Open("/proc/cpuinfo")
 	if err != nil {
 		return "", err
@@ -75,7 +74,6 @@ func getCPUInfo(pattern string) (info string, err error) {
 
 // getCPUVariantFromArch get CPU variant from arch through a system call
 func getCPUVariantFromArch(arch string) (string, error) {
-
 	var variant string
 
 	arch = strings.ToLower(arch)

cpuinfo_linux_test.go

@@ -46,7 +46,6 @@ func TestCPUVariant(t *testing.T) {
 }
 
 func TestGetCPUVariantFromArch(t *testing.T) {
-
 	for _, testcase := range []struct {
 		name        string
 		input       string
@@ -127,13 +126,11 @@ func TestGetCPUVariantFromArch(t *testing.T) {
 						t.Fatalf("Expect to get variant: %v, however %v returned", testcase.output, variant)
 					}
 				}
-
 			} else {
 				if !errors.Is(err, testcase.expectedErr) {
 					t.Fatalf("Expect to get error: %v, however error %v returned", testcase.expectedErr, err)
 				}
 			}
 		})
-
 	}
 }

cpuinfo_other.go

@@ -24,7 +24,6 @@ import (
 )
 
 func getCPUVariant() (string, error) {
-
 	var variant string
 
 	switch runtime.GOOS {

go.mod

@@ -1,6 +1,6 @@
 module github.com/containerd/platforms
 
-go 1.21
+go 1.24
 
 require (
 	github.com/containerd/log v0.1.0

platform_windows_compat_test.go

@@ -183,5 +183,4 @@ func Test_PlatformOrder(t *testing.T) {
 			}
 		})
 	}
-
 }

platforms.go

@@ -280,13 +280,9 @@ func Parse(specifier string) (specs.Platform, error) {
 			}
 			p.OSVersion = osVersion
 			if osOptions[3] != "" {
-				rawFeatures := strings.Split(osOptions[3][1:], "+")
-				p.OSFeatures = make([]string, len(rawFeatures))
-				for i, f := range rawFeatures {
-					p.OSFeatures[i], err = decodeOSOption(f)
-					if err != nil {
-						return specs.Platform{}, fmt.Errorf("%q has an invalid OS feature %q: %w", specifier, f, err)
-					}
+				p.OSFeatures, err = parseOSFeatures(osOptions[3][1:])
+				if err != nil {
+					return specs.Platform{}, fmt.Errorf("%q has invalid OS features: %w", specifier, err)
 				}
 			}
 		} else {
@@ -346,6 +342,30 @@ func Parse(specifier string) (specs.Platform, error) {
 	return specs.Platform{}, fmt.Errorf("%q: cannot parse platform specifier: %w", specifier, errInvalidArgument)
 }
 
+func parseOSFeatures(s string) ([]string, error) {
+	if s == "" {
+		return nil, nil
+	}
+
+	var features []string
+	for raw := range strings.SplitSeq(s, "+") {
+		raw = strings.TrimSpace(raw)
+		if raw == "" {
+			return nil, fmt.Errorf("empty os feature: %w", errInvalidArgument)
+		}
+		feature, err := decodeOSOption(raw)
+		if err != nil {
+			return nil, fmt.Errorf("invalid os feature %q: %w", raw, err)
+		}
+		if feature == "" {
+			continue
+		}
+		features = append(features, feature)
+	}
+
+	return features, nil
+}
+
 // MustParse is like Parses but panics if the specifier cannot be parsed.
 // Simplifies initialization of global variables.
 func MustParse(specifier string) specs.Platform {
@@ -371,21 +391,55 @@ func FormatAll(platform specs.Platform) string {
 	if platform.OS == "" {
 		return "unknown"
 	}
+	if platform.OSVersion == "" && len(platform.OSFeatures) == 0 {
+		return path.Join(platform.OS, platform.Architecture, platform.Variant)
+	}
+
+	var b strings.Builder
+	b.WriteString(platform.OS)
+	osv := encodeOSOption(platform.OSVersion)
+	formatted := formatOSFeatures(platform.OSFeatures)
+	if osv != "" || formatted != "" {
+		b.Grow(len(osv) + len(formatted) + 3) // parens + maybe '+'
+		b.WriteByte('(')
+		if osv != "" {
+			b.WriteString(osv)
+		}
+		if formatted != "" {
+			b.WriteByte('+')
+			b.WriteString(formatted)
+		}
+		b.WriteByte(')')
+	}
+
+	return path.Join(b.String(), platform.Architecture, platform.Variant)
+}
+
+func formatOSFeatures(features []string) string {
+	if len(features) == 0 {
+		return ""
+	}
 
-	osOptions := encodeOSOption(platform.OSVersion)
-	features := platform.OSFeatures
 	if !slices.IsSorted(features) {
 		features = slices.Clone(features)
 		slices.Sort(features)
 	}
+	var b strings.Builder
+	var wrote bool
+	var prev string
 	for _, f := range features {
-		osOptions += "+" + encodeOSOption(f)
-	}
-	if osOptions != "" {
-		OSAndVersion := fmt.Sprintf("%s(%s)", platform.OS, osOptions)
-		return path.Join(OSAndVersion, platform.Architecture, platform.Variant)
+		if f == "" || f == prev {
+			// skip empty and duplicate values
+			continue
+		}
+		prev = f
+		if wrote {
+			b.WriteByte('+')
+		}
+		b.WriteString(encodeOSOption(f))
+		wrote = true
 	}
-	return path.Join(platform.OS, platform.Architecture, platform.Variant)
+	return b.String()
 }
 
 // osOptionReplacer encodes characters in OS option values (version and

platforms_test.go

@@ -20,6 +20,8 @@ import (
 	"path"
 	"reflect"
 	"runtime"
+	"strconv"
+	"strings"
 	"testing"
 
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
@@ -578,6 +580,20 @@ func TestParseSelectorInvalid(t *testing.T) {
 	}
 }
 
+func TestFormatAllSkipsEmptyOSFeatures(t *testing.T) {
+	p := specs.Platform{
+		OS:           "linux",
+		Architecture: "amd64",
+		OSFeatures:   []string{"", "gpu", "", "simd"},
+	}
+
+	formatted := FormatAll(p)
+	expected := "linux(+gpu+simd)/amd64"
+	if formatted != expected {
+		t.Fatalf("unexpected format: %q != %q", formatted, expected)
+	}
+}
+
 func FuzzPlatformsParse(f *testing.F) {
 	f.Add("linux/amd64")
 	f.Fuzz(func(t *testing.T, s string) {
@@ -587,3 +603,131 @@ func FuzzPlatformsParse(f *testing.F) {
 		}
 	})
 }
+
+func BenchmarkParseOSOptions(b *testing.B) {
+	maxFeatures := 16
+
+	benchmarks := []struct {
+		doc   string
+		input string
+	}{
+		{
+			doc:   "valid windows version and feature",
+			input: "windows(10.0.17763+win32k)/amd64",
+		},
+		{
+			doc:   "valid but lengthy features",
+			input: "linux(+" + strings.Repeat("+feature", maxFeatures) + ")/amd64",
+		},
+		{
+			doc:   "exploding plus chain",
+			input: "linux(" + strings.Repeat("+", 64*1024) + ")/amd64",
+		},
+		{
+			doc:   "kernel config feature blob",
+			input: "linux(+CONFIG_" + strings.Repeat("FOO=y_", 16*1024) + "BAR)/amd64",
+		},
+	}
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for _, bm := range benchmarks {
+		b.Run(bm.doc, func(b *testing.B) {
+			for range b.N {
+				_, _ = Parse(bm.input)
+			}
+		})
+	}
+}
+
+func BenchmarkFormatAllOSFeatures(b *testing.B) {
+	maxFeatures := 16
+
+	benchmarks := []struct {
+		doc      string
+		platform specs.Platform
+	}{
+		{
+			doc: "plain linux amd64",
+			platform: specs.Platform{
+				OS:           "linux",
+				Architecture: "amd64",
+			},
+		},
+		{
+			doc: "windows version and feature",
+			platform: specs.Platform{
+				OS:           "windows",
+				OSVersion:    "10.0.17763",
+				OSFeatures:   []string{"win32k"},
+				Architecture: "amd64",
+			},
+		},
+		{
+			doc: "valid but lengthy features",
+			platform: specs.Platform{
+				OS: "linux",
+				OSFeatures: func() (out []string) {
+					for range maxFeatures {
+						out = append(out, "feature")
+					}
+					return out
+				}(),
+				Architecture: "amd64",
+			},
+		},
+		{
+			doc: "skips empty features",
+			platform: specs.Platform{
+				OS:           "linux",
+				OSFeatures:   []string{"", "gpu", "", "simd"},
+				Architecture: "amd64",
+			},
+		},
+		{
+			doc: "kernel config feature blob",
+			platform: specs.Platform{
+				OS:           "linux",
+				OSFeatures:   []string{"CONFIG_" + strings.Repeat("FOO_", 16*1024) + "BAR"},
+				Architecture: "amd64",
+			},
+		},
+		{
+			doc: "many kernel config features with empties",
+			platform: specs.Platform{
+				OS: "linux",
+				OSFeatures: func() []string {
+					n := 1024
+					out := make([]string, n)
+					for i := range out {
+						if i%10 == 0 {
+							out[i] = "" // simulate bad data
+						} else {
+							out[i] = "CONFIG_FOO_" + strconv.Itoa(i)
+						}
+					}
+					return out
+				}(),
+				Architecture: "amd64",
+			},
+		},
+		{
+			doc: "too many features",
+			platform: specs.Platform{
+				OS:           "linux",
+				OSFeatures:   make([]string, maxFeatures+1),
+				Architecture: "amd64",
+			},
+		},
+	}
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for _, bm := range benchmarks {
+		b.Run(bm.doc, func(b *testing.B) {
+			for range b.N {
+				_ = FormatAll(bm.platform)
+			}
+		})
+	}
+}