From 27f82af0d549170e69a39cc85422ab5656f29351 Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Thu, 14 May 2026 13:46:55 -0400 Subject: [PATCH 1/6] distro: add sfdisk command and partitioner backend selection Add SfdiskCmd() and PartitionerBackend() which reads ignition.partitioner from the kernel cmdline. Defaults to sgdisk. --- internal/distro/distro.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/distro/distro.go b/internal/distro/distro.go index 94dd86973..343cad9ff 100644 --- a/internal/distro/distro.go +++ b/internal/distro/distro.go @@ -17,6 +17,7 @@ package distro import ( "fmt" "os" + "strings" ) // Distro-specific settings that can be overridden at link time with e.g. @@ -42,6 +43,7 @@ var ( mountCmd = "mount" partxCmd = "partx" sgdiskCmd = "sgdisk" + sfdiskCmd = "sfdisk" modprobeCmd = "modprobe" udevadmCmd = "udevadm" usermodCmd = "usermod" @@ -113,6 +115,7 @@ func MdadmCmd() string { return mdadmCmd } func MountCmd() string { return mountCmd } func PartxCmd() string { return partxCmd } func SgdiskCmd() string { return sgdiskCmd } +func SfdiskCmd() string { return sfdiskCmd } func ModprobeCmd() string { return modprobeCmd } func UdevadmCmd() string { return udevadmCmd } func UsermodCmd() string { return usermodCmd } @@ -149,6 +152,36 @@ func WriteAuthorizedKeysFragment() bool { return bakedStringToBool(fromEnv("WRITE_AUTHORIZED_KEYS_FRAGMENT", writeAuthorizedKeysFragment)) } +var partitionerBackend string + +func PartitionerBackend() string { + if partitionerBackend == "" { + partitionerBackend = readPartitionerFromCmdline() + } + return partitionerBackend +} + +func readPartitionerFromCmdline() string { + // Allow override via environment variable for testing + if env := os.Getenv("IGNITION_PARTITIONER"); env == "sfdisk" || env == "sgdisk" { + return env + } + cmdline, err := os.ReadFile(kernelCmdlinePath) + if err != nil { + return "sgdisk" + } + for _, arg := range strings.Split(strings.TrimSpace(string(cmdline)), " ") { + parts := strings.SplitN(arg, "=", 2) + if parts[0] == "ignition.partitioner" && len(parts) == 2 { + switch parts[1] { + case "sfdisk", "sgdisk": + return parts[1] + } + } + } + return "sgdisk" +} + func fromEnv(nameSuffix, defaultValue string) string { value := os.Getenv("IGNITION_" + nameSuffix) if value != "" { From d5f19c3d11ee03c305a228c8a7b6cd1cba6440bc Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Thu, 14 May 2026 15:03:00 -0400 Subject: [PATCH 2/6] internal/partitioners: add DeviceManager interface with sgdisk and sfdisk backends Introduce DeviceManager interface abstracting partition table operations. Move sgdisk into internal/partitioners/sgdisk. Add sfdisk backend using script mode with read-modify-write approach. --- config/shared/errors/errors.go | 1 + internal/partitioners/partitioners.go | 47 ++ internal/partitioners/sfdisk/sfdisk.go | 461 +++++++++++++++++++ internal/partitioners/sfdisk/sfdisk_test.go | 291 ++++++++++++ internal/{ => partitioners}/sgdisk/sgdisk.go | 120 ++++- 5 files changed, 903 insertions(+), 17 deletions(-) create mode 100644 internal/partitioners/partitioners.go create mode 100644 internal/partitioners/sfdisk/sfdisk.go create mode 100644 internal/partitioners/sfdisk/sfdisk_test.go rename internal/{ => partitioners}/sgdisk/sgdisk.go (56%) diff --git a/config/shared/errors/errors.go b/config/shared/errors/errors.go index 0c1500b3c..0d8ba629b 100644 --- a/config/shared/errors/errors.go +++ b/config/shared/errors/errors.go @@ -93,6 +93,7 @@ var ( ErrPathConflictsSystemd = errors.New("path conflicts with systemd unit or dropin") ErrCexWithClevis = errors.New("cannot use cex with clevis") ErrCexWithKeyFile = errors.New("cannot use key file with cex") + ErrBadSfdiskPretend = errors.New("sfdisk had unexpected output while pretending partition configuration on device") // Systemd section errors ErrInvalidSystemdExt = errors.New("invalid systemd unit extension") diff --git a/internal/partitioners/partitioners.go b/internal/partitioners/partitioners.go new file mode 100644 index 000000000..82411551f --- /dev/null +++ b/internal/partitioners/partitioners.go @@ -0,0 +1,47 @@ +// Copyright 2024 Red Hat, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package partitioners + +import ( + "github.com/coreos/ignition/v2/config/v3_7_experimental/types" +) + +type DeviceManager interface { + CreatePartition(p Partition) + DeletePartition(num int) + Info(num int) + WipeTable(wipe bool) + Pretend() (string, error) + Commit() error + ParseOutput(string, []int) (map[int]Output, error) + NeedsPartx() bool + WritesCompleteTable() bool +} + +// Partition wraps config types with sector-level positioning. +// StartMiB/SizeMiB shadow the config fields to prevent accidental use; +// callers should use StartSector/SizeInSectors instead. +type Partition struct { + types.Partition + StartSector *int64 + SizeInSectors *int64 + StartMiB string + SizeMiB string +} + +type Output struct { + Start int64 + Size int64 +} diff --git a/internal/partitioners/sfdisk/sfdisk.go b/internal/partitioners/sfdisk/sfdisk.go new file mode 100644 index 000000000..465f07704 --- /dev/null +++ b/internal/partitioners/sfdisk/sfdisk.go @@ -0,0 +1,461 @@ +// Copyright 2024 Red Hat, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sfdisk + +import ( + "bytes" + "fmt" + "io" + "os/exec" + "regexp" + "sort" + "strconv" + "strings" + + sharedErrors "github.com/coreos/ignition/v2/config/shared/errors" + "github.com/coreos/ignition/v2/config/util" + "github.com/coreos/ignition/v2/internal/distro" + "github.com/coreos/ignition/v2/internal/log" + "github.com/coreos/ignition/v2/internal/partitioners" +) + +type Operation struct { + logger *log.Logger + dev string + wipe bool + parts []partitioners.Partition + deletions []int + infos []int +} + +// Begin begins an sfdisk operation +func Begin(logger *log.Logger, dev string) *Operation { + return &Operation{logger: logger, dev: dev} +} + +// CreatePartition adds the supplied partition to the list of partitions to be created as part of an operation. +func (op *Operation) CreatePartition(p partitioners.Partition) { + op.parts = append(op.parts, p) +} + +func (op *Operation) DeletePartition(num int) { + op.deletions = append(op.deletions, num) +} + +func (op *Operation) Info(num int) { + op.infos = append(op.infos, num) +} + +// WipeTable toggles if the table is to be wiped first when committing this operation. +func (op *Operation) WipeTable(wipe bool) { + op.wipe = wipe +} + +func (op *Operation) NeedsPartx() bool { + return false +} + +func (op *Operation) WritesCompleteTable() bool { + return true +} + +// getLastLBA reads the last usable LBA from the device's GPT header using +// sfdisk --dump. Returns -1 if unavailable. +func (op *Operation) getLastLBA() int64 { + cmd := exec.Command(distro.SfdiskCmd(), "--dump", op.dev) + stdout, err := cmd.Output() + if err != nil { + return -1 + } + re := regexp.MustCompile(`(?m)^last-lba:\s*(\d+)`) + match := re.FindSubmatch(stdout) + if len(match) >= 2 { + v, err := strconv.ParseInt(string(match[1]), 10, 64) + if err == nil { + return v + } + } + return -1 +} + +// readExistingPartitions reads the current partition table from the device +// using sfdisk --dump. Returns a map of partition number to Partition. +func (op *Operation) readExistingPartitions() (map[int]partitioners.Partition, error) { + cmd := exec.Command(distro.SfdiskCmd(), "--dump", op.dev) + stdout, err := cmd.Output() + if err != nil { + // If sfdisk fails (e.g., no partition table), return empty map + return map[int]partitioners.Partition{}, nil + } + + partitions := make(map[int]partitioners.Partition) + // Match lines like: /dev/sda1 : start= 2048, size= 65536, type=..., uuid=..., name="..." + lineRegex := regexp.MustCompile(`^(/dev/\S+)\s*:\s*(.*)$`) + // Extract partition number from device name: /dev/sda1 -> 1, /dev/nvme0n1p2 -> 2 + numRegex := regexp.MustCompile(`(\d+)$`) + + for _, line := range strings.Split(string(stdout), "\n") { + line = strings.TrimSpace(line) + matches := lineRegex.FindStringSubmatch(line) + if matches == nil { + continue + } + + devName := matches[1] + attrs := matches[2] + + numMatch := numRegex.FindStringSubmatch(devName) + if numMatch == nil { + continue + } + partNum, err := strconv.Atoi(numMatch[1]) + if err != nil { + continue + } + + p := partitioners.Partition{} + p.Number = partNum + + // Parse key=value pairs from the attributes + for _, field := range strings.Split(attrs, ",") { + field = strings.TrimSpace(field) + kv := strings.SplitN(field, "=", 2) + if len(kv) != 2 { + continue + } + key := strings.TrimSpace(kv[0]) + value := strings.TrimSpace(kv[1]) + + switch key { + case "start": + v, err := strconv.ParseInt(value, 10, 64) + if err == nil { + p.StartSector = &v + } + case "size": + v, err := strconv.ParseInt(value, 10, 64) + if err == nil { + p.SizeInSectors = &v + } + case "type": + p.TypeGUID = util.StrToPtr(value) + case "uuid": + p.GUID = util.StrToPtr(value) + case "name": + // Remove surrounding quotes + value = strings.Trim(value, "\"") + p.Label = util.StrToPtr(value) + } + } + + partitions[partNum] = p + } + + return partitions, nil +} + +// writePartitionLine writes a single partition line to the script buffer. +// lastLBA is the last usable sector; if >= 0, fill-remaining partitions get an +// explicit size matching sgdisk's inclusive interpretation of last-lba. +func writePartitionLine(script *bytes.Buffer, p partitioners.Partition, lastLBA int64) { + var line bytes.Buffer + + if p.Number > 0 { + fmt.Fprintf(&line, "%d :", p.Number) + } else { + line.WriteString(":") + } + + if p.StartSector != nil && *p.StartSector != 0 { + fmt.Fprintf(&line, " start=%d,", *p.StartSector) + } + + if p.SizeInSectors != nil && *p.SizeInSectors != 0 { + fmt.Fprintf(&line, " size=%d,", *p.SizeInSectors) + } else if lastLBA >= 0 && p.StartSector != nil && *p.StartSector > 0 { + // sfdisk's "fill remaining" leaves 1 sector unused compared to + // sgdisk. Compute the exact size to match sgdisk behaviour. + fmt.Fprintf(&line, " size=%d,", lastLBA-*p.StartSector+1) + } else { + line.WriteString(" size=+,") + } + + if util.NotEmpty(p.TypeGUID) { + fmt.Fprintf(&line, " type=%s,", *p.TypeGUID) + } + + if util.NotEmpty(p.GUID) { + fmt.Fprintf(&line, " uuid=%s,", *p.GUID) + } + + if p.Label != nil { + fmt.Fprintf(&line, " name=\"%s\",", *p.Label) + } + + script.WriteString(strings.TrimSuffix(line.String(), ",")) + script.WriteString("\n") +} + +// buildCompleteScript constructs an sfdisk script from the given partition map. +// lastLBA is forwarded to writePartitionLine for fill-remaining size +// calculation; pass -1 if unknown. +func buildCompleteScript(partitions map[int]partitioners.Partition, lastLBA int64) string { + script := &bytes.Buffer{} + script.WriteString("label: gpt\n") + script.WriteString("grain: 512\n\n") + + // Sort partition numbers: numbered partitions first (ascending), then auto-numbered (0) + numbers := make([]int, 0, len(partitions)) + for num := range partitions { + numbers = append(numbers, num) + } + sort.Slice(numbers, func(i, j int) bool { + if numbers[i] == 0 { + return false + } + if numbers[j] == 0 { + return true + } + return numbers[i] < numbers[j] + }) + + for _, num := range numbers { + p := partitions[num] + writePartitionLine(script, p, lastLBA) + } + + return script.String() +} + +// Pretend is like Commit() but uses the --no-act flag and returns the output +// on stdout for parsing. +func (op *Operation) Pretend() (string, error) { + var existing map[int]partitioners.Partition + var err error + + if op.wipe { + existing = make(map[int]partitioners.Partition) + } else { + existing, err = op.readExistingPartitions() + if err != nil { + return "", err + } + } + + // Apply deletions + for _, num := range op.deletions { + delete(existing, num) + } + + // Apply creations + for _, p := range op.parts { + existing[p.Number] = p + } + + lastLBA := op.getLastLBA() + scriptContent := buildCompleteScript(existing, lastLBA) + + if op.logger != nil { + op.logger.Info("running sfdisk --no-act with script:\n%s", scriptContent) + } + + cmd := exec.Command(distro.SfdiskCmd(), "--no-act", "-X", "gpt", op.dev) + cmd.Stdin = strings.NewReader(scriptContent) + + stdout, err := cmd.StdoutPipe() + if err != nil { + return "", err + } + stderr, err := cmd.StderrPipe() + if err != nil { + return "", err + } + + if err := cmd.Start(); err != nil { + return "", err + } + + output, err := io.ReadAll(stdout) + if err != nil { + return "", err + } + + errors, err := io.ReadAll(stderr) + if err != nil { + return "", err + } + + if err := cmd.Wait(); err != nil { + return "", fmt.Errorf("failed to pretend to create partitions. Err: %v. Stderr: %v", err, string(errors)) + } + + return string(output), nil +} + +// Commit commits a partitioning operation. +func (op *Operation) Commit() error { + var existing map[int]partitioners.Partition + + if op.wipe { + // Create empty GPT table first + if op.logger != nil { + op.logger.Info("wiping partition table on %q", op.dev) + } + cmd := exec.Command(distro.SfdiskCmd(), "--wipe", "always", "--label", "gpt", op.dev) + cmd.Stdin = strings.NewReader("label: gpt\n") + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("failed to wipe partition table on %q: %v: %s", op.dev, err, string(output)) + } + existing = make(map[int]partitioners.Partition) + } else { + // Read the current table so that partitions present on disk but + // not referenced in the ignition config are preserved. The + // caller already passes matching/modified partitions via + // CreatePartition, but unreferenced ones would be lost without + // this read-modify-write. + var err error + existing, err = op.readExistingPartitions() + if err != nil { + return err + } + } + + // Apply deletions + for _, num := range op.deletions { + delete(existing, num) + } + + // Apply creations + for _, p := range op.parts { + existing[p.Number] = p + } + + if len(existing) == 0 && !op.wipe { + return nil + } + + // Handle info requests + if err := op.handleInfo(); err != nil { + return err + } + + if len(existing) == 0 { + return nil + } + + lastLBA := op.getLastLBA() + scriptContent := buildCompleteScript(existing, lastLBA) + + if op.logger != nil { + op.logger.Info("running sfdisk with script:\n%s", scriptContent) + } + + cmd := exec.Command(distro.SfdiskCmd(), "--wipe", "auto", "-X", "gpt", op.dev) + cmd.Stdin = strings.NewReader(scriptContent) + output, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("sfdisk failed on %q: %v: %s", op.dev, err, string(output)) + } + + return nil +} + +// ParseOutput parses the table-format output from sfdisk (the "New situation:" +// section after --no-act) and returns a map of partition number to Output. +func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (map[int]partitioners.Output, error) { + // Check for error/failure keywords only in the preamble before the + // partition table. The table itself can contain type names like + // "Linux filesystem" that are harmless, and user-supplied partition + // labels could also contain "error" or "failed". + preamble := sfdiskOutput + if idx := strings.Index(sfdiskOutput, "Device"); idx >= 0 { + preamble = sfdiskOutput[:idx] + } + lower := strings.ToLower(preamble) + if strings.Contains(lower, "failed") || strings.Contains(lower, "error") { + return nil, fmt.Errorf("%w: %s", sharedErrors.ErrBadSfdiskPretend, sfdiskOutput) + } + + result := make(map[int]partitioners.Output) + + // Match lines like: /dev/vda1 2048 67583 65536 32M Linux filesystem + // Device, optional boot flag (*), start, end, sectors, size, type + partitionRegex := regexp.MustCompile(`^/dev/\S+\s+\*?\s*(\d+)\s+(\d+)\s+(\d+)\s+`) + + // Extract partition number from device name + numRegex := regexp.MustCompile(`^(/dev/\S+)`) + devNumRegex := regexp.MustCompile(`(\d+)$`) + + for _, line := range strings.Split(sfdiskOutput, "\n") { + line = strings.TrimSpace(line) + matches := partitionRegex.FindStringSubmatch(line) + if matches == nil { + continue + } + + // Get device name to extract partition number + devMatch := numRegex.FindStringSubmatch(line) + if devMatch == nil { + continue + } + devName := devMatch[1] + partNumMatch := devNumRegex.FindStringSubmatch(devName) + if partNumMatch == nil { + continue + } + partNum, err := strconv.Atoi(partNumMatch[1]) + if err != nil { + continue + } + + start, err := strconv.ParseInt(matches[1], 10, 64) + if err != nil { + continue + } + + end, err := strconv.ParseInt(matches[2], 10, 64) + if err != nil { + continue + } + + size := end - start + 1 + + result[partNum] = partitioners.Output{ + Start: start, + Size: size, + } + } + + return result, nil +} + +// handleInfo logs partition information for each requested partition number. +func (op *Operation) handleInfo() error { + if len(op.infos) == 0 { + return nil + } + + cmd := exec.Command(distro.SfdiskCmd(), "--list", op.dev) + stdout, err := cmd.Output() + if err != nil { + return fmt.Errorf("sfdisk --list failed on %q: %v", op.dev, err) + } + + if op.logger != nil { + op.logger.Info("partition info for %q (requested partitions %v):\n%s", op.dev, op.infos, string(stdout)) + } + + return nil +} diff --git a/internal/partitioners/sfdisk/sfdisk_test.go b/internal/partitioners/sfdisk/sfdisk_test.go new file mode 100644 index 000000000..1d4219382 --- /dev/null +++ b/internal/partitioners/sfdisk/sfdisk_test.go @@ -0,0 +1,291 @@ +// Copyright 2024 Red Hat, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sfdisk + +import ( + "bytes" + "errors" + "strings" + "testing" + + sharedErrors "github.com/coreos/ignition/v2/config/shared/errors" + "github.com/coreos/ignition/v2/internal/partitioners" +) + +func TestParseOutputSinglePartition(t *testing.T) { + op := Begin(nil, "") + + sfdiskOutput := `Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors +Units: sectors of 1 * 512 = 512 bytes +Sector size (logical/physical): 512 bytes / 512 bytes +I/O size (minimum/optimal): 512 bytes / 512 bytes + +>>> Created a new GPT disklabel. +/dev/vda1: Created a new partition 1 of type 'Linux filesystem' and of size 32 MiB. +/dev/vda2: Done. + +New situation: +Disklabel type: gpt + +Device Start End Sectors Size Type +/dev/vda1 2048 67583 65536 32M Linux filesystem + +The partition table is unchanged (--no-act).` + + result, err := op.ParseOutput(sfdiskOutput, []int{1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(result) != 1 { + t.Fatalf("expected 1 partition, got %d", len(result)) + } + + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) +} + +func TestParseOutputTwoPartitions(t *testing.T) { + op := Begin(nil, "") + + sfdiskOutput := `Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors + +New situation: +Disklabel type: gpt + +Device Start End Sectors Size Type +/dev/vda1 2048 67583 65536 32M Linux filesystem +/dev/vda2 67584 133119 65536 32M Linux filesystem + +The partition table is unchanged (--no-act).` + + result, err := op.ParseOutput(sfdiskOutput, []int{1, 2}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(result) != 2 { + t.Fatalf("expected 2 partitions, got %d", len(result)) + } + + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) + assertOutput(t, result, 2, partitioners.Output{Start: 67584, Size: 65536}) +} + +func TestParseOutputError(t *testing.T) { + op := Begin(nil, "") + + sfdiskOutput := `/dev/vda1: Start sector 0 out of range. +Failed to add #1 partition: Numerical result out of range` + + _, err := op.ParseOutput(sfdiskOutput, []int{1}) + if err == nil { + t.Fatal("expected error, got nil") + } + + if !errors.Is(err, sharedErrors.ErrBadSfdiskPretend) { + t.Fatalf("expected error wrapping ErrBadSfdiskPretend, got: %v", err) + } +} + +func TestParseOutputNoFalsePositiveOnTableContent(t *testing.T) { + op := Begin(nil, "") + + sfdiskOutput := `Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors + +New situation: +Disklabel type: gpt + +Device Start End Sectors Size Type +/dev/vda1 2048 67583 65536 32M Linux filesystem + +The partition table is unchanged (--no-act).` + + result, err := op.ParseOutput(sfdiskOutput, []int{1}) + if err != nil { + t.Fatalf("unexpected error (false positive on table content): %v", err) + } + + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) +} + +func TestBuildCompleteScriptFillRemaining(t *testing.T) { + start := int64(67584) + size := int64(65536) + partitions := map[int]partitioners.Partition{ + 1: {StartSector: &start, SizeInSectors: &size}, + 2: {StartSector: &start}, + } + partitions[1] = partitioners.Partition{StartSector: func() *int64 { v := int64(2048); return &v }(), SizeInSectors: &size} + partitions[2] = partitioners.Partition{StartSector: &start, SizeInSectors: func() *int64 { v := int64(0); return &v }()} + + script := buildCompleteScript(partitions, 204766) + + // Should contain explicit size for fill-remaining partition + if expected := "size=137183"; !strings.Contains(script, expected) { + t.Errorf("expected script to contain %q for fill-remaining partition, got:\n%s", expected, script) + } + if strings.Contains(script, "size=+") { + t.Errorf("script should not contain size=+ when lastLBA is known, got:\n%s", script) + } +} + +func TestBuildCompleteScriptFillRemainingUnknownLBA(t *testing.T) { + start := int64(67584) + partitions := map[int]partitioners.Partition{ + 1: {StartSector: &start, SizeInSectors: func() *int64 { v := int64(0); return &v }()}, + } + + script := buildCompleteScript(partitions, -1) + + if !strings.Contains(script, "size=+") { + t.Errorf("expected script to contain size=+ when lastLBA is unknown, got:\n%s", script) + } +} + +func TestParseOutputNvmeDevice(t *testing.T) { + op := Begin(nil, "") + + sfdiskOutput := `Disk /dev/nvme0n1: 100 GiB, 107374182400 bytes, 209715200 sectors + +New situation: +Disklabel type: gpt + +Device Start End Sectors Size Type +/dev/nvme0n1p1 2048 67583 65536 32M Linux filesystem +/dev/nvme0n1p2 67584 133119 65536 32M Linux filesystem + +The partition table is unchanged (--no-act).` + + result, err := op.ParseOutput(sfdiskOutput, []int{1, 2}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) + assertOutput(t, result, 2, partitioners.Output{Start: 67584, Size: 65536}) +} + +func TestParseOutputWithBootFlag(t *testing.T) { + op := Begin(nil, "") + + sfdiskOutput := `Disk /dev/sda: 50 GiB + +New situation: +Disklabel type: gpt + +Device Start End Sectors Size Type +/dev/sda1 * 2048 67583 65536 32M EFI System +/dev/sda2 67584 133119 65536 32M Linux filesystem + +The partition table is unchanged (--no-act).` + + result, err := op.ParseOutput(sfdiskOutput, []int{1, 2}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) + assertOutput(t, result, 2, partitioners.Output{Start: 67584, Size: 65536}) +} + +func TestParseOutputEmptyPartitions(t *testing.T) { + op := Begin(nil, "") + result, err := op.ParseOutput("anything", []int{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result) != 0 { + t.Fatalf("expected empty result for empty partitionNumbers, got %v", result) + } +} + +func TestBuildCompleteScriptMixedNumbering(t *testing.T) { + start1 := int64(2048) + size1 := int64(65536) + start2 := int64(67584) + size2 := int64(65536) + + p1 := partitioners.Partition{StartSector: &start1, SizeInSectors: &size1} + p1.Number = 1 + p3 := partitioners.Partition{StartSector: &start2, SizeInSectors: &size2} + p3.Number = 3 + p0 := partitioners.Partition{SizeInSectors: &size2} + p0.Number = 0 + + partitions := map[int]partitioners.Partition{ + 1: p1, + 3: p3, + 0: p0, + } + + script := buildCompleteScript(partitions, -1) + + lines := strings.Split(script, "\n") + var num1Idx, num3Idx, autoIdx int + num1Idx, num3Idx, autoIdx = -1, -1, -1 + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "1 :") { + num1Idx = i + } + if strings.HasPrefix(trimmed, "3 :") { + num3Idx = i + } + if trimmed == ":" || strings.HasPrefix(trimmed, ": ") { + autoIdx = i + } + } + + if num1Idx < 0 || num3Idx < 0 || autoIdx < 0 { + t.Fatalf("missing partition lines in script:\n%s", script) + } + if num1Idx >= num3Idx || num3Idx >= autoIdx { + t.Errorf("expected partition order: 1, 3, auto-numbered; got lines %d, %d, %d in script:\n%s", + num1Idx, num3Idx, autoIdx, script) + } +} + +func TestWritePartitionLineMinimal(t *testing.T) { + p := partitioners.Partition{} + p.Number = 1 + + var buf bytes.Buffer + writePartitionLine(&buf, p, -1) + line := buf.String() + + if !strings.HasPrefix(line, "1 :") { + t.Errorf("expected line to start with '1 :', got %q", line) + } + if !strings.Contains(line, "size=+") { + t.Errorf("expected size=+ for partition with no size, got %q", line) + } + if !strings.HasSuffix(line, "\n") { + t.Errorf("expected line to end with newline, got %q", line) + } +} + +func assertOutput(t *testing.T, result map[int]partitioners.Output, num int, expected partitioners.Output) { + t.Helper() + out, ok := result[num] + if !ok { + t.Fatalf("partition %d not found in result", num) + } + if out.Start != expected.Start { + t.Errorf("partition %d: expected start %d, got %d", num, expected.Start, out.Start) + } + if out.Size != expected.Size { + t.Errorf("partition %d: expected size %d, got %d", num, expected.Size, out.Size) + } +} diff --git a/internal/sgdisk/sgdisk.go b/internal/partitioners/sgdisk/sgdisk.go similarity index 56% rename from internal/sgdisk/sgdisk.go rename to internal/partitioners/sgdisk/sgdisk.go index ffd3d9caa..6604b4e28 100644 --- a/internal/sgdisk/sgdisk.go +++ b/internal/partitioners/sgdisk/sgdisk.go @@ -15,44 +15,38 @@ package sgdisk import ( + "errors" "fmt" "io" "os/exec" + "regexp" + "strconv" + "strings" "github.com/coreos/ignition/v2/config/util" - "github.com/coreos/ignition/v2/config/v3_7_experimental/types" "github.com/coreos/ignition/v2/internal/distro" "github.com/coreos/ignition/v2/internal/log" + "github.com/coreos/ignition/v2/internal/partitioners" ) +var ErrBadSgdiskOutput = errors.New("sgdisk had unexpected output") + type Operation struct { logger *log.Logger dev string wipe bool - parts []Partition + parts []partitioners.Partition deletions []int infos []int } -// We ignore types.Partition.StartMiB/SizeMiB in favor of -// StartSector/SizeInSectors. The caller is expected to do the conversion. -type Partition struct { - types.Partition - StartSector *int64 - SizeInSectors *int64 - - // shadow StartMiB/SizeMiB so they're not accidentally used - StartMiB string - SizeMiB string -} - // Begin begins an sgdisk operation func Begin(logger *log.Logger, dev string) *Operation { return &Operation{logger: logger, dev: dev} } // CreatePartition adds the supplied partition to the list of partitions to be created as part of an operation. -func (op *Operation) CreatePartition(p Partition) { +func (op *Operation) CreatePartition(p partitioners.Partition) { op.parts = append(op.parts, p) } @@ -125,6 +119,83 @@ func (op *Operation) Commit() error { return nil } +// NeedsPartx returns true because sgdisk does not trigger the update of the +// kernel partition table with BLKPG but only uses BLKRRPART which fails +// as soon as one partition of the disk is mounted. +func (op *Operation) NeedsPartx() bool { + return true +} + +func (op *Operation) WritesCompleteTable() bool { + return false +} + +// ParseOutput parses the output of running sgdisk pretend with --info specified for each partition +// number specified in partitionNumbers. E.g. if partitionNumbers is [1,4,5], it is expected that the sgdisk +// output was from running `sgdisk --pretend --info=1 --info=4 --info=5`. It assumes the +// partition labels are well behaved (i.e. contain no control characters). It returns a map of partition +// numbers to Output structs with the start and size information as determined by sgdisk. +// The partition numbers need to be passed in because sgdisk includes them in its output. +func (op *Operation) ParseOutput(sgdiskOut string, partitionNumbers []int) (map[int]partitioners.Output, error) { + if len(partitionNumbers) == 0 { + return nil, nil + } + startRegex := regexp.MustCompile(`^First sector: (\d*) \(.*\)$`) + endRegex := regexp.MustCompile(`^Last sector: (\d*) \(.*\)$`) + const ( + START = iota + END = iota + FAIL_ON_START_END = iota + ) + + output := map[int]partitioners.Output{} + state := START + current := partitioners.Output{} + i := 0 + + lines := strings.Split(sgdiskOut, "\n") + for _, line := range lines { + switch state { + case START: + start, err := parseLine(startRegex, line) + if err != nil { + return nil, err + } + if start != -1 { + current.Start = start + state = END + } + case END: + end, err := parseLine(endRegex, line) + if err != nil { + return nil, err + } + if end != -1 { + current.Size = 1 + end - current.Start + output[partitionNumbers[i]] = current + i++ + if i == len(partitionNumbers) { + state = FAIL_ON_START_END + } else { + current = partitioners.Output{} + state = START + } + } + case FAIL_ON_START_END: + if len(startRegex.FindStringSubmatch(line)) != 0 || + len(endRegex.FindStringSubmatch(line)) != 0 { + return nil, ErrBadSgdiskOutput + } + } + } + + if state != FAIL_ON_START_END { + return nil, ErrBadSgdiskOutput + } + + return output, nil +} + func (op Operation) buildOptions() []string { opts := []string{} @@ -162,16 +233,31 @@ func (op Operation) buildOptions() []string { return opts } -func partitionGetStart(p Partition) string { +func partitionGetStart(p partitioners.Partition) string { if p.StartSector != nil { return fmt.Sprintf("%d", *p.StartSector) } return "0" } -func partitionGetSize(p Partition) string { +func partitionGetSize(p partitioners.Partition) string { if p.SizeInSectors != nil { return fmt.Sprintf("%d", *p.SizeInSectors) } return "0" } + +// parseLine takes a regexp that captures an int64 and a string to match on. On success it returns +// the captured int64 and nil. If the regexp does not match it returns -1 and nil. If it encountered +// an error it returns 0 and the error. +func parseLine(r *regexp.Regexp, line string) (int64, error) { + matches := r.FindStringSubmatch(line) + switch len(matches) { + case 0: + return -1, nil + case 2: + return strconv.ParseInt(matches[1], 10, 64) + default: + return 0, ErrBadSgdiskOutput + } +} From 8b29d3063b5e4bc37ffb23adfcb90e6863522685 Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Thu, 14 May 2026 15:03:04 -0400 Subject: [PATCH 3/6] internal/exec/stages/disks: use DeviceManager interface for partitioning Replace direct sgdisk usage with getDeviceManager(). Include matching partitions in commit for whole-table backends, add waitForUdev after wipe, and gate partx behind NeedsPartx(). --- internal/exec/stages/disks/partitions.go | 154 +++++++---------------- 1 file changed, 42 insertions(+), 112 deletions(-) diff --git a/internal/exec/stages/disks/partitions.go b/internal/exec/stages/disks/partitions.go index 9a7ae9cad..b6915a12e 100644 --- a/internal/exec/stages/disks/partitions.go +++ b/internal/exec/stages/disks/partitions.go @@ -20,12 +20,10 @@ package disks import ( "bufio" - "errors" "fmt" "os" "os/exec" "path/filepath" - "regexp" "sort" "strconv" "strings" @@ -34,13 +32,21 @@ import ( "github.com/coreos/ignition/v2/config/v3_7_experimental/types" "github.com/coreos/ignition/v2/internal/distro" "github.com/coreos/ignition/v2/internal/exec/util" - "github.com/coreos/ignition/v2/internal/sgdisk" + "github.com/coreos/ignition/v2/internal/log" + "github.com/coreos/ignition/v2/internal/partitioners" + "github.com/coreos/ignition/v2/internal/partitioners/sfdisk" + "github.com/coreos/ignition/v2/internal/partitioners/sgdisk" iutil "github.com/coreos/ignition/v2/internal/util" ) -var ( - ErrBadSgdiskOutput = errors.New("sgdisk had unexpected output") -) +func getDeviceManager(logger *log.Logger, dev string) partitioners.DeviceManager { + switch distro.PartitionerBackend() { + case "sfdisk": + return sfdisk.Begin(logger, dev) + default: + return sgdisk.Begin(logger, dev) + } +} // createPartitions creates the partitions described in config.Storage.Disks. func (s stage) createPartitions(config types.Config) error { @@ -75,7 +81,7 @@ func (s stage) createPartitions(config types.Config) error { // partitionMatches determines if the existing partition matches the spec given. See doc/operator notes for what // what it means for an existing partition to match the spec. spec must have non-zero Start and Size. -func partitionMatches(existing util.PartitionInfo, spec sgdisk.Partition) error { +func partitionMatches(existing util.PartitionInfo, spec partitioners.Partition) error { if err := partitionMatchesCommon(existing, spec); err != nil { return err } @@ -87,13 +93,13 @@ func partitionMatches(existing util.PartitionInfo, spec sgdisk.Partition) error // partitionMatchesResize returns if the existing partition should be resized by evaluating if // `resize`field is true and partition matches in all respects except size. -func partitionMatchesResize(existing util.PartitionInfo, spec sgdisk.Partition) bool { +func partitionMatchesResize(existing util.PartitionInfo, spec partitioners.Partition) bool { return cutil.IsTrue(spec.Resize) && partitionMatchesCommon(existing, spec) == nil } // partitionMatchesCommon handles the common tests (excluding the partition size) to determine // if the existing partition matches the spec given. -func partitionMatchesCommon(existing util.PartitionInfo, spec sgdisk.Partition) error { +func partitionMatchesCommon(existing util.PartitionInfo, spec partitioners.Partition) error { if spec.Number != existing.Number { return fmt.Errorf("partition numbers did not match (specified %d, got %d). This should not happen, please file a bug", spec.Number, existing.Number) } @@ -113,7 +119,7 @@ func partitionMatchesCommon(existing util.PartitionInfo, spec sgdisk.Partition) } // partitionShouldBeInspected returns if the partition has zeroes that need to be resolved to sectors. -func partitionShouldBeInspected(part sgdisk.Partition) bool { +func partitionShouldBeInspected(part partitioners.Partition) bool { if part.Number == 0 { return false } @@ -133,17 +139,17 @@ func convertMiBToSectors(mib *int, sectorSize int) *int64 { // getRealStartAndSize returns a map of partition numbers to a struct that contains what their real start // and end sector should be. It runs sgdisk --pretend to determine what the partitions would look like if // everything specified were to be (re)created. -func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo util.DiskInfo) ([]sgdisk.Partition, error) { - partitions := []sgdisk.Partition{} +func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo util.DiskInfo) ([]partitioners.Partition, error) { + partitions := []partitioners.Partition{} for _, cpart := range dev.Partitions { - partitions = append(partitions, sgdisk.Partition{ + partitions = append(partitions, partitioners.Partition{ Partition: cpart, StartSector: convertMiBToSectors(cpart.StartMiB, diskInfo.LogicalSectorSize), SizeInSectors: convertMiBToSectors(cpart.SizeMiB, diskInfo.LogicalSectorSize), }) } - op := sgdisk.Begin(s.Logger, devAlias) + op := getDeviceManager(s.Logger, devAlias) for _, part := range partitions { if info, exists := diskInfo.GetPartition(part.Number); exists { // delete all existing partitions @@ -177,19 +183,19 @@ func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo uti return nil, err } - realDimensions, err := parseSgdiskPretend(output, partitionsToInspect) + realDimensions, err := op.ParseOutput(output, partitionsToInspect) if err != nil { return nil, err } - result := []sgdisk.Partition{} + result := []partitioners.Partition{} for _, part := range partitions { if dims, ok := realDimensions[part.Number]; ok { if part.StartSector != nil { - part.StartSector = &dims.start + part.StartSector = &dims.Start } if part.SizeInSectors != nil { - part.SizeInSectors = &dims.size + part.SizeInSectors = &dims.Size } } result = append(result, part) @@ -197,96 +203,9 @@ func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo uti return result, nil } -type sgdiskOutput struct { - start int64 - size int64 -} - -// parseLine takes a regexp that captures an int64 and a string to match on. On success it returns -// the captured int64 and nil. If the regexp does not match it returns -1 and nil. If it encountered -// an error it returns 0 and the error. -func parseLine(r *regexp.Regexp, line string) (int64, error) { - matches := r.FindStringSubmatch(line) - switch len(matches) { - case 0: - return -1, nil - case 2: - return strconv.ParseInt(matches[1], 10, 64) - default: - return 0, ErrBadSgdiskOutput - } -} - -// parseSgdiskPretend parses the output of running sgdisk pretend with --info specified for each partition -// number specified in partitionNumbers. E.g. if paritionNumbers is [1,4,5], it is expected that the sgdisk -// output was from running `sgdisk --pretend --info=1 --info=4 --info=5`. It assumes the the -// partition labels are well behaved (i.e. contain no control characters). It returns a list of partitions -// matching the partition numbers specified, but with the start and size information as determined by sgdisk. -// The partition numbers need to passed in because sgdisk includes them in its output. -func parseSgdiskPretend(sgdiskOut string, partitionNumbers []int) (map[int]sgdiskOutput, error) { - if len(partitionNumbers) == 0 { - return nil, nil - } - startRegex := regexp.MustCompile(`^First sector: (\d*) \(.*\)$`) - endRegex := regexp.MustCompile(`^Last sector: (\d*) \(.*\)$`) - const ( - START = iota - END = iota - FAIL_ON_START_END = iota - ) - - output := map[int]sgdiskOutput{} - state := START - current := sgdiskOutput{} - i := 0 - - lines := strings.Split(sgdiskOut, "\n") - for _, line := range lines { - switch state { - case START: - start, err := parseLine(startRegex, line) - if err != nil { - return nil, err - } - if start != -1 { - current.start = start - state = END - } - case END: - end, err := parseLine(endRegex, line) - if err != nil { - return nil, err - } - if end != -1 { - current.size = 1 + end - current.start - output[partitionNumbers[i]] = current - i++ - if i == len(partitionNumbers) { - state = FAIL_ON_START_END - } else { - current = sgdiskOutput{} - state = START - } - } - case FAIL_ON_START_END: - if len(startRegex.FindStringSubmatch(line)) != 0 || - len(endRegex.FindStringSubmatch(line)) != 0 { - return nil, ErrBadSgdiskOutput - } - } - } - - if state != FAIL_ON_START_END { - // We stopped parsing in the middle of a info block. Something is wrong - return nil, ErrBadSgdiskOutput - } - - return output, nil -} - // partitionShouldExist returns whether a bool is indicating if a partition should exist or not. // nil (unspecified in json) is treated the same as true. -func partitionShouldExist(part sgdisk.Partition) bool { +func partitionShouldExist(part partitioners.Partition) bool { return !cutil.IsFalse(part.ShouldExist) } @@ -450,7 +369,7 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error { return fmt.Errorf("refusing to operate on directly active disk %q", devAlias) } if cutil.IsTrue(dev.WipeTable) { - op := sgdisk.Begin(s.Logger, devAlias) + op := getDeviceManager(s.Logger, devAlias) s.Info("wiping partition table requested on %q", devAlias) if len(activeParts) > 0 { return fmt.Errorf("refusing to wipe active disk %q", devAlias) @@ -464,12 +383,15 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error { return err } } + if err := s.waitForUdev(blockDevResolved); err != nil { + return fmt.Errorf("failed to wait for udev after wipe on %q: %v", blockDevResolved, err) + } } // Ensure all partitions with number 0 are last sort.Stable(PartitionList(dev.Partitions)) - op := sgdisk.Begin(s.Logger, devAlias) + op := getDeviceManager(s.Logger, devAlias) diskInfo, err := s.getPartitionMap(devAlias) if err != nil { @@ -519,6 +441,14 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error { partxDelete = append(partxDelete, uint64(part.Number)) case exists && shouldExist && matches: s.Info("partition %d found with correct specifications", part.Number) + if op.WritesCompleteTable() { + part.StartSector = &info.StartSector + part.SizeInSectors = &info.SizeInSectors + part.TypeGUID = &info.TypeGUID + part.GUID = &info.GUID + part.Label = &info.Label + op.CreatePartition(part) + } case exists && shouldExist && !wipeEntry && !matches: if partitionMatchesResize(info, part) { s.Info("resizing partition %d", part.Number) @@ -554,10 +484,10 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error { return fmt.Errorf("commit failure: %v", err) } - // In contrast to similar tools, sgdisk does not trigger the update of the - // kernel partition table with BLKPG but only uses BLKRRPART which fails - // as soon as one partition of the disk is mounted - if len(activeParts) > 0 { + // sgdisk does not trigger the update of the kernel partition table with + // BLKPG but only uses BLKRRPART which fails as soon as one partition of + // the disk is mounted. sfdisk handles this natively. + if op.NeedsPartx() && len(activeParts) > 0 { runPartxCommand := func(op string, partitions []uint64) error { for _, partNr := range partitions { cmd := exec.Command(distro.PartxCmd(), "--"+op, "--nr", strconv.FormatUint(partNr, 10), blockDevResolved) From a33df65d219b2abc4093719236dda008cc2a7cec Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Thu, 14 May 2026 15:03:11 -0400 Subject: [PATCH 4/6] tests: support sfdisk fallback in validator Add sfdisk fallback to getPartitionSet and getPartitionInfo when sgdisk is unavailable. Add manual comparison script. --- tests/manual/compare-partitioners.sh | 160 +++++++++++++++++++++++++++ tests/validator.go | 117 ++++++++++++++------ 2 files changed, 241 insertions(+), 36 deletions(-) create mode 100644 tests/manual/compare-partitioners.sh diff --git a/tests/manual/compare-partitioners.sh b/tests/manual/compare-partitioners.sh new file mode 100644 index 000000000..fbf9fff5e --- /dev/null +++ b/tests/manual/compare-partitioners.sh @@ -0,0 +1,160 @@ +#!/bin/bash +# Compare sgdisk and sfdisk partition output for identical Ignition configs. +# Must run as root on a system with loop device support. +# Usage: sudo ./tests/manual/compare-partitioners.sh [path-to-ignition-binary] +set -euo pipefail + +IGNITION="${1:-./bin/amd64/ignition}" +IMG_SIZE=200M +PASS=0 +FAIL=0 + +if [ "$(id -u)" -ne 0 ]; then + echo "ERROR: must run as root" >&2 + exit 1 +fi + +if ! command -v sgdisk &>/dev/null || ! command -v sfdisk &>/dev/null; then + echo "ERROR: both sgdisk and sfdisk must be installed" >&2 + exit 1 +fi + +tmpdir=$(mktemp -d /var/tmp/partitioner-compare.XXXXXX) +trap 'rm -rf "$tmpdir"' EXIT + +dump_partition_table() { + local dev="$1" + # Normalize output: partition number, start, size, type GUID, partition GUID, label + sfdisk --dump "$dev" 2>/dev/null | grep "^/dev/" | while read -r line; do + local start size type uuid name + start=$(echo "$line" | sed -n 's/.*start=\s*\([0-9]*\).*/\1/p') + size=$(echo "$line" | sed -n 's/.*size=\s*\([0-9]*\).*/\1/p') + type=$(echo "$line" | sed -n 's/.*type=\s*\([^ ,]*\).*/\1/p') + uuid=$(echo "$line" | sed -n 's/.*uuid=\s*\([^ ,]*\).*/\1/p') + name=$(echo "$line" | sed -n 's/.*name="\([^"]*\)".*/\1/p') + echo "start=$start size=$size type=$type name=$name" + done +} + +run_test() { + local test_name="$1" + local config="$2" + local setup_script="${3:-}" + + echo "=== TEST: $test_name ===" + + # --- sgdisk run --- + local img_sg="$tmpdir/sgdisk.img" + truncate -s "$IMG_SIZE" "$img_sg" + local loop_sg + loop_sg=$(losetup --find --show "$img_sg") + trap "losetup -d $loop_sg 2>/dev/null; rm -f $img_sg" RETURN + + if [ -n "$setup_script" ]; then + eval "$setup_script" "$loop_sg" + fi + + local cfg_sg + cfg_sg=$(echo "$config" | sed "s|\\\$disk|$loop_sg|g") + local cwd_sg="$tmpdir/cwd-sg" + mkdir -p "$cwd_sg" + echo "$cfg_sg" > "$cwd_sg/ignition.json" + touch "$cwd_sg/neednet" "$cwd_sg/state" + + IGNITION_PARTITIONER=sgdisk "$IGNITION" \ + -platform file -stage disks -root "$tmpdir/root-sg" \ + -log-to-stdout -config-cache "$cwd_sg/ignition.json" \ + -neednet "$cwd_sg/neednet" -state-file "$cwd_sg/state" \ + 2>&1 | tail -5 || true + + local sg_result + sg_result=$(dump_partition_table "$loop_sg") + losetup -d "$loop_sg" + + # --- sfdisk run --- + local img_sf="$tmpdir/sfdisk.img" + truncate -s "$IMG_SIZE" "$img_sf" + local loop_sf + loop_sf=$(losetup --find --show "$img_sf") + + if [ -n "$setup_script" ]; then + eval "$setup_script" "$loop_sf" + fi + + local cfg_sf + cfg_sf=$(echo "$config" | sed "s|\\\$disk|$loop_sf|g") + local cwd_sf="$tmpdir/cwd-sf" + mkdir -p "$cwd_sf" + echo "$cfg_sf" > "$cwd_sf/ignition.json" + touch "$cwd_sf/neednet" "$cwd_sf/state" + + IGNITION_PARTITIONER=sfdisk "$IGNITION" \ + -platform file -stage disks -root "$tmpdir/root-sf" \ + -log-to-stdout -config-cache "$cwd_sf/ignition.json" \ + -neednet "$cwd_sf/neednet" -state-file "$cwd_sf/state" \ + 2>&1 | tail -5 || true + + local sf_result + sf_result=$(dump_partition_table "$loop_sf") + losetup -d "$loop_sf" + + # --- Compare --- + if [ "$sg_result" = "$sf_result" ]; then + echo " PASS: partition tables match" + echo " Result: $sg_result" + ((PASS++)) + else + echo " FAIL: partition tables differ" + echo " sgdisk: $sg_result" + echo " sfdisk: $sf_result" + ((FAIL++)) + fi + echo + + # Cleanup for this test + rm -f "$img_sg" "$img_sf" + rm -rf "$cwd_sg" "$cwd_sf" "$tmpdir/root-sg" "$tmpdir/root-sf" +} + +# --- Test 1: Simple partition creation --- +run_test "create single partition" '{ + "ignition": {"version": "3.4.0"}, + "storage": {"disks": [{"device": "$disk", "partitions": [ + {"number": 1, "sizeMiB": 32, "label": "testpart", + "typeGuid": "0FC63DAF-8483-4772-8E79-3D69D8477DE4"} + ]}]} +}' + +# --- Test 2: Multiple partitions --- +run_test "create two partitions" '{ + "ignition": {"version": "3.4.0"}, + "storage": {"disks": [{"device": "$disk", "partitions": [ + {"number": 1, "sizeMiB": 32, "label": "part1", + "typeGuid": "0FC63DAF-8483-4772-8E79-3D69D8477DE4"}, + {"number": 2, "sizeMiB": 32, "label": "part2", + "typeGuid": "0FC63DAF-8483-4772-8E79-3D69D8477DE4"} + ]}]} +}' + +# --- Test 3: Wipe table and create --- +run_test "wipe table then create" '{ + "ignition": {"version": "3.4.0"}, + "storage": {"disks": [{"device": "$disk", "wipeTable": true, "partitions": [ + {"number": 1, "sizeMiB": 32, "label": "fresh", + "typeGuid": "0FC63DAF-8483-4772-8E79-3D69D8477DE4"} + ]}]} +}' 'sgdisk --new=1:2048:+65536 --change-name=1:old' + +# --- Test 4: Fill remaining space (sizeMiB 0) --- +run_test "fill remaining space" '{ + "ignition": {"version": "3.4.0"}, + "storage": {"disks": [{"device": "$disk", "partitions": [ + {"number": 1, "sizeMiB": 32, "label": "fixed", + "typeGuid": "0FC63DAF-8483-4772-8E79-3D69D8477DE4"}, + {"number": 2, "sizeMiB": 0, "startMiB": 0, "label": "rest", + "typeGuid": "0FC63DAF-8483-4772-8E79-3D69D8477DE4"} + ]}]} +}' + +echo "=== SUMMARY: $PASS passed, $FAIL failed ===" +[ "$FAIL" -eq 0 ] diff --git a/tests/validator.go b/tests/validator.go index 9e837357e..e9a8d6270 100644 --- a/tests/validator.go +++ b/tests/validator.go @@ -42,25 +42,42 @@ func regexpSearch(itemName, pattern string, data []byte) (string, error) { func getPartitionSet(device string) (map[int]struct{}, error) { sgdiskOverview, err := exec.Command("sgdisk", "-p", device).CombinedOutput() - if err != nil { - return nil, fmt.Errorf("sgdisk -p %s failed: %v", device, err) + if err == nil { + //What this regex means: num start end size,code,name + re := regexp.MustCompile("\n\\W+(\\d+)\\W+\\d+\\W+\\d+\\W+\\d+.*") + ret := map[int]struct{}{} + for _, match := range re.FindAllStringSubmatch(string(sgdiskOverview), -1) { + if len(match) == 0 { + continue + } + if len(match) != 2 { + return nil, fmt.Errorf("invalid regex result from parsing sgdisk") + } + num, err := strconv.Atoi(match[1]) + if err != nil { + return nil, err + } + ret[num] = struct{}{} + } + return ret, nil } - //What this regex means: num start end size,code,name - re := regexp.MustCompile("\n\\W+(\\d+)\\W+\\d+\\W+\\d+\\W+\\d+.*") + // Fall back to sfdisk --list + sfdiskOverview, err := exec.Command("sfdisk", "--list", device).CombinedOutput() + if err != nil { + return nil, fmt.Errorf("neither sgdisk -p nor sfdisk --list %s succeeded", device) + } + re := regexp.MustCompile(`^` + regexp.QuoteMeta(device) + `\S*?(\d+)\s+`) ret := map[int]struct{}{} - for _, match := range re.FindAllStringSubmatch(string(sgdiskOverview), -1) { - if len(match) == 0 { - continue - } - if len(match) != 2 { - return nil, fmt.Errorf("invalid regex result from parsing sgdisk") - } - num, err := strconv.Atoi(match[1]) - if err != nil { - return nil, err + for _, line := range strings.Split(string(sfdiskOverview), "\n") { + match := re.FindStringSubmatch(line) + if match != nil { + num, err := strconv.Atoi(match[1]) + if err != nil { + continue + } + ret[num] = struct{}{} } - ret[num] = struct{}{} } return ret, nil } @@ -81,27 +98,7 @@ func validateDisk(t *testing.T, d types.Disk) error { } delete(partitionSet, e.Number) - sgdiskInfo, err := exec.Command( - "sgdisk", "-i", strconv.Itoa(e.Number), - d.Device).CombinedOutput() - if err != nil { - t.Error("sgdisk -i", strconv.Itoa(e.Number), err) - return nil - } - - actualGUID, err := regexpSearch("GUID", "Partition unique GUID: (?P[\\d\\w-]+)", sgdiskInfo) - if err != nil { - return err - } - actualTypeGUID, err := regexpSearch("type GUID", "Partition GUID code: (?P[\\d\\w-]+)", sgdiskInfo) - if err != nil { - return err - } - actualSectors, err := regexpSearch("partition size", "Partition size: (?P\\d+) sectors", sgdiskInfo) - if err != nil { - return err - } - actualLabel, err := regexpSearch("partition name", "Partition name: '(?P[\\d\\w-_]+)'", sgdiskInfo) + actualGUID, actualTypeGUID, actualSectors, actualLabel, err := getPartitionInfo(d.Device, e.Number) if err != nil { return err } @@ -135,6 +132,54 @@ func validateDisk(t *testing.T, d types.Disk) error { return nil } +func getPartitionInfo(device string, partNum int) (guid, typeGUID, sectors, label string, err error) { + numStr := strconv.Itoa(partNum) + + // Try sgdisk first + sgdiskInfo, sgErr := exec.Command("sgdisk", "-i", numStr, device).CombinedOutput() + if sgErr == nil { + guid, err = regexpSearch("GUID", "Partition unique GUID: (?P[\\d\\w-]+)", sgdiskInfo) + if err != nil { + return + } + typeGUID, err = regexpSearch("type GUID", "Partition GUID code: (?P[\\d\\w-]+)", sgdiskInfo) + if err != nil { + return + } + sectors, err = regexpSearch("partition size", "Partition size: (?P\\d+) sectors", sgdiskInfo) + if err != nil { + return + } + label, err = regexpSearch("partition name", "Partition name: '(?P[\\d\\w-_]+)'", sgdiskInfo) + return + } + + // Fall back to sfdisk + if out, e := exec.Command("sfdisk", "--part-uuid", device, numStr).CombinedOutput(); e == nil { + guid = strings.TrimSpace(string(out)) + } + if out, e := exec.Command("sfdisk", "--part-type", device, numStr).CombinedOutput(); e == nil { + typeGUID = strings.TrimSpace(string(out)) + } + if out, e := exec.Command("sfdisk", "--part-label", device, numStr).CombinedOutput(); e == nil { + label = strings.TrimSpace(string(out)) + } + + // Get sectors from sfdisk --list output + listOut, listErr := exec.Command("sfdisk", "--list", device).CombinedOutput() + if listErr != nil { + err = fmt.Errorf("neither sgdisk -i nor sfdisk commands succeeded for partition %d on %s", partNum, device) + return + } + re := regexp.MustCompile(regexp.QuoteMeta(device) + `\S*?` + numStr + `\s+\*?\s*\d+\s+\d+\s+(\d+)\s+`) + match := re.FindSubmatch(listOut) + if len(match) >= 2 { + sectors = string(match[1]) + } + + return +} + func formatUUID(s string) string { return strings.ToUpper(strings.ReplaceAll(s, "-", "")) } From 3f106fc9c9909bca33a7c89bdbce9f12da610e56 Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Fri, 15 May 2026 09:13:02 -0400 Subject: [PATCH 5/6] distro: temporarily default to sfdisk for CI testing Flip the default partitioner from sgdisk to sfdisk so blackbox tests exercise the sfdisk backend. This commit should be reverted or replaced with a proper compile-time default mechanism. --- internal/distro/distro.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/distro/distro.go b/internal/distro/distro.go index 343cad9ff..d98e14ec7 100644 --- a/internal/distro/distro.go +++ b/internal/distro/distro.go @@ -168,7 +168,7 @@ func readPartitionerFromCmdline() string { } cmdline, err := os.ReadFile(kernelCmdlinePath) if err != nil { - return "sgdisk" + return "sfdisk" } for _, arg := range strings.Split(strings.TrimSpace(string(cmdline)), " ") { parts := strings.SplitN(arg, "=", 2) @@ -179,7 +179,7 @@ func readPartitionerFromCmdline() string { } } } - return "sgdisk" + return "sfdisk" } func fromEnv(nameSuffix, defaultValue string) string { From ccef6492e6db8eb182e5d00d792c3a6aab0c8d0c Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Tue, 2 Jun 2026 10:03:35 -0400 Subject: [PATCH 6/6] internal/partitioners/sfdisk: rewrite to whole-table with correct fixes Revert to whole-table script approach (which had only 8 failures in CI) and apply all discovered fixes: - Slice-based merge for auto-numbered partitions (not map) - Regex matches any absolute path (not just /dev/) - Sort: fixed-size partitions first, fill-remaining last (prevents overlap in --no-act) - last-lba header in script for correct fill-remaining bounds - ParseOutput correction: +1 size when partition ends at lastLBA-1 - ensureGPTLabel: create empty GPT on disks without one so getLastLBA can read the correct value from the header - --wipe-partitions always on both Pretend and Commit - WritesCompleteTable returns true --- internal/partitioners/sfdisk/sfdisk.go | 238 ++++++++++---------- internal/partitioners/sfdisk/sfdisk_test.go | 207 ++++++++--------- 2 files changed, 219 insertions(+), 226 deletions(-) diff --git a/internal/partitioners/sfdisk/sfdisk.go b/internal/partitioners/sfdisk/sfdisk.go index 465f07704..bcafa7aa8 100644 --- a/internal/partitioners/sfdisk/sfdisk.go +++ b/internal/partitioners/sfdisk/sfdisk.go @@ -38,14 +38,13 @@ type Operation struct { parts []partitioners.Partition deletions []int infos []int + lastLBA int64 } -// Begin begins an sfdisk operation func Begin(logger *log.Logger, dev string) *Operation { return &Operation{logger: logger, dev: dev} } -// CreatePartition adds the supplied partition to the list of partitions to be created as part of an operation. func (op *Operation) CreatePartition(p partitioners.Partition) { op.parts = append(op.parts, p) } @@ -58,7 +57,6 @@ func (op *Operation) Info(num int) { op.infos = append(op.infos, num) } -// WipeTable toggles if the table is to be wiped first when committing this operation. func (op *Operation) WipeTable(wipe bool) { op.wipe = wipe } @@ -71,9 +69,29 @@ func (op *Operation) WritesCompleteTable() bool { return true } -// getLastLBA reads the last usable LBA from the device's GPT header using -// sfdisk --dump. Returns -1 if unavailable. +// ensureGPTLabel creates an empty GPT label on the device if one does +// not already exist. This is needed so that getLastLBA can read +// last-lba from the GPT header and so that sfdisk --no-act works. +func (op *Operation) ensureGPTLabel() { + cmd := exec.Command(distro.SfdiskCmd(), "--dump", op.dev) + if out, err := cmd.Output(); err == nil && strings.Contains(string(out), "label:") { + return + } + if op.logger != nil { + op.logger.Info("creating empty GPT label on %q", op.dev) + } + labelCmd := exec.Command(distro.SfdiskCmd(), "-X", "gpt", op.dev) + labelCmd.Stdin = strings.NewReader("label: gpt\n") + if output, err := labelCmd.CombinedOutput(); err != nil { + if op.logger != nil { + op.logger.Warning("failed to create GPT label on %q: %v: %s", op.dev, err, string(output)) + } + } +} + +// getLastLBA reads the last usable LBA from the device's GPT header. func (op *Operation) getLastLBA() int64 { + op.ensureGPTLabel() cmd := exec.Command(distro.SfdiskCmd(), "--dump", op.dev) stdout, err := cmd.Output() if err != nil { @@ -90,20 +108,17 @@ func (op *Operation) getLastLBA() int64 { return -1 } -// readExistingPartitions reads the current partition table from the device -// using sfdisk --dump. Returns a map of partition number to Partition. -func (op *Operation) readExistingPartitions() (map[int]partitioners.Partition, error) { +// readExistingPartitions reads the current partition table using +// sfdisk --dump. +func (op *Operation) readExistingPartitions() ([]partitioners.Partition, error) { cmd := exec.Command(distro.SfdiskCmd(), "--dump", op.dev) stdout, err := cmd.Output() if err != nil { - // If sfdisk fails (e.g., no partition table), return empty map - return map[int]partitioners.Partition{}, nil + return nil, nil } - partitions := make(map[int]partitioners.Partition) - // Match lines like: /dev/sda1 : start= 2048, size= 65536, type=..., uuid=..., name="..." - lineRegex := regexp.MustCompile(`^(/dev/\S+)\s*:\s*(.*)$`) - // Extract partition number from device name: /dev/sda1 -> 1, /dev/nvme0n1p2 -> 2 + var partitions []partitioners.Partition + lineRegex := regexp.MustCompile(`^(/\S+)\s*:\s*(.*)$`) numRegex := regexp.MustCompile(`(\d+)$`) for _, line := range strings.Split(string(stdout), "\n") { @@ -113,10 +128,7 @@ func (op *Operation) readExistingPartitions() (map[int]partitioners.Partition, e continue } - devName := matches[1] - attrs := matches[2] - - numMatch := numRegex.FindStringSubmatch(devName) + numMatch := numRegex.FindStringSubmatch(matches[1]) if numMatch == nil { continue } @@ -128,8 +140,7 @@ func (op *Operation) readExistingPartitions() (map[int]partitioners.Partition, e p := partitioners.Partition{} p.Number = partNum - // Parse key=value pairs from the attributes - for _, field := range strings.Split(attrs, ",") { + for _, field := range strings.Split(matches[2], ",") { field = strings.TrimSpace(field) kv := strings.SplitN(field, "=", 2) if len(kv) != 2 { @@ -154,22 +165,17 @@ func (op *Operation) readExistingPartitions() (map[int]partitioners.Partition, e case "uuid": p.GUID = util.StrToPtr(value) case "name": - // Remove surrounding quotes - value = strings.Trim(value, "\"") - p.Label = util.StrToPtr(value) + p.Label = util.StrToPtr(strings.Trim(value, "\"")) } } - partitions[partNum] = p + partitions = append(partitions, p) } return partitions, nil } -// writePartitionLine writes a single partition line to the script buffer. -// lastLBA is the last usable sector; if >= 0, fill-remaining partitions get an -// explicit size matching sgdisk's inclusive interpretation of last-lba. -func writePartitionLine(script *bytes.Buffer, p partitioners.Partition, lastLBA int64) { +func writePartitionLine(script *bytes.Buffer, p partitioners.Partition) { var line bytes.Buffer if p.Number > 0 { @@ -184,10 +190,6 @@ func writePartitionLine(script *bytes.Buffer, p partitioners.Partition, lastLBA if p.SizeInSectors != nil && *p.SizeInSectors != 0 { fmt.Fprintf(&line, " size=%d,", *p.SizeInSectors) - } else if lastLBA >= 0 && p.StartSector != nil && *p.StartSector > 0 { - // sfdisk's "fill remaining" leaves 1 sector unused compared to - // sgdisk. Compute the exact size to match sgdisk behaviour. - fmt.Fprintf(&line, " size=%d,", lastLBA-*p.StartSector+1) } else { line.WriteString(" size=+,") } @@ -208,70 +210,105 @@ func writePartitionLine(script *bytes.Buffer, p partitioners.Partition, lastLBA script.WriteString("\n") } -// buildCompleteScript constructs an sfdisk script from the given partition map. -// lastLBA is forwarded to writePartitionLine for fill-remaining size -// calculation; pass -1 if unknown. -func buildCompleteScript(partitions map[int]partitioners.Partition, lastLBA int64) string { +// buildScript constructs an sfdisk script. Fixed-size partitions are +// written first (sorted by start sector) so that fill-remaining +// partitions (size=+) see the correct free blocks. +func buildScript(partitions []partitioners.Partition, lastLBA int64) string { script := &bytes.Buffer{} script.WriteString("label: gpt\n") - script.WriteString("grain: 512\n\n") - - // Sort partition numbers: numbered partitions first (ascending), then auto-numbered (0) - numbers := make([]int, 0, len(partitions)) - for num := range partitions { - numbers = append(numbers, num) + script.WriteString("grain: 512\n") + if lastLBA >= 0 { + fmt.Fprintf(script, "last-lba: %d\n", lastLBA) } - sort.Slice(numbers, func(i, j int) bool { - if numbers[i] == 0 { - return false + script.WriteString("\n") + + sorted := make([]partitioners.Partition, len(partitions)) + copy(sorted, partitions) + sort.SliceStable(sorted, func(i, j int) bool { + iFixed := sorted[i].SizeInSectors != nil && *sorted[i].SizeInSectors != 0 + jFixed := sorted[j].SizeInSectors != nil && *sorted[j].SizeInSectors != 0 + if iFixed != jFixed { + return iFixed } - if numbers[j] == 0 { - return true + if iFixed && jFixed { + iStart := int64(0) + jStart := int64(0) + if sorted[i].StartSector != nil { + iStart = *sorted[i].StartSector + } + if sorted[j].StartSector != nil { + jStart = *sorted[j].StartSector + } + return iStart < jStart } - return numbers[i] < numbers[j] + return false }) - for _, num := range numbers { - p := partitions[num] - writePartitionLine(script, p, lastLBA) + for _, p := range sorted { + writePartitionLine(script, p) } return script.String() } -// Pretend is like Commit() but uses the --no-act flag and returns the output -// on stdout for parsing. -func (op *Operation) Pretend() (string, error) { - var existing map[int]partitioners.Partition - var err error +// mergePartitions builds the final partition list from existing disk +// state plus queued operations. +func (op *Operation) mergePartitions() ([]partitioners.Partition, error) { + var merged []partitioners.Partition - if op.wipe { - existing = make(map[int]partitioners.Partition) - } else { - existing, err = op.readExistingPartitions() + if !op.wipe { + existing, err := op.readExistingPartitions() if err != nil { - return "", err + return nil, err } + merged = existing } - // Apply deletions - for _, num := range op.deletions { - delete(existing, num) + for _, delNum := range op.deletions { + var filtered []partitioners.Partition + for _, p := range merged { + if p.Number != delNum { + filtered = append(filtered, p) + } + } + merged = filtered } - // Apply creations for _, p := range op.parts { - existing[p.Number] = p + if p.Number == 0 { + merged = append(merged, p) + } else { + replaced := false + for i := range merged { + if merged[i].Number == p.Number { + merged[i] = p + replaced = true + break + } + } + if !replaced { + merged = append(merged, p) + } + } } - lastLBA := op.getLastLBA() - scriptContent := buildCompleteScript(existing, lastLBA) + return merged, nil +} + +func (op *Operation) Pretend() (string, error) { + merged, err := op.mergePartitions() + if err != nil { + return "", err + } + + op.lastLBA = op.getLastLBA() + scriptContent := buildScript(merged, op.lastLBA) if op.logger != nil { op.logger.Info("running sfdisk --no-act with script:\n%s", scriptContent) } - cmd := exec.Command(distro.SfdiskCmd(), "--no-act", "-X", "gpt", op.dev) + cmd := exec.Command(distro.SfdiskCmd(), "--no-act", "--wipe-partitions", "always", "-X", "gpt", op.dev) cmd.Stdin = strings.NewReader(scriptContent) stdout, err := cmd.StdoutPipe() @@ -304,12 +341,8 @@ func (op *Operation) Pretend() (string, error) { return string(output), nil } -// Commit commits a partitioning operation. func (op *Operation) Commit() error { - var existing map[int]partitioners.Partition - if op.wipe { - // Create empty GPT table first if op.logger != nil { op.logger.Info("wiping partition table on %q", op.dev) } @@ -318,51 +351,29 @@ func (op *Operation) Commit() error { if output, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("failed to wipe partition table on %q: %v: %s", op.dev, err, string(output)) } - existing = make(map[int]partitioners.Partition) - } else { - // Read the current table so that partitions present on disk but - // not referenced in the ignition config are preserved. The - // caller already passes matching/modified partitions via - // CreatePartition, but unreferenced ones would be lost without - // this read-modify-write. - var err error - existing, err = op.readExistingPartitions() - if err != nil { - return err - } } - // Apply deletions - for _, num := range op.deletions { - delete(existing, num) - } - - // Apply creations - for _, p := range op.parts { - existing[p.Number] = p + merged, err := op.mergePartitions() + if err != nil { + return err } - if len(existing) == 0 && !op.wipe { + if len(merged) == 0 { return nil } - // Handle info requests if err := op.handleInfo(); err != nil { return err } - if len(existing) == 0 { - return nil - } - lastLBA := op.getLastLBA() - scriptContent := buildCompleteScript(existing, lastLBA) + scriptContent := buildScript(merged, lastLBA) if op.logger != nil { op.logger.Info("running sfdisk with script:\n%s", scriptContent) } - cmd := exec.Command(distro.SfdiskCmd(), "--wipe", "auto", "-X", "gpt", op.dev) + cmd := exec.Command(distro.SfdiskCmd(), "--wipe", "auto", "--wipe-partitions", "always", "-X", "gpt", op.dev) cmd.Stdin = strings.NewReader(scriptContent) output, err := cmd.CombinedOutput() if err != nil { @@ -372,13 +383,7 @@ func (op *Operation) Commit() error { return nil } -// ParseOutput parses the table-format output from sfdisk (the "New situation:" -// section after --no-act) and returns a map of partition number to Output. func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (map[int]partitioners.Output, error) { - // Check for error/failure keywords only in the preamble before the - // partition table. The table itself can contain type names like - // "Linux filesystem" that are harmless, and user-supplied partition - // labels could also contain "error" or "failed". preamble := sfdiskOutput if idx := strings.Index(sfdiskOutput, "Device"); idx >= 0 { preamble = sfdiskOutput[:idx] @@ -390,12 +395,8 @@ func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (m result := make(map[int]partitioners.Output) - // Match lines like: /dev/vda1 2048 67583 65536 32M Linux filesystem - // Device, optional boot flag (*), start, end, sectors, size, type - partitionRegex := regexp.MustCompile(`^/dev/\S+\s+\*?\s*(\d+)\s+(\d+)\s+(\d+)\s+`) - - // Extract partition number from device name - numRegex := regexp.MustCompile(`^(/dev/\S+)`) + partitionRegex := regexp.MustCompile(`^/\S+\s+\*?\s*(\d+)\s+(\d+)\s+(\d+)\s+`) + numRegex := regexp.MustCompile(`^(/\S+)`) devNumRegex := regexp.MustCompile(`(\d+)$`) for _, line := range strings.Split(sfdiskOutput, "\n") { @@ -405,13 +406,11 @@ func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (m continue } - // Get device name to extract partition number devMatch := numRegex.FindStringSubmatch(line) if devMatch == nil { continue } - devName := devMatch[1] - partNumMatch := devNumRegex.FindStringSubmatch(devName) + partNumMatch := devNumRegex.FindStringSubmatch(devMatch[1]) if partNumMatch == nil { continue } @@ -432,6 +431,12 @@ func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (m size := end - start + 1 + // sfdisk's fill-remaining (size=+) ends 1 sector before lastLBA. + // Correct this to match sgdisk which fills to the exact lastLBA. + if op.lastLBA > 0 && end == op.lastLBA-1 { + size++ + } + result[partNum] = partitioners.Output{ Start: start, Size: size, @@ -441,7 +446,6 @@ func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (m return result, nil } -// handleInfo logs partition information for each requested partition number. func (op *Operation) handleInfo() error { if len(op.infos) == 0 { return nil diff --git a/internal/partitioners/sfdisk/sfdisk_test.go b/internal/partitioners/sfdisk/sfdisk_test.go index 1d4219382..72d08365d 100644 --- a/internal/partitioners/sfdisk/sfdisk_test.go +++ b/internal/partitioners/sfdisk/sfdisk_test.go @@ -17,6 +17,7 @@ package sfdisk import ( "bytes" "errors" + "fmt" "strings" "testing" @@ -25,12 +26,9 @@ import ( ) func TestParseOutputSinglePartition(t *testing.T) { - op := Begin(nil, "") + op := &Operation{lastLBA: 67583} sfdiskOutput := `Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors -Units: sectors of 1 * 512 = 512 bytes -Sector size (logical/physical): 512 bytes / 512 bytes -I/O size (minimum/optimal): 512 bytes / 512 bytes >>> Created a new GPT disklabel. /dev/vda1: Created a new partition 1 of type 'Linux filesystem' and of size 32 MiB. @@ -48,20 +46,13 @@ The partition table is unchanged (--no-act).` if err != nil { t.Fatalf("unexpected error: %v", err) } - - if len(result) != 1 { - t.Fatalf("expected 1 partition, got %d", len(result)) - } - assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) } func TestParseOutputTwoPartitions(t *testing.T) { - op := Begin(nil, "") - - sfdiskOutput := `Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors + op := &Operation{} -New situation: + sfdiskOutput := `New situation: Disklabel type: gpt Device Start End Sectors Size Type @@ -74,17 +65,12 @@ The partition table is unchanged (--no-act).` if err != nil { t.Fatalf("unexpected error: %v", err) } - - if len(result) != 2 { - t.Fatalf("expected 2 partitions, got %d", len(result)) - } - assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) assertOutput(t, result, 2, partitioners.Output{Start: 67584, Size: 65536}) } func TestParseOutputError(t *testing.T) { - op := Begin(nil, "") + op := &Operation{} sfdiskOutput := `/dev/vda1: Start sector 0 out of range. Failed to add #1 partition: Numerical result out of range` @@ -93,18 +79,15 @@ Failed to add #1 partition: Numerical result out of range` if err == nil { t.Fatal("expected error, got nil") } - if !errors.Is(err, sharedErrors.ErrBadSfdiskPretend) { - t.Fatalf("expected error wrapping ErrBadSfdiskPretend, got: %v", err) + t.Fatalf("expected ErrBadSfdiskPretend, got: %v", err) } } func TestParseOutputNoFalsePositiveOnTableContent(t *testing.T) { - op := Begin(nil, "") - - sfdiskOutput := `Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors + op := &Operation{} -New situation: + sfdiskOutput := `New situation: Disklabel type: gpt Device Start End Sectors Size Type @@ -114,52 +97,33 @@ The partition table is unchanged (--no-act).` result, err := op.ParseOutput(sfdiskOutput, []int{1}) if err != nil { - t.Fatalf("unexpected error (false positive on table content): %v", err) + t.Fatalf("unexpected error: %v", err) } - assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) } -func TestBuildCompleteScriptFillRemaining(t *testing.T) { - start := int64(67584) - size := int64(65536) - partitions := map[int]partitioners.Partition{ - 1: {StartSector: &start, SizeInSectors: &size}, - 2: {StartSector: &start}, - } - partitions[1] = partitioners.Partition{StartSector: func() *int64 { v := int64(2048); return &v }(), SizeInSectors: &size} - partitions[2] = partitioners.Partition{StartSector: &start, SizeInSectors: func() *int64 { v := int64(0); return &v }()} - - script := buildCompleteScript(partitions, 204766) +func TestParseOutputLastLBACorrection(t *testing.T) { + op := &Operation{lastLBA: 67583} - // Should contain explicit size for fill-remaining partition - if expected := "size=137183"; !strings.Contains(script, expected) { - t.Errorf("expected script to contain %q for fill-remaining partition, got:\n%s", expected, script) - } - if strings.Contains(script, "size=+") { - t.Errorf("script should not contain size=+ when lastLBA is known, got:\n%s", script) - } -} + sfdiskOutput := `New situation: +Disklabel type: gpt -func TestBuildCompleteScriptFillRemainingUnknownLBA(t *testing.T) { - start := int64(67584) - partitions := map[int]partitioners.Partition{ - 1: {StartSector: &start, SizeInSectors: func() *int64 { v := int64(0); return &v }()}, - } +Device Start End Sectors Size Type +/dev/vda1 2048 67582 65535 32M Linux filesystem - script := buildCompleteScript(partitions, -1) +The partition table is unchanged (--no-act).` - if !strings.Contains(script, "size=+") { - t.Errorf("expected script to contain size=+ when lastLBA is unknown, got:\n%s", script) + result, err := op.ParseOutput(sfdiskOutput, []int{1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) } + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) } func TestParseOutputNvmeDevice(t *testing.T) { - op := Begin(nil, "") - - sfdiskOutput := `Disk /dev/nvme0n1: 100 GiB, 107374182400 bytes, 209715200 sectors + op := &Operation{} -New situation: + sfdiskOutput := `New situation: Disklabel type: gpt Device Start End Sectors Size Type @@ -172,17 +136,32 @@ The partition table is unchanged (--no-act).` if err != nil { t.Fatalf("unexpected error: %v", err) } - assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) assertOutput(t, result, 2, partitioners.Output{Start: 67584, Size: 65536}) } -func TestParseOutputWithBootFlag(t *testing.T) { - op := Begin(nil, "") +func TestParseOutputDeviceAlias(t *testing.T) { + op := &Operation{} - sfdiskOutput := `Disk /dev/sda: 50 GiB + sfdiskOutput := `New situation: +Disklabel type: gpt -New situation: +Device Start End Sectors Size Type +/run/ignition/dev_aliases/dev/loop0p1 2048 67583 65536 32M Linux filesystem + +The partition table is unchanged (--no-act).` + + result, err := op.ParseOutput(sfdiskOutput, []int{1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) +} + +func TestParseOutputWithBootFlag(t *testing.T) { + op := &Operation{} + + sfdiskOutput := `New situation: Disklabel type: gpt Device Start End Sectors Size Type @@ -195,65 +174,75 @@ The partition table is unchanged (--no-act).` if err != nil { t.Fatalf("unexpected error: %v", err) } - assertOutput(t, result, 1, partitioners.Output{Start: 2048, Size: 65536}) assertOutput(t, result, 2, partitioners.Output{Start: 67584, Size: 65536}) } -func TestParseOutputEmptyPartitions(t *testing.T) { - op := Begin(nil, "") - result, err := op.ParseOutput("anything", []int{}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if len(result) != 0 { - t.Fatalf("expected empty result for empty partitionNumbers, got %v", result) - } -} - -func TestBuildCompleteScriptMixedNumbering(t *testing.T) { - start1 := int64(2048) - size1 := int64(65536) - start2 := int64(67584) - size2 := int64(65536) - - p1 := partitioners.Partition{StartSector: &start1, SizeInSectors: &size1} +func TestBuildScriptFixedSizeBeforeFillRemaining(t *testing.T) { + p1 := partitioners.Partition{StartSector: int64Ptr(2048), SizeInSectors: int64Ptr(65536)} p1.Number = 1 - p3 := partitioners.Partition{StartSector: &start2, SizeInSectors: &size2} + p5 := partitioners.Partition{StartSector: int64Ptr(460800), SizeInSectors: int64Ptr(65536)} + p5.Number = 5 + p3 := partitioners.Partition{SizeInSectors: int64Ptr(0)} p3.Number = 3 - p0 := partitioners.Partition{SizeInSectors: &size2} - p0.Number = 0 - partitions := map[int]partitioners.Partition{ - 1: p1, - 3: p3, - 0: p0, - } - - script := buildCompleteScript(partitions, -1) + script := buildScript([]partitioners.Partition{p3, p1, p5}, 657407) lines := strings.Split(script, "\n") - var num1Idx, num3Idx, autoIdx int - num1Idx, num3Idx, autoIdx = -1, -1, -1 + var idx1, idx5, idx3 int = -1, -1, -1 for i, line := range lines { trimmed := strings.TrimSpace(line) if strings.HasPrefix(trimmed, "1 :") { - num1Idx = i + idx1 = i } - if strings.HasPrefix(trimmed, "3 :") { - num3Idx = i + if strings.HasPrefix(trimmed, "5 :") { + idx5 = i } - if trimmed == ":" || strings.HasPrefix(trimmed, ": ") { - autoIdx = i + if strings.HasPrefix(trimmed, "3 :") { + idx3 = i } } - if num1Idx < 0 || num3Idx < 0 || autoIdx < 0 { + if idx1 < 0 || idx5 < 0 || idx3 < 0 { t.Fatalf("missing partition lines in script:\n%s", script) } - if num1Idx >= num3Idx || num3Idx >= autoIdx { - t.Errorf("expected partition order: 1, 3, auto-numbered; got lines %d, %d, %d in script:\n%s", - num1Idx, num3Idx, autoIdx, script) + if idx1 >= idx3 || idx5 >= idx3 { + t.Errorf("expected fixed-size before fill-remaining; 1=%d, 5=%d, 3=%d in:\n%s", idx1, idx5, idx3, script) + } +} + +func TestBuildScriptMultipleAutoNumbered(t *testing.T) { + p1 := partitioners.Partition{SizeInSectors: int64Ptr(65536)} + p1.Number = 0 + p1.Label = strPtr("uno") + p2 := partitioners.Partition{SizeInSectors: int64Ptr(65536)} + p2.Number = 0 + p2.Label = strPtr("dos") + p3 := partitioners.Partition{SizeInSectors: int64Ptr(65536)} + p3.Number = 0 + p3.Label = strPtr("tres") + + script := buildScript([]partitioners.Partition{p1, p2, p3}, -1) + + for _, name := range []string{"uno", "dos", "tres"} { + if !strings.Contains(script, fmt.Sprintf(`name="%s"`, name)) { + t.Errorf("missing partition %q in script:\n%s", name, script) + } + } +} + +func TestBuildScriptLastLBAHeader(t *testing.T) { + p := partitioners.Partition{SizeInSectors: int64Ptr(0)} + p.Number = 1 + + script := buildScript([]partitioners.Partition{p}, 67583) + if !strings.Contains(script, "last-lba: 67583") { + t.Errorf("expected last-lba header, got:\n%s", script) + } + + script = buildScript([]partitioners.Partition{p}, -1) + if strings.Contains(script, "last-lba:") { + t.Errorf("should not contain last-lba when unknown, got:\n%s", script) } } @@ -262,20 +251,20 @@ func TestWritePartitionLineMinimal(t *testing.T) { p.Number = 1 var buf bytes.Buffer - writePartitionLine(&buf, p, -1) + writePartitionLine(&buf, p) line := buf.String() if !strings.HasPrefix(line, "1 :") { - t.Errorf("expected line to start with '1 :', got %q", line) + t.Errorf("expected '1 :', got %q", line) } if !strings.Contains(line, "size=+") { - t.Errorf("expected size=+ for partition with no size, got %q", line) - } - if !strings.HasSuffix(line, "\n") { - t.Errorf("expected line to end with newline, got %q", line) + t.Errorf("expected size=+, got %q", line) } } +func int64Ptr(v int64) *int64 { return &v } +func strPtr(v string) *string { return &v } + func assertOutput(t *testing.T, result map[int]partitioners.Output, num int, expected partitioners.Output) { t.Helper() out, ok := result[num]