crates: add composefs-capi cdylib crate#321
Conversation
c218f1f to
52e8adb
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Thanks for this! I took a quick skim.
One thing I think that would be a great test: run tests for the original C mkcomposefs and composefs-info from the C codebase using this shared library. I think we could hack that up in an integration test container.
Also a bigger and even more important test: Change the bootc revdep test to drop this shared library into the target container - then the existing ostree -> composefs logic would run through it.
| return -1; | ||
| } | ||
|
|
||
| let dup_fd = unsafe { libc::dup(fd) }; |
There was a problem hiding this comment.
I think it's cleaner to use https://doc.rust-lang.org/stable/std/os/fd/struct.BorrowedFd.html#method.borrow_raw
| if digest.is_null() || data.is_null() { | ||
| set_errno(libc::EINVAL); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
More elegant I think to use e.g. https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.new like
let Some(digest) = NonNull::new(digest) else { set_errno(libc::EINVAL); return -1 };
could wrap in a macro too
| return -1; | ||
| } | ||
|
|
||
| let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| unsafe { |
There was a problem hiding this comment.
I would personally prefer to compile the Rust code with panic=abort. That's what we do in rpm-ostree.
|
|
||
| // If it's a directory, recurse | ||
| if ((*child).mode & libc::S_IFMT) == libc::S_IFDIR { | ||
| let child_fd = libc::openat( |
There was a problem hiding this comment.
I know this is a translation, but there's nothing that stops us from using say rustix here instead so we get safety (including Drop handling).
| } | ||
|
|
||
| unsafe fn build_dir_recursive(dirfd: c_int, buildflags: c_int) -> Option<*mut FfiNode> { | ||
| unsafe { |
There was a problem hiding this comment.
I am thinking it might actually be cleaner to have this whole function be implemented by using the rust-side Filesystem API and then having a single mapping between Filesystem -> FfiNode ?
|
|
||
| unsafe { | ||
| let path_cstr = CStr::from_ptr(path); | ||
| let path_str = match path_cstr.to_str() { |
There was a problem hiding this comment.
We don't need to require the paths are utf8
| mtime_nsec: u32, | ||
| digest: [u8; LCFS_DIGEST_SIZE], | ||
| digest_set: bool, | ||
| hardlink_target: *mut FfiNode, |
There was a problem hiding this comment.
This case can use Option<NonNull<FfiNode>>
| let path_cstr = CStr::from_ptr(path); | ||
| let path_str = match path_cstr.to_str() { | ||
| Ok(s) => s, | ||
| Err(_) => { |
There was a problem hiding this comment.
Here and elsewhere I think it'd be cleaner to have a helper macro or closure which handles this Err -> errno+return. I think we have code for that in rpm-ostree.
| @@ -0,0 +1,25 @@ | |||
| [package] | |||
| name = "composefs-ffi" | |||
There was a problem hiding this comment.
I'd kind of prefer the name -capi, there's not a really strong convention for this though.
3aed60d to
738bf32
Compare
| /// Like `External`, but without embedding the fsverity digest in the | ||
| /// overlay metacopy xattr. Used by the C API when the caller set a | ||
| /// content-address payload but did not explicitly set a verified digest. | ||
| ExternalNoVerity(ObjectID, u64), |
There was a problem hiding this comment.
Is this coming from dumpfiles in the C test cases? I don't think it's something mkcomposefs would do right?
I guess in the end, if one is intending to use composefs without enforcing fsverity, it is technically OK...but OTOH, a caller can just always pass a verity digest and then just mount without the verity=require option for overlayfs.
Oh you know what, I bet libostree is doing this case if we don't have fsverity enabled on the object store. OK so yeah we probably do need to handle this.
| ExternalNoVerity(ObjectID, u64), | ||
| /// File with declared size but no content or external reference. | ||
| /// Produces ChunkBased layout with null chunk indices. | ||
| Sparse(u64), |
There was a problem hiding this comment.
Hmmm...this one seems just nonsensical. I can imagine the C library allows constructing it, and the C dumpfile parser probably does too, but the file would just be unreadable at runtime right? Or maybe it appears as a big chunk of zeroes?
I guess again if the C library technically allowed this, we probably need to as well...
There was a problem hiding this comment.
Yes, i think its just ends up being read as zeros. I'm not sure why we would want to use this though, I don't see any use for it. Maybe just an accident of the API?
There was a problem hiding this comment.
But, yeah, if the C api allows it, and its not too much work to support...
|
OK, I spent some tokens on crosschecking with the C impl, can you squash in cgwalters@34ee730 - I think that's all correct. |
| anyhow::bail!( | ||
| "Sparse file not supported as delta source: {}", | ||
| path.display() | ||
| ); |
There was a problem hiding this comment.
I mean, we could easily create a sparse tmpfile fd of the right lenght with just a truncate() call here.
There was a problem hiding this comment.
Yeah; OTOH no one should be using this anyways
|
|
||
| fn bitor(self, permissions: u32) -> ModeField { | ||
| ModeField(self | (permissions as u16)) | ||
| ModeField(self | (permissions as u16 & !S_IFMT)) |
There was a problem hiding this comment.
I think this is likely right. If we want to or some new permissions we don't want to inherit some new mode. But, maybe a comment about this?
| } else { | ||
| if img.composefs_restricted { | ||
| if img.composefs_restricted | ||
| && img.header.composefs_version == COMPOSEFS_VERSION |
There was a problem hiding this comment.
I see the C code doesn't enforce this limit, so this is correct, but can you add a comment to the MAX_INLINE_CONTENT that it only applies to validating v2 images.
| let target_data = extract_all_file_data(img, &child_inode)?; | ||
| if img.composefs_restricted | ||
| && img.header.composefs_version == COMPOSEFS_VERSION | ||
| && target_data.len() > crate::SYMLINK_MAX |
There was a problem hiding this comment.
Same here, SYMLINK_MAX_V2
| const CHUNK_FORMAT_BLKBITS_MASK: u32 = 0x001F; // 31 | ||
|
|
||
| // Compute the chunkbits to use for the file size. | ||
| // We want as few chunks as possible, but not an unnecessarily large chunk. |
There was a problem hiding this comment.
Bunch of comments here are unnecessarily removed (which claude seems to do all the time...)
| /// xattrs + target would fill a full block. Must run after share_xattrs (so | ||
| /// xattr sizes are final) and after calculate_min_mtime (so compact/extended | ||
| /// is deterministic). | ||
| fn fixup_epoch1_data_blocks<ObjectID: FsVerityHashValue>( |
There was a problem hiding this comment.
There is so many details here with the padding, block/inline splitting, etc that I fear that a regular review just cannot be trusted for this. We already have test_mkcomposefs_vs_c_mkcomposefs which compares that a basic image gets the same thing, I think we need to extend this to a more fuzz-like thing that generates a bunch of random images with different file sizes, etc, to try to shake out any issues in this.
There was a problem hiding this comment.
There's actually several of those tests - and I think test_v1_binary_identical_to_c_mkcomposefs is already covering this.
proptests aren't quite the same thing as fuzzing, but IME get us 90% of the way there cheaply. The challenge is to really do intelligent coverage-guided fuzzing we need it all in the same process so we'd need a harness that calls into the original C library through Rust bindings or so.
What is likely missing though here is to extend LeafContentSpec to cover both the new cases (external-no-verity and sparse-zeroes).
| @@ -0,0 +1,461 @@ | |||
| /* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */ | |||
There was a problem hiding this comment.
I wonder if we should put the non-public headers in a separate directory?
|
|
||
| # Headers | ||
| SCRIPTDIR="$(cd "$(dirname "$0")" && pwd)" | ||
| for h in "${SCRIPTDIR}"/include/libcomposefs/*.h; do |
There was a problem hiding this comment.
Not all the headers we have locally should be public, they are not in the C version.
| set -eu | ||
|
|
||
| PREFIX="${1:-/usr/local}" | ||
| LIBDIR="${PREFIX}/lib" |
There was a problem hiding this comment.
Shouldn't we allow overrides for this too for lib64, etc?
| } | ||
|
|
||
| pub(crate) struct FfiNode { | ||
| ref_count: i32, |
There was a problem hiding this comment.
Would it not be possible to use an actual Rc for this? Maybe that just overcomplicates things internally.
There was a problem hiding this comment.
Yeah, it probably just makes it more likely that we'll get some behavioral differences.
|
I didn't do a super detailed review, but left some comments. Overall I think this is a viable approach, although i have some overall ideas. First of all, (and i mentioned this in a comment), we really need to machine-verify that these block allocation and inline behavior produces the same result in all cases. We should have some code to generate random dump files with a wide variety of file sizes and stuff, and then feed it into both implementation and validate we get the exact same results. Secondly, the primary C consumer of the library is ostree, we need to test it with the new version, and specifically we need to ensure it produces the same bitwise identical composefs images for a given ostree commit. Also, I'm not sure some of the rust code in libcomposefs-capi is actually cleaner than the C code. In particular the code around managing lcfs_node_s trees. I wonder if for some of this code it wouldn't be better to just take the C code directly, and then just have the "meat" in pure rust. I dunno... |
428fcaf to
922d647
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
| value: Option<String>, | ||
| } | ||
|
|
||
| fn parse_mount_options(options: &str) -> Vec<ParsedOption> { |
There was a problem hiding this comment.
We could use clap right?
|
|
||
| /// Applies an ID mapping from a user namespace to a mount. | ||
| pub fn mount_setattr_idmap( | ||
| mount_fd: BorrowedFd<'_>, |
There was a problem hiding this comment.
Fine as is, but a bit more typical to use impl AsFd for these
Add two new RegularFile variants to support files that the C composefs library produces but the Rust writer did not previously handle: - ExternalNoVerity: external file with payload path but no fsverity digest (empty overlay.metacopy xattr + overlay.redirect). - Sparse: file with declared size but no content or external reference (ChunkBased layout with null chunk indices). Update the EROFS writer, reader, and dumpfile round-trip to handle both variants correctly in Epoch1 format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
It provides the libcomposefs C API. Add a new crate that produces a drop-in libcomposefs.so shared library backed by the Rust composefs implementation. This enables C-only consumers to use the Rust codebase without source changes. The crate ships the original C headers unchanged and includes a pkg-config template. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Run the C composefs test suite (test-checksums, test-units) against the Rust-built libcomposefs shared library in a container. Uses the existing `just test-capi` recipe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Change composefs_fsmount to accept a slice of base directory file descriptors instead of a single one. This allows mounting composefs images that reference objects spread across multiple directories, matching the colon-separated basedir support in the C implementation. Update both the pre-6.15 and modern kernel code paths in mountcompat, the repository mount helper, and the C API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Replace the boolean enable_verity parameter with a VerityRequirement enum that distinguishes between Required (fail if unsupported), Try (silently continue if EINVAL/ENOSYS), and Disabled. This matches the C implementation's distinction between the verity and tryverity mount options. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Add support for applying a user namespace ID mapping to the erofs lower layer via mount_setattr(MOUNT_ATTR_IDMAP). The raw syscall wrapper lives in composefs-ioctls since rustix does not yet expose mount_setattr and the composefs crate forbids unsafe code. MountOptions gains an idmap_fd field and set_idmap() method. The idmap is applied to the erofs mount between fsmount() and prepare_mount(), matching the C implementation's placement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Add a Rust reimplementation of the C mount.composefs tool as a new module in composefs-ctl. The tool is dispatched via the existing multi-call binary pattern — when cfsctl is symlinked as `mount.composefs`, the kernel dispatches to it for `mount -t composefs` commands. Supported mount options match the C tool: basedir (with colon-separated multiple directories), digest (with fsverity image verification), verity, tryverity, idmap, ro, rw, upperdir, and workdir. The Makefile gains `build-ctl` and `install-ctl` targets that build cfsctl and install it along with symlinks for mkcomposefs, composefs-info, and mount.composefs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Port the mount digest verification tests from the C composefs test
suite (tests/test-units.sh). These exercise:
- test_mount_basic: creates an image and attempts a mount
- test_mount_digest: verifies fs-verity digest checking, including
"Image has no fs-verity" and "Image has wrong fs-verity" errors
Adapted from composefs tests/test-units.sh and tests/test-lib.sh.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Copy man pages from the C composefs project for mount.composefs(8), mkcomposefs(1), composefs-info(1), and composefs-dump(5). These document the same CLI interface implemented by cfsctl. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
@alexlarsson @cgwalters I've added |
Yeah, though I think we can strengthen testing here by copying in |
| VerityRequirement::Try => { | ||
| match fsconfig_set_string(overlayfs.as_fd(), "verity", "require") { |
There was a problem hiding this comment.
I wonder though...was this ever really working? AFAICS, this is basically about just the raw overlayfs verity=require support, and I think at this point most systems that care about composefs should have that.
Those that don't really just should be specifying enable_verity: false.
Actually something notable - since this fsmount code was written, at the repository level we now know directly from meta.json whether fsverity is enabled or not. I think we could probably wire this up better in the future - if the repository's verity is disabled, then we should default to disabling it for mounts too. Well, except for the sealed UKI case, where source of truth has to be the composefs= karg.
| anyhow::bail!( | ||
| "Sparse file not supported as delta source: {}", | ||
| path.display() | ||
| ); |
There was a problem hiding this comment.
Yeah; OTOH no one should be using this anyways
do you prefer it as part of this PR or as a follow-up? |
cgwalters
left a comment
There was a problem hiding this comment.
From my PoV I think since (like composefs-ostree) this is basically mostly just a new opt-in crate for the C library, it should be good to merge and continue to iterate from there!
| @@ -0,0 +1,272 @@ | |||
| //! mount.composefs - Mount helper for composefs images. | |||
| //! | |||
| //! This is a Rust reimplementation of the C mount.composefs tool, providing | |||
There was a problem hiding this comment.
I think ideally in the future we bridge this to the varlink API, and we also should extend it to support --repo.
It provides the libcomposefs C API.
Add a new crate that produces a drop-in libcomposefs.so shared library backed by the Rust composefs implementation. This enables C-only consumers to use the Rust codebase without source changes.
The crate ships the original C headers unchanged and includes a pkg-config template.
I've not reviewed carefully the code generated by Claude, before I do it, I'd like to gather some comments, and if this is a viable option to replace libcomposefs.
@alexlarsson @cgwalters PTAL