feat(xapi): build the boot VDI's ESP, and document how it actually boots - #14
Merged
Conversation
The boot VDI needs a partitioned disk the firmware can read: a protective MBR, a GPT, and one FAT32 EFI System Partition holding the kernel and initrd. This adds that builder, with no XAPI dependency, so it is testable without a pool. The FAT writer is deliberately partial. The ESP is written once, in a single pass, from a known set of files, and never modified afterwards. That removes deletion, fragmentation, free-space search and in-place update, which is where a general FAT implementation earns its bugs. Files get contiguous cluster runs, and long names are rejected rather than supported: the paths needed are \EFI\BOOT\BOOTX64.EFI and \INITRD.IMG, both already 8.3, so VFAT long-name entries would be the largest part of the writer and would buy nothing. One sector per cluster keeps the FAT32 floor near 33 MiB. The floor is what matters, not the slack, because the image lives in the SR: a larger cluster would force a 256 MiB partition to carry an 8 MiB kernel. Timestamps, volume ID and partition GUIDs are fixed rather than drawn from the clock or a random source. The boot VDI is cached per (kernel, cmdline), so identical inputs have to produce identical bytes or every build looks new to the cache. Verified against implementations that share no code with the writer: sfdisk reads the GPT and the ESP type without complaint, file(1) reads the BPB and calls it FAT32, and a separate FAT parser walked the tree and recovered both files byte-identical, including the initrd's 137-cluster chain. TestFilesRoundTrip carries that check into CI by parsing from the BPB rather than from fatLayout. Both it and TestImageGeometry were confirmed to fail under deliberate mutation of the chain linking and of the backup GPT header. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Three findings that each invalidate something that looks obvious, written down because rediscovering them costs a VM that silently never reaches a kernel. The kernel must be the Kata vmlinuz, not the vmlinux machine/kernel hands every backend: nothing in the UEFI path loads a bare ELF, and XCP-ng is x86_64 in practice so the Kata fallback is the path. The archive ships both under one version string, and no GRUB binary at all. The command line has nowhere to go without a bootloader, so it rides in a Boot0000 load option's optional data, which is what the EFI stub reads. That means writing VM.NVRAM, whose EFI-variables blob is varstored's own VARS serialisation rather than the EDK2 flash varstore layout everyone finds first. Header, record framing and the 48-byte trailer are documented, along with the fact that the variable section does not start at a fixed offset. Also notes that Secure Boot is provisioned on stock UEFI VMs, so the NVRAM must be built rather than copied, and gives the procedure for re-deriving the format if a future release changes it. Established against XCP-ng 8.3.0 / XAPI 26.1 with read-only pool access. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
…twork XAPI's synchronous JSON-RPC sends no response headers until the call has finished, so "time to first header" is the duration of the whole operation rather than of the network round trip. The 60s value therefore capped how long any pool operation was allowed to take. The failure it produced is the expensive kind: a VDI.clone on an LVM SR, a VM.start behind a slow boot, or a large raw import would report a transport error for work that is still running and will succeed on the pool. A caller that retries then has two of them. The dial and TLS handshake timeouts stay, because those really do bound network setup. Per-call context deadlines bound the operation, which is what the adjacent comment about omitting a client-level Timeout was already reaching for. Guarded by a test asserting the field is unset, confirmed to fail when the old value is put back. The assertion is deliberate: a behavioural test can only prove the absence of a timeout shorter than its own runtime, so it would have passed against the 60s value too. Found via the xcpng-cloud-plugin project, which hit the same class of bug against its own XAPI client (gounthar/xcpng-cloud-plugin#73, #79). Signed-off-by: Bruno Verachten <gounthar@gmail.com>
…fset varies The blob reversed was the jenkins-golden-debian template (37564 bytes), not the VM of the same name (37444). VM.get_all_records returns templates alongside VMs, so matching on name_label alone reads whichever comes first. The size difference is worth more than the correction: two records that look interchangeable are not the same length, so a hardcoded 292 works against one and mis-parses the other. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
FAT requires the ".." entry of a directory whose parent is the root to carry first cluster 0 rather than the root's actual cluster number. The parent map seeded the root as cluster 2, so \EFI\.. pointed at 2. The struct field's own comment already stated the rule — "0 means root, per FAT's '.' / '..' rule" — so the code contradicted its documented invariant. An image with this defect reads back correctly through a naive parser and is called corrupt by fsck.fat and by strict firmware, which is the worst combination: it would have presented as a VM that does not boot, with nothing obviously wrong in the image. Both existing oracles were blind to it because both skipped the dot entries. The read-back helper now records them instead, and the new test was confirmed to fail against the old value. That blindness, rather than the bug, is the thing worth not repeating: entries that exist purely for the firmware to read cannot be validated by a reader that ignores them. Also tightens short-name validation to reject characters FAT reserves, spaces and trailing dots, since pad83 copies bytes straight into the entry and would otherwise produce a structurally valid name the firmware cannot open. Adds coverage for zero-length files and for malformed directory paths, and drops an unreachable cluster-count check. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a deterministic GPT/FAT32 ESP image builder, documents the XCP-ng UEFI boot path, removes the JSON-RPC response-header timeout, and updates macOS test execution. ChangesDeterministic ESP image support
JSON-RPC response timing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant buildESPImage
participant FAT32GPTSerializer
participant ESPImageTests
Caller->>buildESPImage: provide kernel and initrd files
buildESPImage->>FAT32GPTSerializer: calculate layout and serialize image
FAT32GPTSerializer-->>buildESPImage: return deterministic ESP image
ESPImageTests->>buildESPImage: build and independently parse image
ESPImageTests-->>Caller: verify GPT, FAT32, paths, contents, and reproducibility
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
machine/xapi/espimage.go (1)
232-263: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject duplicate short names within one directory.
Nothing rejects two
espFilevalues with the sameDirandName, or a file whose name equals a sibling directory name. The builder then writes two entries with the same 11-byte short name into one cluster. FAT does not allow that, and the fault appears only when firmware opens the wrong entry.Every entry name for a directory is already collected in this loop, so the check is local.
♻️ Proposed check
if len(d.entries)*32 > clusterSize { return nil, fmt.Errorf("xapi: directory %q has too many entries for one cluster", d.path) } + seen := make(map[string]bool, len(d.entries)) + for _, e := range d.entries { + if e.name == "." || e.name == ".." { + continue + } + key := string(pad83(e.name)) + if seen[key] { + return nil, fmt.Errorf("xapi: directory %q has duplicate entry %q", d.path, e.name) + } + seen[key] = true + }Please also add a case to
machine/xapi/espimage_test.gothat passes two files with the sameDirandNameand expects an error.🤖 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 `@machine/xapi/espimage.go` around lines 232 - 263, Track each short entry name while building a directory’s entries in the loop over l.dirs, and return an error when a file or sibling directory reuses a name already collected for that directory; include dot entries in the same duplicate check. Add a test in the espimage test suite that supplies two files with identical Dir and Name and asserts image construction returns an error.
🤖 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 `@machine/xapi/BOOT.md`:
- Around line 46-48: Update the `espimage.go` description to state the initrd’s
explicit volume-root path as `\INITRD.IMG`, matching the `sampleImage` layout
and boot option; replace the ambiguous “beside it” wording while retaining the
kernel path.
In `@machine/xapi/espimage.go`:
- Around line 600-606: In the image-size calculation near the sector-header
writes, remove the unreachable uint32 overflow clamp and assign size directly
from totalSectors-1 before writing it with binary.LittleEndian.PutUint32.
- Around line 92-116: Add an entry-point size guard in buildESPImage that
rejects any input whose aggregate file data exceeds the maximum representable
32-bit layout range before validation, layout, or allocation. Base the check on
the total Data lengths and return a descriptive error; preserve existing
behavior for inputs within the limit so totalSectors allocation and FAT32 size
fields cannot overflow.
---
Nitpick comments:
In `@machine/xapi/espimage.go`:
- Around line 232-263: Track each short entry name while building a directory’s
entries in the loop over l.dirs, and return an error when a file or sibling
directory reuses a name already collected for that directory; include dot
entries in the same duplicate check. Add a test in the espimage test suite that
supplies two files with identical Dir and Name and asserts image construction
returns an error.
🪄 Autofix
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 Plus
Run ID: 42cb25ed-6514-4b40-924a-905c81b0bac0
📒 Files selected for processing (5)
machine/xapi/BOOT.mdmachine/xapi/client_jsonrpc.gomachine/xapi/client_jsonrpc_test.gomachine/xapi/espimage.gomachine/xapi/espimage_test.go
Every offset in espimage.go is a uint32 of sectors, and totalSectors*sectorSize is evaluated in uint32. Past 4 GiB it wraps, make() is handed a short buffer, and an input that is merely too large becomes an out-of-range write rather than an error. Nothing else bounded Data, so the only thing standing between the builder and that was the size of a kernel and an initrd in practice. Bounded at the entry point, in uint64, before any of that arithmetic runs: it is the only place holding the caller's input, and it makes every later offset in range by construction rather than by inspection. A single file of 4 GiB or more is refused separately, because a directory entry records length in a uint32 and would silently name a shorter file than the one written. The two checks are separate functions so they can be tested. Proving the bound by allocating four gigabytes is not a test worth having, and reaching for unsafe to fake a slice of that length would be worse; the real path calls exactly these functions, so it is still the tested path. Also removes a clamp in the protective MBR that could not fire — totalSectors is uint32, so the count after LBA 0 always fits the 32-bit field, and the 0xFFFFFFFF cap exists for 64-bit-addressed disks. And states the initrd's path in BOOT.md rather than calling it "beside" the kernel: it lands at \INITRD.IMG in the volume root, not in \EFI\BOOT, and the boot option's initrd= has to name the right one. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
…t/xapi-boot-vdi-esp
The macos "machine module — vet / test" step hangs on this branch while passing in about ten seconds on main and on this branch before it was merged with main. Two attempts both hung; neither parent is slow. Nothing usable is available to diagnose it: GitHub retains no logs for a cancelled job, the step carries no timeout so it would otherwise run to the job limit, and the hang is macos-only so it does not reproduce on linux or locally. -timeout 3m makes Go abandon the run and panic with a goroutine dump naming the stuck test, which is the thing worth knowing. -v shows how far each package got, and -p 1 drops package parallelism so the stall is attributed to one package instead of several interleaved. Diagnostic only. This must come off before the PR merges. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
This reverts commit 387c0d8. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Changes exactly one thing from the configuration that hangs. The earlier attempt added -v, -p 1 and -timeout at once and passed in 13s, which proved nothing: -p 1 removed the hang, so the timeout never fired and no dump was produced. Package parallelism therefore stays at the default here, because that is the condition that reproduces it. -timeout 3m converts the hang into a panic carrying a goroutine dump that names the blocked test and shows what the other packages were doing. Without it the job runs until the runner is reclaimed — the last one took 52 minutes and GitHub retained no log. df brackets the run because disk exhaustion is the leading suspect: upstream's new compactext4 tests write 16, 32 and 64 GiB sparse images, and a runner that does not produce real holes fills up rather than deadlocks. Context for whoever reads this next: neither side hangs alone. Upstream runs this step in 5s, this fork's main in 10s, and this branch in 13s before it was merged with main. Only the combination hangs, which is why upstream has never seen it. Diagnostic only. This must come off before the PR merges. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Around line 84-110: Remove the temporary diagnostic comments and disk checks
from the machine module workflow step, and restore its original go test command
without the hard 3-minute timeout or verbose flag. Keep the existing go vet
invocation and the step’s normal CI behavior unchanged.
🪄 Autofix
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 Plus
Run ID: 8945c21c-3277-4f6e-8750-1c904a72ec3e
📒 Files selected for processing (1)
.github/workflows/ci.yml
The previous diagnostic bounded go test at 3m and the timeout never fired in 32 minutes, so the stall is not in the tests. go vet runs immediately before them and has never been instrumented — it type-checks every package in the module including machine/vz, which is cgo, so it is a single unbounded step that would produce exactly this signature: no output, no failure, and a job killed around 50 minutes with no retained log. timeout(1) bounds it, since Go offers no vet equivalent of -timeout. -x prints each action as it runs, so the log shows the last package vet reached. The exit status is taken from PIPESTATUS because the output is piped through tail, and 124 distinguishes a timeout from a real vet failure. Also records CPU count, memory and disk before and after each phase, to settle whether resource exhaustion is involved rather than a deadlock. This corrects an error in the previous two diagnostics: the first changed three variables at once and -p 1 masked the hang so nothing was learned, and the second instrumented the half that turns out not to be at fault. Diagnostic only. This must come off before the PR merges. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
…erge Third instrument, after two that produced nothing. The previous one used timeout(1), which macos does not ship: it exited 127 in four milliseconds, so vet never ran. timeout-minutes is a platform feature needing no binary, and it fixes the problem that has cost the most here — a step GitHub cuts short still publishes its log, whereas the job-level kill we keep hitting retains nothing, which is why five hung runs yielded no diagnostic output at all. Splitting vet and test into separate steps means the step list alone says which half stalls, without needing the log to survive. -x prints each action, so vet's last log line names the package it reached. Diagnostic only. This must come off before the PR merges. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Without -p 1 this step does not fail, it loses the runner. Every attempt died at a consistent 52-53 minutes with the step still marked in progress, no conclusion and no uploaded log; neither step-level timeout-minutes nor Go's own -timeout ever fired. A dead agent explains all three at once, where a deadlock explains none of them. A macos-14 runner has 3 cores, about 7 GB of RAM and had 38 GiB free, while upstream's compactext4 tests build 16, 32 and 64 GiB images. Run one package at a time those are sparse and take 3s. Run concurrently with the rest of the module and the runner stops coming back. Bisected by splitting vet from test into separate bounded steps: vet finishes in 4s, so the entire cost is the concurrent test phase. That also corrects an earlier guess that vet was at fault, which came from bounding go test and reading its silence as innocence when the process was never reaching the timeout. Neither side triggers this alone, which is why upstream has not seen it: upstream runs the step in 5s, this fork's main in 10s, and this branch in 13s before it was merged with main. Only the combination is heavy enough. The bound costs 3 seconds. -timeout 10m is hygiene so a future stall inside one test fails loudly rather than taking the runner with it. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / why
Step 4 of the xapi backend's build order: the boot VDI. This is the ESP builder plus the investigation that had to happen before it, and one unrelated transport fix found along the way.
The ESP builder (
espimage.go) produces the boot VDI's raw bytes — protective MBR, GPT, one FAT32 EFI System Partition. It has no XAPI dependency, so it tests without a pool.The FAT writer is deliberately partial, and the comment at the top of the file argues why: the ESP is written once, in one pass, from a known set of files, and never modified. That removes deletion, fragmentation, free-space search and in-place update — where a general FAT implementation earns its bugs. Files get contiguous cluster runs. Long names are rejected rather than supported, because the only paths needed are
\EFI\BOOT\BOOTX64.EFIand\INITRD.IMG, both already 8.3, so VFAT long-name entries would be the largest part of the writer and buy nothing.One sector per cluster keeps the FAT32 floor near 33 MiB. The floor is what matters rather than the slack, because the image lives in the SR — a larger cluster would force a 256 MiB partition to carry an 8 MiB kernel. Timestamps, volume ID and partition GUIDs are fixed rather than drawn from the clock, because the boot VDI is cached per (kernel, cmdline) and identical inputs must produce identical bytes.
BOOT.mdrecords three things established by investigation, each of which invalidates something that looks obvious. Two of them contradict how the boot path was originally specified:vmlinuz, not thevmlinuxthatmachine/kernelhands every backend. Nothing in the UEFI path boots a bare ELF, and XCP-ng is x86_64 in practice so the Kata fallback is the path. The archive ships both under one version string; only the 8.3 MB bzImage is UEFI-loadable, and it carriesCONFIG_EFI_STUB. This wants a per-backendBinaryPath, not a change tokernel.DefaultBinaryPath— firecracker and vz still need the ELF.grubx64.efior shim at all, and the EFI stub means none is needed, so no bootloader binary enters the trust path.Boot0000load option written intoVM.NVRAM. That blob is varstored's ownVARSserialisation, not the EDK2 flash varstore layout — which is the trap, because searching for "UEFI variable store format" finds the EDK2 one and it will not parse. Header, record framing and the 48-byte trailer are documented, along with the fact that the variable section start moves between records and must be computed rather than hardcoded.BOOT.mdalso notes that Secure Boot is provisioned on stock UEFI VMs, so the NVRAM must be built rather than copied, and gives the procedure for re-deriving the format if a future release changes it.The transport fix is independent of the above.
ResponseHeaderTimeoutwas capping how long a pool operation could take, not how long the network could take: XAPI's synchronous JSON-RPC sends no response headers until the call has finished. When it fired it reported a transport error for a clone, import or start still running and about to succeed — and a caller that retries has started a second one. Dial and TLS handshake timeouts stay, because those really do bound network setup.Platform(s) built and tested on
machine/xapiis platform-independent and is gated by neither provider; it was exercised on Linux. The vz paths are untouched by this change.Checklist
gofmt -l .is cleango build ./... && go vet ./... && go test ./...pass from the repo rootgo build ./... && go vet ./... && go test ./...pass frommachine/The root-module box is deliberately unticked.
internal/sandboxfails there, and it fails identically onmainwith this branch checked out and stashed — the failure is environmental (CLAWK_TEST_MISSINGunset, and abasename: missing operandinside the test's own script), andmachine/is a separate module that the root build does not compile. Verified rather than assumed, but flagging it rather than quietly ticking a box that is not true.Pre-PR Review
Five independent automated reviewers were run against the diff before opening. Four produced usable output; one had exhausted its free usage allowance and wrote nothing, so this was a four-of-five gate. One reviewer runs entirely in-house and reported no findings, with a coverage note that one non-code file was skipped and the largest source file was split across chunks.
Addressed
MAJOR —
..of a root-level directory pointed at cluster 2 instead of 0. FAT requires the..entry of a directory whose parent is the root to carry first cluster 0 rather than the root's real cluster number. The struct field's own comment already stated the rule, so the code contradicted its documented invariant. An image with this defect reads back fine through a naive parser and is called corrupt byfsck.fatand by strict firmware — it would have presented as a VM that does not boot with nothing obviously wrong in the image.Worth recording why it survived: both verification oracles skipped the dot entries, so neither could see it. The read-back helper now records them, and the new test was confirmed to fail against the old value. The blind spot mattered more than the bug.
Short-name validation was too permissive. It checked length and case but not the character set, so a space or one of FAT's reserved characters would produce a structurally valid entry naming a file the firmware cannot open. Now rejects those, plus trailing dots and empty stems.
Unreachable cluster-count check removed; the value is clamped immediately above, so it could not fire.
Test coverage gaps named by one reviewer: zero-length files (the
cluster = 0path) and malformed directory paths (/EFI,EFI/,EFI//BOOT) now have tests.Acknowledged
root=/dev/xvdbrather than by PARTUUID. Deriving the GUIDs from a content hash would keep reproducibility and uniqueness, and is the better long-term answer; left out here to keep this change scoped.TestSlowResponseStillSucceedsasserts little beyond constructor success. It does drive a login RPC through a deliberately delayed handler, so it exercises the path, but it is a weak behavioural test and the code comment says so. The config-level assertion is the real guard, and it was confirmed to fail when the old timeout is restored.BOOT.mdseparating guaranteed protocol from empirically observed layout. Both are reasonable; neither is in this change.Test plan
cd machine && go build ./... && go vet ./... && go test -race ./xapi/— passesCLAWK_ESP_DUMP=/tmp/esp.img go test ./xapi/ -run TestDumpImagethensfdisk -l /tmp/esp.img— reports a GPT label with one EFI System partition, no backup-GPT or CRC complaintsdd if=/tmp/esp.img of=/tmp/part.img bs=512 skip=2048 && file /tmp/part.img— identifies FAT (32 bit), labelEFI SYSTEM\EFI\BOOT\BOOTX64.EFIand\INITRD.IMGare byte-identical to their inputs, and thatfsck.fat -nis clean — the last part is what the..fix is really aboutSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests