Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new --dev disk trailer code in hal/library_fs.c uses fseek()/ftell() with long, which can mis-handle offsets/sizes on 32-bit hosts or large (>2GiB) devices/images.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an optional “boot confirmation + automatic rollback” mechanism for disk-boot targets, using a small on-media trailer state machine shared between the bootloader and the userspace lib-fs tool.
Changes:
- Introduces a shared disk trailer format/state definitions (
include/disk_trailer.h) and wires a slot state machine into the disk boot path (src/update_disk.c) whenDISK_BOOT_CONFIRM=1. - Extends
lib-fsto operate on arbitrary backing stores via--dev <path>, including staging/confirming slot state in the device tail (hal/library_fs.c,hal/filesystem.c,include/hal.h). - Adds a dedicated host unit test and CI coverage for the new disk confirmation path, plus documentation (
tools/unit-tests/*,.github/workflows/test-configs.yml,docs/compile.md).
File summaries
| File | Description |
|---|---|
| tools/unit-tests/unit-update-disk-confirm.c | New unit test suite covering disk slot state transitions and edge cases (overlap/min-size/unconfirmed). |
| tools/unit-tests/Makefile | Adds the new unit test target and build flags. |
| src/update_disk.c | Implements optional disk-slot state read/reap and arming (UPDATING→TESTING) before handoff. |
| options.mk | Adds DISK_BOOT_CONFIRM build option gating and validation. |
| include/hal.h | Declares hal_filesystem_set_target() for the filesystem HAL. |
| include/disk_trailer.h | Defines the on-media trailer layout and pinned state values shared by loader/tool. |
| hal/library_fs.c | Adds --dev support and disk-slot status/stage/success commands using the trailer tail format. |
| hal/filesystem.c | Allows repointing the filesystem HAL backing store at runtime. |
| docs/compile.md | Documents the disk confirmation lifecycle, staging/confirming workflow, and constraints. |
| .github/workflows/test-configs.yml | Adds CI build job that compiles the disk confirmation code path. |
Review details
Suppressed comments (2)
hal/library_fs.c:176
disk_trailer_open()usesfseek()/ftell()and alongsize, which can truncate offsets/sizes on 32-bit hosts and mis-locate the trailer on devices/images >2GiB. Usefseeko()/ftello()withoff_tinstead.
if (fseek(*fp, 0, SEEK_END) != 0 || (end = ftell(*fp)) < 0) {
wolfBoot_printf("Cannot determine the size of %s\n", disk_dev);
fclose(*fp);
return -1;
}
hal/library_fs.c:186
- Seeking to the computed trailer offset should also use
fseeko()(withoff_t) to avoid narrowing the offset via(long)*offon 32-bit hosts.
if (fseek(*fp, (long)*off, SEEK_SET) != 0) {
wolfBoot_printf("Cannot seek to the tail of %s\n", disk_dev);
fclose(*fp);
return -1;
}
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
26
to
+30
| #include "image.h" | ||
| #include "printf.h" | ||
| #include "wolfboot/wolfboot.h" | ||
| #include "hal.h" | ||
| #include "disk_trailer.h" |
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
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.
Summary
Adds optional boot confirmation and automatic rollback (
DISK_BOOT_CONFIRM=1) for disk-based boot targets using a partition trailer state machine.Key Changes
include/disk_trailer.h): Defines on-media format, magic bytes, and states (IMG_STATE_*) shared between loader and userspace tools.src/update_disk.c): Reads slot state, drops unconfirmed slots from selection, and marks staged slots astestingprior to handoff.hal/library_fs.c,hal/filesystem.c): Enableslib-fsto stage and confirm any slot by device path.options.mk): Adds theDISK_BOOT_CONFIRMoption for disk-boot targets.tools/unit-tests/unit-update-disk-confirm.c), CI job forcm4_sdcard, and documentation (docs/compile.md).Design Constraints
DISK_FSexcluded).Verification
lib-fsfrom this branch:lib-fs --dev <slot> stagewroteUPDATING; wolfBoot selected that slot, promoted it toTESTINGand booted it.Slot A was not confirmed; skipping it, dropped its version from the election and booted the other slot.TESTINGafterwards.lib-fs --dev <slot> success, the slot was selected, booted, and left untouched - a steady-state boot performs no writes.NEWrather thanSUCCESSon real media, which is the case the trailer magic exists to catch.update-triggeris refused with--dev, leaving the 64 MB slot bit-identical, since it would otherwise write the UPDATE trailer at a compile-time offset into the middle of the slot.