armv7

[samcday]

No description provided.

[samcday]

@claude review

[claude]

Claude finished @samcday's task in 2m 7s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze changes
• Post review feedback

---

This PR does two things: (1) eliminates the bindgen + ublk_cmd.h build-time dependency in favor of hand-written Rust bindings, and (2) adds ARMv7 as a cross-compile target. Both are well-executed. Here are my observations:

What's good

Bindgen removal (build.rs, ublk_cmd.h deleted; sys.rs rewritten): Replacing generated bindings with hand-written structs + const assertions is a substantial improvement for cross-compilation. bindgen requires a C toolchain and target headers to be available at build time — exactly what makes ARMv7 cross-compilation painful. The compile-time assertions on sizes, alignments, and field offsets in sys.rs provide equivalent correctness guarantees that are target-aware (Rust evaluates size_of::<T>() in const context for the compile target, not the build host).

rust-lld for cross-linking: Using the LLVM linker (already part of the Rust toolchain) means no gcc-arm-linux-gnueabihf or similar cross-toolchain packages needed. The empty extra_packages: "" for armv7 is correct given this approach.

upload_host: false and -p smoo-gadget-cli: Correctly scoping the armv7 build to just the gadget binary, since smoo-host is meant for the host side and has USB library dependencies unsuitable for an ARMv7 embedded target.

---

Observations

--cfg=io_uring_skip_arch_check appears in two places

It's in .cargo/config.toml:16 (for local dev builds) and in build.yml:72 matrix.rustflags (picked up via RUSTFLAGS: "-D warnings ${{ matrix.rustflags }}" at line 105). This duplication is intentional — RUSTFLAGS env var overrides the config.toml target-specific rustflags, so both are needed. It's not wrong, but a brief comment on either side explaining why ARMv7 needs this flag (io-uring crate has an architecture allowlist; ARMv7 kernels 5.1+ do support io_uring) would help the next person who wonders if one of these can be removed.

Musl kernel headers step uses $(uname -m) (host arch)

.github/workflows/build.yml:91 creates symlinks under /usr/include/$(uname -m)-linux-musl/, which on the ubuntu-24.04 runner will resolve to x86_64, not arm. For the armv7 job this is harmless since smoo-gadget-cli doesn't pull in libusb1-sys (which is what needs these headers). But if a libusb-depending crate ever lands in the gadget binary's transitive deps, the cross-compile job would silently fail to find headers. This is a pre-existing issue in the workflow, not introduced here.

sys.rs defines more structs than the old bindings used

The new sys.rs includes ublk_param_discard, ublk_param_devt, ublk_param_zoned, ublk_param_dma_align, ublk_param_segment — these weren't in the public API before (they were defined in the header but only referenced through ublk_params). The assertions for all of them are present, so this is purely additive and correct. No issue, just noting the scope expansion.

ublksrv_io_cmd.addr is u64 on a 32-bit target

This is intentional: the kernel ABI for ublk deliberately uses __u64 for addresses even in the 32-bit userspace ABI, so the gadget can pass 32-bit pointers up-cast to u64. The compile-time assertions will catch any alignment breakage at cross-compile time.

---

Summary

Clean, well-reasoned PR. The bindgen removal is the right long-term call for a project targeting multiple architectures. The compile-time assertions are the correct mitigation for losing bindgen's static analysis. No blocking issues.