Repository navigation
tree: Replace ExternalNoVerity with ExternalPath, keep ostree's redirect - #7
cgwalters-bot wants to merge 2 commits into
Conversation
7dd8472 to
d1b58c8
Compare
cgwalters-bot
left a comment
There was a problem hiding this comment.
Review guide for head d1b58c851f: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Only the last commit (d1b58c8) is this PR; the other twelve are upstream commits that the forge fork's main hasn't picked up yet, so skip them. The commit replaces RegularFile::ExternalNoVerity with ExternalPath { redirect, verity: Option, size }, so ostree's xx/<checksum>.file payload survives the capi, the EROFS reader and dumpfiles. Byte-compat with C is shown by a cmp between C- and Rust-written images in just test-capi (upstream CI runs this; the fork runs no CI), plus a pinned digest in a unit test. Where the risk is: the reader and the dumpfile parser still don't match C for a digest with no payload, and composefs-info (objects/missing-objects/ls) now skips ExternalPath files entirely, which makes missing-objects give false negatives on ostree images. On the API side, the empty-redirect sentinel and the normalization copied between the reader and the dumpfile code could be tighter. The V2 null chunk index change is safe, because the capi only writes V0/V1.
Hotspots
- look-closely · api —
crates/composefs/src/tree.rs:25-39(d1b58c8): Public API shape. The empty redirect is a sentinel meaning 'no redirect' where Option<Box<OsStr>> would say so in the type. ExternalPath{"", None} duplicates Sparse, and ExternalPath{objpath(id), Some(id)} duplicates External. Consider a shared normalizing constructor. - note · api —
crates/composefs/src/tree.rs:54-76(d1b58c8): repo_object_id prefers verity over the redirect, which is right for a composefs repo keyed by fsverity digest. Without verity, it trusts a redirect only if the redirect parses as an object path. - look-closely · logic —
crates/composefs-capi/src/convert.rs:137-167(d1b58c8): Mirrors lcfs-writer-erofs.c add_overlay_xattrs: size 0 and content take priority, and payload and digest are independent. This matches my reading of C at ec2573a. - risky · logic —
crates/composefs/src/erofs/reader.rs:1878-1896(d1b58c8): A digest with no redirect still becomes External, so rewriting a capi image adds a redirect that C never writes. The fix is to take External only when the redirect equals the digest's object path (is_some_and) and otherwise produce ExternalPath with an empty redirect. - risky · logic —
crates/composefs/src/dumpfile.rs:504-520(d1b58c8): The writer emits '-' plus a digest for ExternalPath{"", Some}, but dumpfile_parse.rs:451 still rejects that ('Inline file cannot have fsverity digest'), so our own output doesn't round-trip, while C accepts it. This duplicates the normalization rule in reader.rs. - risky · logic —
crates/composefs-ctl/src/lib.rs:1673-1680(d1b58c8): Not in this diff: composefs_info.rs:180 and :252 still match only External. ostree images (digest plus .file payload) used to read as External and now read as ExternalPath, so composefs-info ls/objects/missing-objects silently skip them, and missing-objects reports nothing missing. C prints the payload. - note · logic —
crates/composefs/src/erofs/writer.rs:1286-1294(d1b58c8): The V2 null chunk index for ExternalPath fixes a missing chunk index in chunk-format-31 inodes. It's safe: the capi only maps versions 0 and 1 (image.rs:274), and no pinned V2 digest moved. - look-closely · test-gap —
crates/composefs-capi/tests/ostree-image.c:69-100(d1b58c8): This covers ostree's forms: .file with and without a digest, an empty file, a symlink, and version 0/max 1 like ostree. Add a digest-only node and one whose payload equals the digest's object path, so the cmp against C covers the edge cases too. - note · test-gap —
crates/composefs-capi/tests/Containerfile.test-capi:8-23(d1b58c8): The C image is written before the .so swap and the Rust one after it, so the cmp really is C against Rust. Once upstream CI runs this, it also shows that the pinned digest is C's.
Safe to skim
crates/composefs-boot/src/android_boot.rs: upstream commit, forge main is stalecrates/composefs-boot/src/bootloader.rs: upstream commit, forge main is stalecrates/composefs-capi/src/mount.rs: upstream commits, forge main is stalecrates/composefs-integration-tests/src/tests/upgrade.rs: upstream commit, forge main is stalecrates/composefs-integration-tests/src/tests/capi.rs: upstream commit, forge main is stalecrates/composefs/src/fs.rs: mechanical switch to repo_object_idcrates/composefs-oci/src/delta.rs: mechanical switch to repo_object_idcrates/composefs-boot/src/selabel.rs: mechanical switch to repo_object_id
d1b58c8 to
596bfab
Compare
|
Addressed the review in
Retested on a 16-core RHEL 10 devspace: |
cgwalters-bot
left a comment
There was a problem hiding this comment.
Review guide for head 596bfab1ff: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Fix round on the ExternalPath change, now one commit. The findings from the last review are addressed. The reader, the capi and the dumpfile parser all go through RegularFile::external(redirect, verity, size), so a digest without a payload stays ExternalPath { redirect: None } everywhere and our own dumpfile output round-trips. composefs-info ls/objects/missing-objects and the cfsctl backing-path dump use RegularFile::backing_path(), so ostree images list their xx/<checksum>.file payloads like C. redirect is Option<Box<OsStr>> instead of an empty sentinel. ostree-image.c adds a digest-only file and one whose payload is the digest's object path, so the cmp against C covers both. The main thing to decide is the public API: dumpfile_parse::Item::Regular's path became an Option. I think that's the right shape (see the hotspot). The new error path in the capi tree conversion frees the partial tree with lcfs_node_unref. By my reading it is memory-safe, but the test fails on a top-level file, so the free only runs on an empty tree. One small new divergence from C: missing-objects joins a payload that starts with / onto basedir, so the path escapes it.
Hotspots
- look-closely · api —
crates/composefs/src/dumpfile_parse.rs:94-102(596bfab): Public API break: Item::Regular.path is now Option. I prefer this to a new variant. A new variant would slip silently past let-else and _ arms in consumers (bootc's GC has one), while this fails to compile. The release is breaking anyway because ExternalNoVerity is removed, and bootc needs a one-line change. - note · logic —
crates/composefs/src/dumpfile_parse.rs:437-458(596bfab): The parser now accepts payload '-' with a digest, as C does. Inline content with a digest is still rejected, so the old error survives only where it applies. - look-closely · logic —
crates/composefs/src/tree.rs:57-80(596bfab): This is the one normalization rule shared by the capi, the reader and the dumpfile parser. Check the comparison of the redirect with the object path, and that an empty redirect counts as none, like C's strlen(payload) > 0. - note · api —
crates/composefs/src/tree.rs:23-45(596bfab): The variant's fields are public, so a caller can build a non-normalized ExternalPath without going through external(). That's harmless: the writer emits the same bytes for it as for the equivalent External or Sparse. - note · logic —
crates/composefs/src/erofs/reader.rs:1875-1886(596bfab): The earlier risky finding is fixed: a metacopy digest with no redirect now reads back as ExternalPath { redirect: None }, so rewriting a capi image no longer adds a redirect. - look-closely · error-handling —
crates/composefs-capi/src/convert.rs:199-212(596bfab): The partial tree is freed on error. Subdirectories are now attached before the recursion, and a failing leaf is still an owned Box that Drop frees. So the unref reaches every node exactly once, hardlink targets included, and by my reading it's sound. - note · error-handling —
crates/composefs-capi/src/convert.rs:304-324(596bfab): A redirect with an interior NUL now fails instead of being dropped. It can only come from a tree built in Rust or from an EROFS xattr, never through the C API. - note · error-handling —
crates/composefs-capi/src/image.rs:74-86(596bfab): The conversion error is discarded and only EINVAL is set. Logging it at debug level would keep the cause. - look-closely · test-gap —
crates/composefs-capi/src/tests.rs:486-513(596bfab): The only file is at the root, so the error comes before anything is attached, and lcfs_node_unref frees an empty root. Add a subdirectory and a hardlinked pair that sort before the bad file, so the new free path actually runs, ideally under ASan or Miri. - note · logic —
crates/composefs-ctl/src/composefs_info.rs:283-292(596bfab): When a payload starts with '/', basedir.join(obj) replaces basedir. That can happen now that arbitrary payloads come through. C strips leading slashes (abs_to_rel_path), so trim them before the join. - note · test-gap —
crates/composefs-capi/tests/ostree-image.c:88-100(596bfab): The new digest-only and object-path nodes are in the image that Containerfile.test-capi cmps between C and Rust. DIGEST_OBJ matches the i*7+3 digest, and the unit test checks that it reads back as External.
Safe to skim
crates/composefs-boot/src/selabel.rs: mechanical switch to repo_object_idcrates/composefs-oci/src/delta.rs: mechanical switch to repo_object_idcrates/composefs/src/fs.rs: mechanical switch to repo_object_idcrates/composefs-ostree/src/commit.rs: mechanical switch to repo_object_idcrates/composefs-capi/build.rs: adds the test C file to the buildcrates/composefs-capi/tests/test-capi-container.sh: copies ostree-image.c into the build contextcrates/composefs/src/erofs/writer.rs:706-800: mechanical variant rename in match arms
596bfab to
ceafade
Compare
|
Verification follow-ups squashed in |
Avoid indexing arrays when parsing the split cmdline, instead use CStr::from_bytes_until_nul() and iterators. Signed-off-by: Alexander Larsson <alexl@redhat.com>
ceafade to
88ea0fe
Compare
ostree gives each file a libcomposefs payload of `xx/<checksum>.file` and sets the fsverity digest separately. The capi turned the payload into an ObjectID (dropping `.file`) or ignored it when a digest was set, so the redirect pointed at a file that doesn't exist and an ostree deployment on the Rust libcomposefs couldn't boot. ExternalPath carries an optional redirect as given plus an optional verity digest, which is how libcomposefs models it. RegularFile::external() builds it from the pair and keeps the usual composefs layout (redirect is the digest's object path) as External, so the capi, the EROFS reader and the dumpfile parser all normalize the same way. A digest without a payload now stays that way everywhere, as in C: no redirect is made up for it, and the dumpfile parser accepts it. composefs-info lists the redirect as the object path, like C's, so ostree images don't lose their objects there. In V2 images ExternalPath gets the same single null chunk index as External. Note Item::Regular's path in dumpfile_parse is now an Option, and a redirect with an interior NUL fails the capi conversion instead of being dropped. The new test builds an image like ostree does via the C API and pins its digest; `just test-capi` compares the same image against the C library. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
|
Signed off 1 commit(s) with |
88ea0fe to
0ca2f44
Compare
|
Opened upstream as composefs#409. Closing this review draft. |
ostree gives each file a libcomposefs payload of
xx/<checksum>.fileand sets the fsverity digest separately. composefs-capi turned the payload into anObjectID(dropping.file), or ignored it when a digest was set, so the overlay redirect pointed at a file that doesn't exist and an ostree deployment on the Rust libcomposefs couldn't boot ("overlayfs: lazy lowerdata lookup failed", "No /sbin/init").This replaces
RegularFile::ExternalNoVeritywithExternalPath { redirect: Option<_>, verity: Option<_>, size }, which is how libcomposefs models a file: an optional redirect as given, plus an independent optional digest for the metacopy xattr.RegularFile::external()builds one from that pair and normalizes it, so the capi, the EROFS reader and the dumpfile parser all agree: a redirect that is the digest's object path givesExternal(the usual composefs layout), neither givesSparse, and anything else isExternalPath. The capi maps payload and digest exactly like C (empty files get no metacopy or redirect, an empty payload writes no redirect).RegularFile::backing_path()gives the redirect target of either external variant, whichcomposefs-info ls/objects/missing-objectsandcfsctl's backing-path dump now list, like C'scomposefs-infodoes with the payload.RegularFile::repo_object_id()gives callers that open the backing object from a repository the object for either variant.This is a public API change, and so is
dumpfile_parse::Item::Regular'spathbecoming anOption(see below). bootc needs a small change for both (bot/composefs-externalpathon cgwalters-bot/bootc, on top ofbootc-dev/bootc#2490): fourExternalNoVeritymatches and oneItem::Regularin its GC. That only matters when bootc bumps to a composefs-rs release with this; the revdep workflow is disabled until then anyway.Tested on a 16-core RHEL 10 devspace (kernel 6.12):
OSTREE_IMAGE_C_DIGESTcomes from the C libcomposefs (composefs/composefsec2573a):tests/ostree-image.cbuilt against it in a Fedora container, thenfsverity digestof its output. Besides ostree's forms (.filewith and without a digest, an empty file, a symlink), the image has a digest-only file and one whose payload is the digest's object path. The capi testtest_ostree_image_matches_cbuilds the same image through our capi, checks what the reader makes of each file, and gets the same digest.just test-capipasses, including the step thatcmps the image fromostree-image.cwritten by each library, and the Ctest-checksums.sh/test-units.shsuites.cargo test --workspace --exclude composefs-integration-tests -- --skip fsverity::in a Fedora container like thefedoraCI job: all pass (composefs 219, capi 22). After the last round of fixes,cargo test -p composefs-capi -p composefs-ctl,cargo clippy --workspace -- -D warnings, fmt andjust test-capiwere rerun and pass. The fsverity tests were skipped since the container's /var/tmp has no fs-verity.just clippy,check-feature-combos,fmt-checkandcheck-fuzzpass.just test-integrationin the same container: 109 passed, 3 failed, and the same 3 fail on main there (no skopeo, nosecurity.selinuxon the container's /var/tmp, and a varlink connection reset).just bootc/test-ostree(bootc from the branch above) builds the composefs RPMs, confirms libostree and the initramfs load our libcomposefs, and bootc'sreadonlyandimage-upgrade-rebootplans on the ostree backend pass, so the deployment boots and upgrades through the Rust libcomposefs.Design notes:
ExternalPath { redirect: None, verity: Some(_) }everywhere now: the capi no longer makes up a redirect from the digest, the reader no longer turns such a file intoExternal(which would add a redirect when the image is rewritten), and the dumpfile parser accepts payload-with a digest, as C does and as our own writer emits. For that,Item::Regular'spathis now anOption. Such a file has no backing path, socomposefs-info objectsdoesn't list it, like C.lcfs_load_node_from_image*, with the error logged at debug) instead of silently dropping the redirect. The test for it has a subdirectory and a hardlinked pair converted before the bad file, so freeing the partial tree is exercised; it passes under ASan (nightly-Zsanitizer=address, with LeakSanitizer) on the devspace.composefs-info missing-objectstrims leading slashes before joining a payload to--basedir, like C'sabs_to_rel_path(), so a/-prefixed payload can't escape it.Externalgets.ExternalNoVeritygot none there. That was harmless in practice, since only the capi produced it and the capi writes V1.ExternalPathgets the same index asExternal. V1 output is unchanged, and no pinned V2 digest moved.Sparsein V2 still gets no chunk index; that looks like the same issue but is left alone here.Related: composefs#323
Generated-by: https://github.com/cgwalters/#llms
Review draft in cgwalters-forge, not upstream yet. This section is removed when the PR is opened upstream.
composefs/composefs-rs, basemainPVTI_lADOE9oHIs4BlJLczg9kESwTo review:
/promoteon a line of its own, to open it upstream, ready for review. Either covers only the commits pushed so far.Signed-off-by: Colin Walters <walters@verbum.org>to the commits lacking it (the bot's and yours; anyone else's only if you ask), with you as committer./draftline (in the same comment or before) to open it upstream as a draft (/readyundoes that).