diff --git a/rust-bindings/rust/src/repo_checkout_at_options.rs b/rust-bindings/rust/src/repo_checkout_at_options.rs index 4d345d22..0ebf7be7 100644 --- a/rust-bindings/rust/src/repo_checkout_at_options.rs +++ b/rust-bindings/rust/src/repo_checkout_at_options.rs @@ -1,13 +1,12 @@ use crate::{RepoCheckoutMode, RepoCheckoutOverwriteMode, RepoDevInoCache, SePolicy}; use glib::translate::{Stash, ToGlib, ToGlibPtr}; -use glib_sys::gpointer; use libc::c_char; -use ostree_sys::OstreeRepoCheckoutAtOptions; +use ostree_sys::{OstreeRepoCheckoutAtOptions, OstreeRepoDevInoCache, OstreeSePolicy}; use std::path::PathBuf; mod repo_checkout_filter; -pub use self::repo_checkout_filter::{repo_checkout_filter, RepoCheckoutFilter}; +pub use self::repo_checkout_filter::RepoCheckoutFilter; pub struct RepoCheckoutAtOptions { pub mode: RepoCheckoutMode, @@ -48,15 +47,25 @@ impl Default for RepoCheckoutAtOptions { } type StringStash<'a, T> = Stash<'a, *const c_char, Option>; +type WrapperStash<'a, GlibT, WrappedT> = Stash<'a, *mut GlibT, Option>; impl<'a> ToGlibPtr<'a, *const OstreeRepoCheckoutAtOptions> for RepoCheckoutAtOptions { type Storage = ( Box, StringStash<'a, PathBuf>, StringStash<'a, String>, + WrapperStash<'a, OstreeRepoDevInoCache, RepoDevInoCache>, + WrapperStash<'a, OstreeSePolicy, SePolicy>, ); + // We need to make sure that all memory pointed to by the returned pointer is kept alive by + // either the `self` reference or the returned Stash. fn to_glib_none(&'a self) -> Stash<*const OstreeRepoCheckoutAtOptions, Self> { + // Creating this struct from zeroed memory is fine since it's `repr(C)` and only contains + // primitive types. In fact, the libostree docs say to zero the struct. This means we handle + // the unused bytes correctly. + // The struct needs to be boxed so the pointer we return remains valid even as the Stash is + // moved around. let mut options = Box::new(unsafe { std::mem::zeroed::() }); options.mode = self.mode.to_glib(); options.overwrite_mode = self.overwrite_mode.to_glib(); @@ -68,6 +77,8 @@ impl<'a> ToGlibPtr<'a, *const OstreeRepoCheckoutAtOptions> for RepoCheckoutAtOpt options.bareuseronly_dirs = self.bareuseronly_dirs.to_glib(); options.force_copy_zerosized = self.force_copy_zerosized.to_glib(); + // We keep these complex values alive by returning them in our Stash. Technically, some of + // these are being kept alive by `self` already, but it's better to be consistent here. let subpath = self.subpath.to_glib_none(); options.subpath = subpath.0; let sepolicy_prefix = self.sepolicy_prefix.to_glib_none(); @@ -78,11 +89,20 @@ impl<'a> ToGlibPtr<'a, *const OstreeRepoCheckoutAtOptions> for RepoCheckoutAtOpt options.sepolicy = sepolicy.0; if let Some(filter) = &self.filter { - options.filter_user_data = filter as *const RepoCheckoutFilter as gpointer; + options.filter_user_data = filter.to_glib_none().0; options.filter = repo_checkout_filter::trampoline(); } - Stash(options.as_ref(), (options, subpath, sepolicy_prefix)) + Stash( + options.as_ref(), + ( + options, + subpath, + sepolicy_prefix, + devino_to_csum_cache, + sepolicy, + ), + ) } } @@ -91,7 +111,7 @@ mod tests { use super::*; use crate::RepoCheckoutFilterResult; use gio::{File, NONE_CANCELLABLE}; - use glib_sys::{gpointer, GFALSE, GTRUE}; + use glib_sys::{GFALSE, GTRUE}; use ostree_sys::{ OSTREE_REPO_CHECKOUT_MODE_NONE, OSTREE_REPO_CHECKOUT_MODE_USER, OSTREE_REPO_CHECKOUT_OVERWRITE_NONE, OSTREE_REPO_CHECKOUT_OVERWRITE_UNION_IDENTICAL, @@ -140,7 +160,7 @@ mod tests { force_copy_zerosized: true, subpath: Some("sub/path".into()), devino_to_csum_cache: Some(RepoDevInoCache::new()), - filter: repo_checkout_filter(|_repo, _path, _stat| RepoCheckoutFilterResult::Skip), + filter: RepoCheckoutFilter::new(|_repo, _path, _stat| RepoCheckoutFilterResult::Skip), sepolicy: Some(SePolicy::new(&File::new_for_path("a/b"), NONE_CANCELLABLE).unwrap()), sepolicy_prefix: Some("prefix".into()), }; @@ -173,7 +193,7 @@ mod tests { assert_eq!((*ptr).filter, repo_checkout_filter::trampoline()); assert_eq!( (*ptr).filter_user_data, - options.filter.as_ref().unwrap() as *const RepoCheckoutFilter as gpointer + options.filter.as_ref().unwrap().to_glib_none().0, ); assert_eq!((*ptr).sepolicy, options.sepolicy.to_glib_none().0); assert_eq!( diff --git a/rust-bindings/rust/src/repo_checkout_at_options/repo_checkout_filter.rs b/rust-bindings/rust/src/repo_checkout_at_options/repo_checkout_filter.rs index c4caac4b..f83af232 100644 --- a/rust-bindings/rust/src/repo_checkout_at_options/repo_checkout_filter.rs +++ b/rust-bindings/rust/src/repo_checkout_at_options/repo_checkout_filter.rs @@ -1,20 +1,69 @@ use crate::{Repo, RepoCheckoutFilterResult}; -use glib::translate::{FromGlibPtrNone, ToGlib}; +use glib::translate::{ + from_glib_borrow, from_glib_none, FromGlibPtrNone, Stash, ToGlib, ToGlibPtr, +}; use glib_sys::gpointer; use libc::c_char; use ostree_sys::{OstreeRepo, OstreeRepoCheckoutFilterResult}; use std::path::{Path, PathBuf}; -pub type RepoCheckoutFilter = Box RepoCheckoutFilterResult>; +/// A filter callback to decide which files to checkout from a [Repo](struct.Repo.html). The +/// function is called for every directory and file in the dirtree. +/// +/// # Arguments +/// * `repo` - the `Repo` that is being checked out +/// * `path` - the path of the current file, as an absolute path rooted at the commit's root. The +/// root directory is '/', a subdir would be '/subdir' etc. +/// * `stat` - the metadata of the current file +/// +/// # Return Value +/// The return value determines whether the current file is checked out or skipped. +pub struct RepoCheckoutFilter(Box RepoCheckoutFilterResult>); -pub fn repo_checkout_filter(closure: F) -> Option -where - F: 'static, - F: Fn(&Repo, &Path, &libc::stat) -> RepoCheckoutFilterResult, -{ - Some(Box::new(closure)) +impl RepoCheckoutFilter { + /// Wrap a closure for use as a filter function. + /// + /// # Return Value + /// The return value is always `Some` containing the value. It simply comes pre-wrapped for your + /// convenience. + pub fn new(closure: F) -> Option + where + F: Fn(&Repo, &Path, &libc::stat) -> RepoCheckoutFilterResult, + F: 'static, + { + Some(RepoCheckoutFilter(Box::new(closure))) + } + + /// Call the contained closure. + fn call(&self, repo: &Repo, path: &Path, stat: &libc::stat) -> RepoCheckoutFilterResult { + self.0(repo, path, stat) + } } +impl<'a> ToGlibPtr<'a, gpointer> for RepoCheckoutFilter { + type Storage = (); + + fn to_glib_none(&'a self) -> Stash { + Stash(self as *const RepoCheckoutFilter as gpointer, ()) + } +} + +impl FromGlibPtrNone for &RepoCheckoutFilter { + // `ptr` must be valid for the lifetime of the returned reference. + unsafe fn from_glib_none(ptr: gpointer) -> Self { + assert!(!ptr.is_null()); + &*(ptr as *const RepoCheckoutFilter) + } +} + +/// Trampoline to be called by libostree that calls the Rust closure in the `user_data` parameter. +/// +/// # Safety +/// All parameters must be valid pointers for the runtime of the function. In particular, +/// `user_data` must point to a [RepoCheckoutFilter](struct.RepoCheckoutFilter.html) value. +/// +/// # Panics +/// If any parameter is a null pointer, the function panics. unsafe extern "C" fn filter_trampoline( repo: *mut OstreeRepo, path: *const c_char, @@ -22,13 +71,25 @@ unsafe extern "C" fn filter_trampoline( user_data: gpointer, ) -> OstreeRepoCheckoutFilterResult { // TODO: handle unwinding - let closure = user_data as *const RepoCheckoutFilter; - let repo = FromGlibPtrNone::from_glib_none(repo); - let path: PathBuf = FromGlibPtrNone::from_glib_none(path); - let result = (*closure)(&repo, &path, &*stat); + // We can't guarantee it's a valid pointer, but we can make sure it's not null. + assert!(!stat.is_null()); + let stat = &*stat; + // This reference is valid until the end of this function, which is shorter than the lifetime + // of `user_data` so we're fine. + let closure: &RepoCheckoutFilter = from_glib_none(user_data); + // `repo` lives at least until the end of this function. This means we can just borrow the + // reference so long as our `repo` is not moved out of this function. + let repo = from_glib_borrow(repo); + // This is a copy so no problems here. + let path: PathBuf = from_glib_none(path); + + let result = closure.call(&repo, &path, stat); result.to_glib() } +/// Returns the trampoline function in a `Some`. +/// +/// This is mostly convenient because the full type needs to be written out in fewer places. pub(super) fn trampoline() -> Option< unsafe extern "C" fn( *mut OstreeRepo, @@ -39,3 +100,101 @@ pub(super) fn trampoline() -> Option< > { Some(filter_trampoline) } + +#[cfg(test)] +mod tests { + use super::*; + use glib::translate::ToGlibPtr; + use ostree_sys::OSTREE_REPO_CHECKOUT_FILTER_SKIP; + use std::ffi::CString; + use std::ptr; + + #[test] + #[should_panic] + fn trampoline_should_panic_if_repo_is_nullptr() { + let path = CString::new("/a/b/c").unwrap(); + let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + let filter = RepoCheckoutFilter(Box::new(|_, _, _| RepoCheckoutFilterResult::Allow)); + unsafe { + filter_trampoline( + ptr::null_mut(), + path.as_ptr(), + &mut stat, + filter.to_glib_none().0, + ); + } + } + + #[test] + #[should_panic] + fn trampoline_should_panic_if_path_is_nullptr() { + let repo = Repo::new_default(); + let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + let filter = RepoCheckoutFilter(Box::new(|_, _, _| RepoCheckoutFilterResult::Allow)); + unsafe { + filter_trampoline( + repo.to_glib_none().0, + ptr::null(), + &mut stat, + filter.to_glib_none().0, + ); + } + } + + #[test] + #[should_panic] + fn trampoline_should_panic_if_stat_is_nullptr() { + let repo = Repo::new_default(); + let path = CString::new("/a/b/c").unwrap(); + let filter = RepoCheckoutFilter(Box::new(|_, _, _| RepoCheckoutFilterResult::Allow)); + unsafe { + filter_trampoline( + repo.to_glib_none().0, + path.as_ptr(), + ptr::null_mut(), + filter.to_glib_none().0, + ); + } + } + + #[test] + #[should_panic] + fn trampoline_should_panic_if_user_data_is_nullptr() { + let repo = Repo::new_default(); + let path = CString::new("/a/b/c").unwrap(); + let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + unsafe { + filter_trampoline( + repo.to_glib_none().0, + path.as_ptr(), + &mut stat, + ptr::null_mut(), + ); + } + } + + #[test] + fn trampoline_should_call_the_closure() { + let repo = Repo::new_default(); + let path = CString::new("/a/b/c").unwrap(); + let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + let filter = { + let repo = repo.clone(); + let path = path.clone(); + RepoCheckoutFilter(Box::new(move |arg_repo, arg_path, _| { + assert_eq!(arg_repo, &repo); + assert_eq!(&CString::new(arg_path.to_str().unwrap()).unwrap(), &path); + RepoCheckoutFilterResult::Skip + })) + }; + let result = unsafe { + filter_trampoline( + repo.to_glib_none().0, + path.as_ptr(), + &mut stat, + filter.to_glib_none().0, + ) + }; + assert_eq!(result, OSTREE_REPO_CHECKOUT_FILTER_SKIP); + } +} diff --git a/rust-bindings/rust/tests/repo/checkout_at.rs b/rust-bindings/rust/tests/repo/checkout_at.rs index f0476f09..01bfac9d 100644 --- a/rust-bindings/rust/tests/repo/checkout_at.rs +++ b/rust-bindings/rust/tests/repo/checkout_at.rs @@ -2,6 +2,7 @@ use crate::util::*; use gio::NONE_CANCELLABLE; use ostree::*; use std::os::unix::io::AsRawFd; +use std::path::PathBuf; #[test] fn should_checkout_at_with_none_options() { @@ -62,7 +63,9 @@ fn should_checkout_at_with_options() { force_copy: true, force_copy_zerosized: true, devino_to_csum_cache: Some(RepoDevInoCache::new()), - filter: repo_checkout_filter(|_repo, _path, _stat| RepoCheckoutFilterResult::Allow), + filter: RepoCheckoutFilter::new(|_repo, _path, _stat| { + RepoCheckoutFilterResult::Allow + }), ..Default::default() }), dirfd.as_raw_fd(), @@ -86,8 +89,8 @@ fn should_checkout_at_with_filter() { .repo .checkout_at( Some(&RepoCheckoutAtOptions { - filter: repo_checkout_filter(|_repo, path, _stat| { - if let Some("testfile") = path.file_name().map(|s| s.to_str().unwrap()) { + filter: RepoCheckoutFilter::new(|_repo, path, _stat| { + if path == PathBuf::from("/testdir/testfile") { RepoCheckoutFilterResult::Skip } else { RepoCheckoutFilterResult::Allow