Add ubi_volume_write action for UBI volumes - #293
Conversation
|
@Hermanverschooten Just wanted to let you know that this is on my list for my next pass on |
|
First of all, I'm so sorry about this taking a terribly long time to get to. There are a few easy things to fix - mostly trimming some of the Claude verbosity down, deleting the CHANGELOG.md entry completely and moving the actual UBI code to a Linux-only file (similar to mmc_linux.c). The hard part is that I really need a regression test for the success case. It could be something that hooks into a shim or the verify_syscalls.c. I don't think it needs to be airtight, but something that I can run if I ever need to move code around in the future. Could you see if you and your Claude can address the hard part? I'm not worried about the easy parts since those would take me no time at all to manually update if that turns out to be easiest. |
|
Also, if you're running on macOS, I think the CI build failures on mac should be easy to fix. |
Review feedback from fwup-home#293: - ubi_volume_write's open/UBI_IOCVOLUP/write/close now live in ubi_linux.c behind a small API in ubi.h, similar to mmc_linux.c. functions.c keeps the portable sequencing/zero-fill logic. This also fixes the macOS and Windows builds, which failed because mtd/ubi-user.h pulls in linux/types.h. - Non-Linux builds get stubs that error at run time. - Removed the CHANGELOG entry and trimmed the README row.
|
Thanks for the review! Claude and I addressed everything:
Full test suite passes locally on Linux. CI needs a workflow approval to run on the new commits. |
Review feedback from fwup-home#293: - ubi_volume_write's open/UBI_IOCVOLUP/write/close now live in ubi_linux.c behind a small API in ubi.h, similar to mmc_linux.c. functions.c keeps the portable sequencing/zero-fill logic. This also fixes the macOS and Windows builds, which failed because mtd/ubi-user.h pulls in linux/types.h. - Non-Linux builds get stubs that error at run time. - Removed the CHANGELOG entry and trimmed the README row.
eebcf63 to
ca69ced
Compare
UBI volumes (/dev/ubi0_N) reject pwrite() with EPERM. Updates have to
be initiated via the UBI_IOCVOLUP ioctl, which puts the volume in
atomic-update mode for a declared number of bytes; subsequent
sequential write()s fill the volume, and close() commits the update
atomically. raw_write/path_write/pipe_write all use pwrite, so none
of them work for OTA updates targeting individual UBI volumes.
ubi_volume_write(device_path) does the right thing:
on-resource fit_a.itb {
ubi_volume_write("/dev/ubi0_3")
}
* Issues UBI_IOCVOLUP with the resource's full size (sparse holes
included — they are materialized as zero writes since UBI can't
seek).
* Tracks a write cursor and rejects out-of-order callbacks; fills
gaps before sparse data with zeros.
* Requires --unsafe (consistent with path_write / pipe_write since
the destination is a user-supplied path, not the -d device).
* Falls back to a build-time guard error on non-Linux hosts where
<mtd/ubi-user.h> isn't available.
Includes a smoke test (225_ubi_volume_write.test) covering arg
validation, --unsafe enforcement, and clean failure when the path
isn't actually a UBI volume. Real ioctl behaviour belongs to the
kernel and isn't exercised here.
Per Frank's review on fwup-home#292: ship ubi-user.h with fwup so the ubi_volume_write action is always compiled in, and let it fail hard at runtime if anything goes wrong rather than silently no-op'ing on hosts without mtd-utils headers. Header copy comes from mtd-utils' own UAPI version, license "GPL-2.0+ WITH Linux-syscall-note" (the same exemption that lets non-GPL programs include kernel UAPI). Lives at src/3rdparty/mtd-utils/include/mtd/ubi-user.h, included via a new -I flag in src/Makefile.am. Removed: - mtd/ubi-user.h from configure.ac AC_CHECK_HEADERS - #ifdef HAVE_MTD_UBI_USER_H / non-Linux stub from src/functions.c Build clean, existing test (225_ubi_volume_write.test) still passes.
Review feedback from fwup-home#293: - ubi_volume_write's open/UBI_IOCVOLUP/write/close now live in ubi_linux.c behind a small API in ubi.h, similar to mmc_linux.c. functions.c keeps the portable sequencing/zero-fill logic. This also fixes the macOS and Windows builds, which failed because mtd/ubi-user.h pulls in linux/types.h. - Non-Linux builds get stubs that error at run time. - Removed the CHANGELOG entry and trimmed the README row.
An LD_PRELOAD shim emulates the UBI volume update API on a regular file: UBI_IOCVOLUP starts update mode and records the declared size, write() is only allowed while updating, and pwrite() fails with EPERM like a real UBI volume. The test verifies that both a regular and a sparse resource round-trip byte-for-byte (holes written as zeros) and that the declared size matches the bytes written. Linux-only; the test skips elsewhere and when the shim can't be preloaded.
ca69ced to
1019276
Compare
|
Thanks for all of the updates. This looks good and like something I can maintain long-term. |
Closes #292.
Adds a new
ubi_volume_write(device_path)action sofwupcan writeto UBI volume character devices (
/dev/ubi0_3, etc.). UBI rejectspwrite()withEPERM; updates have to be initiated via theUBI_IOCVOLUPioctl, which puts the volume in atomic-update mode fora declared number of bytes. Subsequent sequential
write()s fill thevolume, and
close()commits the update atomically. Previouslyraw_write/path_write/pipe_writeall usepwrite(), so noneof them worked for OTA updates targeting individual UBI volumes.
How it works
UBI_IOCVOLUPwith the resource's full size (sparse holesincluded — they're materialized as zero writes since UBI can't seek).
gaps before sparse data with zeros.
--unsafe(consistent withpath_write/pipe_writesince the destination is a user-supplied path, not the
-ddevice).mtd/ubi-user.h(GPL-2.0+ WITH Linux-syscall-note, the sameexemption that lets non-GPL programs include kernel UAPI) under
src/3rdparty/mtd-utils/. No autoconf header check, no Linux-onlyguards — fails hard at run time if anything goes wrong.
Tests
Smoke test (
tests/225_ubi_volume_write.test) covers:ubi_volume_write()with no args fails at config-parse time.--unsafe, the action refuses to apply.--unsafe, applying to/dev/nullcleanly errors with theioctl-failure message rather than silently succeeding.
Real ioctl behaviour is the kernel's responsibility and isn't
exercised in CI (would need
nandsim+ubiin a VM).End-to-end hardware verification
Validated on real OpenWRT One hardware (MediaTek MT7981B, SPI NAND)
with this binary cross-compiled for aarch64:
.fwcontaining a 2828-bytepayload.binresource.fwup --unsafe -a -d /dev/null -i test.fw -t complete(thefwup.conf has
ubi_volume_write("/dev/ubi0_3")).Success!with558 bytes in / 2.83 KB out.dd if=/dev/ubi0_3 bs=1 count=2828 | sha256sum—hash matched the source
payload.binexactly.OKafter the atomic update.I'm using this on real OpenWRT One hardware with
nerves_system_openwrt_one—A/B kernel slots and U-Boot env all live in UBI volumes. With this
action plus a future tweak to
uboot_setenvto detect UBI devicetargets, my custom OTA-script glue can be retired.