feat: auto-discover META partition from full disk via GPT#5
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR refactors talos-meta-tool to discover and write configuration into the GPT META partition instead of a specified device path. It changes writeConfig to accept an io.ReaderAt/io.WriterAt partition handle, adds GPT discovery helpers, updates main to open disks and locate META partitions, and expands test coverage for the new interface and partition discovery scenarios. ChangesGPT META Partition Discovery and Configuration Writing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Talos metadata writer tool to automatically discover the 'META' partition on a disk device using GPT, removing the need to specify the partition manually. It introduces a partAt struct to handle offset-based reads and writes, and adds comprehensive tests using mock disk images. The review feedback suggests improving safety by using a named field instead of embedding *os.File in partAt to avoid exposing un-offsetted methods, adding defensive nil checks for input parameters, and explicitly calling Sync() on the device to ensure data is flushed to disk.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| type partAt struct { | ||
| *os.File | ||
| offset int64 | ||
| } | ||
|
|
||
| func (p *partAt) ReadAt(buf []byte, off int64) (int, error) { | ||
| return p.File.ReadAt(buf, p.offset+off) | ||
| } | ||
|
|
||
| func (p *partAt) WriteAt(buf []byte, off int64) (int, error) { | ||
| return p.File.WriteAt(buf, p.offset+off) | ||
| } |
There was a problem hiding this comment.
Embedding *os.File in partAt exposes all of *os.File's methods (like Read, Write, Seek, Close) to any consumer of partAt. Since these inherited methods do not respect the partition offset, calling them would read/write from the wrong disk locations. It is safer to use a named field file *os.File to prevent accidental misuse of the underlying file descriptor.
| type partAt struct { | |
| *os.File | |
| offset int64 | |
| } | |
| func (p *partAt) ReadAt(buf []byte, off int64) (int, error) { | |
| return p.File.ReadAt(buf, p.offset+off) | |
| } | |
| func (p *partAt) WriteAt(buf []byte, off int64) (int, error) { | |
| return p.File.WriteAt(buf, p.offset+off) | |
| } | |
| type partAt struct { | |
| file *os.File | |
| offset int64 | |
| } | |
| func (p *partAt) ReadAt(buf []byte, off int64) (int, error) { | |
| return p.file.ReadAt(buf, p.offset+off) | |
| } | |
| func (p *partAt) WriteAt(buf []byte, off int64) (int, error) { | |
| return p.file.WriteAt(buf, p.offset+off) | |
| } |
| func findMetaPartition(f *os.File) (interface{ io.ReaderAt; io.WriterAt }, error) { | ||
| gptdev, err := openGPTDevice(f) |
There was a problem hiding this comment.
Add a defensive nil check for f to prevent a nil pointer dereference panic when calling openGPTDevice.
| func findMetaPartition(f *os.File) (interface{ io.ReaderAt; io.WriterAt }, error) { | |
| gptdev, err := openGPTDevice(f) | |
| func findMetaPartition(f *os.File) (interface{ io.ReaderAt; io.WriterAt }, error) { | |
| if f == nil { | |
| return nil, fmt.Errorf("file is nil") | |
| } | |
| gptdev, err := openGPTDevice(f) |
|
|
||
| for _, p := range table.Partitions() { | ||
| if p != nil && p.Name == "META" { | ||
| return &partAt{File: f, offset: int64(p.FirstLBA) * sectorSize}, nil |
There was a problem hiding this comment.
Update the initialization of partAt to use the named file field instead of the embedded File field.
| return &partAt{File: f, offset: int64(p.FirstLBA) * sectorSize}, nil | |
| return &partAt{file: f, offset: int64(p.FirstLBA) * sectorSize}, nil |
| func writeConfig(dev interface{ io.ReaderAt; io.WriterAt }, configData []byte) error { | ||
| adv, loadErr := talos.NewADV(io.NewSectionReader(dev, 0, int64(talos.Size))) |
There was a problem hiding this comment.
Add a defensive nil check for dev to prevent a nil pointer dereference panic when NewSectionReader or NewADV attempts to read from it.
func writeConfig(dev interface{ io.ReaderAt; io.WriterAt }, configData []byte) error {
if dev == nil {
return fmt.Errorf("device is nil")
}
adv, loadErr := talos.NewADV(io.NewSectionReader(dev, 0, int64(talos.Size)))| if err := writeConfig(meta, validatedConfigData); err != nil { | ||
| log.Fatalf("Error: %v", err) | ||
| } |
There was a problem hiding this comment.
When writing to a block device or disk image, it is important to explicitly call Sync() to flush any dirty pages from the OS cache to the physical media before reporting success. This ensures data integrity and prevents silent data loss if the system crashes or the drive is removed immediately after the tool exits.
if err := writeConfig(meta, validatedConfigData); err != nil {
log.Fatalf("Error: %v", err)
}
if err := device.Sync(); err != nil {
log.Fatalf("Error syncing device: %v", err)
}
Add findMetaPartition which reads the GPT table and locates the
partition labelled "META", so -device now accepts a whole disk path
(e.g. /dev/sda) instead of requiring the caller to know the partition
number. Real block devices use ioctl (BLKGETSIZE64) for correct size
via go-blockdevice; regular files fall back to stat, keeping tests
root-free. Add //go:build linux since the tool targets Talos Linux.
writeConfig is refactored to accept interface{ io.ReaderAt; io.WriterAt }
instead of a device path, which lets it operate on the offset-scoped
partition view returned by findMetaPartition.
Add a full-disk test that builds a 20 MiB GPT image with all six Talos
partition labels (EFI/BIOS/BOOT/META/STATE/EPHEMERAL) using
go-blockdevice/v2, exercises writeConfig via findMetaPartition, and
reads back through the partition table to verify the data landed in the
right place. Also cover the error paths: zeroed disk (no GPT) and valid
GPT without a META partition.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Mickaël Canévet <mickael.canevet@proton.ch>
Mickaël Canévet (mcanevet)
force-pushed
the
feat/gpt-auto-discover
branch
from
f6b82e3 to
c9cfcd6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
9-11: ⚡ Quick winAdd language identifiers to fenced code blocks.
Specifying the language improves syntax highlighting and tooling support.
📝 Proposed fixes
Compile: -``` +```bash GOOS=linux GOARCH=amd64 go build -o talos-meta-tool .Usage:
-+bash
talos-meta-tool -device /dev/sda -config config.yamlAlso applies to: 14-16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 9 - 11, The fenced code blocks in README.md containing shell commands (e.g., "GOOS=linux GOARCH=amd64 go build -o talos-meta-tool ." and the usage example "talos-meta-tool -device /dev/sda -config config.yaml") should include a language identifier for syntax highlighting; update those fenced blocks to use ```bash instead of ``` so both the build command block and the Usage block (and the similar block at lines 14-16) are labeled as bash.main.go (1)
20-33: 💤 Low valueConsider tracking partition size for bounds checking.
The
partAtwrapper doesn't store the partition size, which means reads/writes could theoretically extend beyond the partition boundary into adjacent partitions. Whiletalos.Sizeis small and unlikely to exceed typical META partition sizes, storing and validating against the partition size would provide defense-in-depth.💡 Optional: Add partition size bounds checking
type partAt struct { *os.File offset int64 + size int64 } func (p *partAt) ReadAt(buf []byte, off int64) (int, error) { + if off < 0 || off+int64(len(buf)) > p.size { + return 0, fmt.Errorf("read at offset %d with length %d exceeds partition size %d", off, len(buf), p.size) + } return p.File.ReadAt(buf, p.offset+off) } func (p *partAt) WriteAt(buf []byte, off int64) (int, error) { + if off < 0 || off+int64(len(buf)) > p.size { + return 0, fmt.Errorf("write at offset %d with length %d exceeds partition size %d", off, len(buf), p.size) + } return p.File.WriteAt(buf, p.offset+off) }Then in
findMetaPartition:- return &partAt{File: f, offset: int64(p.FirstLBA) * sectorSize}, nil + size := int64(p.LastLBA-p.FirstLBA+1) * sectorSize + return &partAt{File: f, offset: int64(p.FirstLBA) * sectorSize, size: size}, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.go` around lines 20 - 33, The partAt wrapper needs a partition size field and bounds checking: add a size int64 to the partAt struct and in its ReadAt and WriteAt methods validate that off >= 0 and off+int64(len(buf)) <= p.size (return io.EOF or io.ErrUnexpectedEOF / appropriate error when out-of-bounds), and clamp/adjust behavior for partial reads/writes as needed; ensure wherever partAt is constructed (e.g., in findMetaPartition) you pass the discovered partition size into the new partAt instance so bounds checks use the correct partition length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.go`:
- Around line 104-107: The current main() argument check prints usage and
returns with exit code 0 when devicePath or configPath are empty; change this to
print the usage to stderr (use fmt.Fprintln(os.Stderr, ...)) and call os.Exit
with a non-zero code (e.g., os.Exit(1)) instead of returning. Update the block
that checks *devicePath == "" || *configPath == "" in main to use stderr and
os.Exit to signal failure.
---
Nitpick comments:
In `@main.go`:
- Around line 20-33: The partAt wrapper needs a partition size field and bounds
checking: add a size int64 to the partAt struct and in its ReadAt and WriteAt
methods validate that off >= 0 and off+int64(len(buf)) <= p.size (return io.EOF
or io.ErrUnexpectedEOF / appropriate error when out-of-bounds), and clamp/adjust
behavior for partial reads/writes as needed; ensure wherever partAt is
constructed (e.g., in findMetaPartition) you pass the discovered partition size
into the new partAt instance so bounds checks use the correct partition length.
In `@README.md`:
- Around line 9-11: The fenced code blocks in README.md containing shell
commands (e.g., "GOOS=linux GOARCH=amd64 go build -o talos-meta-tool ." and the
usage example "talos-meta-tool -device /dev/sda -config config.yaml") should
include a language identifier for syntax highlighting; update those fenced
blocks to use ```bash instead of ``` so both the build command block and the
Usage block (and the similar block at lines 14-16) are labeled as bash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf4cefd7-8255-4dda-8a4b-eb8526a5ca5f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
README.mdgo.modmain.gomain_test.go
| if *devicePath == "" || *configPath == "" { | ||
| fmt.Println("Usage: go run main.go -device <META-device> -config <path to config file>") | ||
| fmt.Println("Usage: talos-meta-tool -device <disk-device> -config <file>") | ||
| return | ||
| } |
There was a problem hiding this comment.
Exit with non-zero code on missing arguments.
When required arguments are missing, the tool exits with code 0 (success). CLI tools should exit with a non-zero code to indicate an error condition, allowing callers (scripts, CI) to detect failures.
🔧 Proposed fix
if *devicePath == "" || *configPath == "" {
- fmt.Println("Usage: talos-meta-tool -device <disk-device> -config <file>")
- return
+ fmt.Fprintln(os.Stderr, "Usage: talos-meta-tool -device <disk-device> -config <file>")
+ os.Exit(2)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.go` around lines 104 - 107, The current main() argument check prints
usage and returns with exit code 0 when devicePath or configPath are empty;
change this to print the usage to stderr (use fmt.Fprintln(os.Stderr, ...)) and
call os.Exit with a non-zero code (e.g., os.Exit(1)) instead of returning.
Update the block that checks *devicePath == "" || *configPath == "" in main to
use stderr and os.Exit to signal failure.
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM — solid implementation, and the test coverage in particular is great. Walking through it:
findMetaPartitioncorrectly usesFirstLBA * sectorSizeand the GPT partition-name lookup matches the Talos convention.- The
partAtadapter +interface{ io.ReaderAt; io.WriterAt }decoupling makeswriteConfigcleanly testable without touching disks. - Adding
device.Sync()before exit is the right call — without it block-device writes can sit in the page cache and silently fail later. - The 20 MiB GPT integration test (
TestWriteConfigFullDisk) building a real Talos-shaped disk viago-blockdevice/v2and round-tripping throughfindMetaPartitionis impressive — that's the test I would have asked for.
One small nit (non-blocking): openGPTDevice swallows the first error and only surfaces the second. If both paths fail, debugging the actual block-device problem becomes opaque. Could be worth errors.Join(berr, ferr) or just logging the first. Up to you.
2 participants
Add findMetaPartition which reads the GPT table and locates the partition labelled "META", so -device now accepts a whole disk path (e.g. /dev/sda) instead of requiring the caller to know the partition number. Real block devices use ioctl (BLKGETSIZE64) for correct size via go-blockdevice; regular files fall back to stat, keeping tests root-free. Add //go:build linux since the tool targets Talos Linux.
writeConfig is refactored to accept interface{ io.ReaderAt; io.WriterAt } instead of a device path, which lets it operate on the offset-scoped partition view returned by findMetaPartition.
Add a full-disk test that builds a 20 MiB GPT image with all six Talos partition labels (EFI/BIOS/BOOT/META/STATE/EPHEMERAL) using go-blockdevice/v2, exercises writeConfig via findMetaPartition, and reads back through the partition table to verify the data landed in the right place. Also cover the error paths: zeroed disk (no GPT) and valid GPT without a META partition.
Summary by CodeRabbit
Documentation
New Features
-deviceflag now accepts the full disk path.