lib: add docs and safety notes to RepoCheckoutFilter

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<T>>;
+type WrapperStash<'a, GlibT, WrappedT> = Stash<'a, *mut GlibT, Option<WrappedT>>;
 
 impl<'a> ToGlibPtr<'a, *const OstreeRepoCheckoutAtOptions> for RepoCheckoutAtOptions {
     type Storage = (
         Box<OstreeRepoCheckoutAtOptions>,
         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::<OstreeRepoCheckoutAtOptions>() });
         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!(

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<dyn Fn(&Repo, &Path, &libc::stat) -> 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<dyn Fn(&Repo, &Path, &libc::stat) -> RepoCheckoutFilterResult>);
 
-pub fn repo_checkout_filter<F>(closure: F) -> Option<RepoCheckoutFilter>
+impl RepoCheckoutFilter {
-where
+    /// Wrap a closure for use as a filter function.
-    F: 'static,
+    ///
-    F: Fn(&Repo, &Path, &libc::stat) -> RepoCheckoutFilterResult,
+    /// # Return Value
-{
+    /// The return value is always `Some` containing the value. It simply comes pre-wrapped for your
-    Some(Box::new(closure))
+    /// convenience.
+    pub fn new<F>(closure: F) -> Option<RepoCheckoutFilter>
+    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<gpointer, Self> {
+        Stash(self as *const RepoCheckoutFilter as gpointer, ())
+    }
+}
+
+impl FromGlibPtrNone<gpointer> 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;
+    // We can't guarantee it's a valid pointer, but we can make sure it's not null.
-    let repo = FromGlibPtrNone::from_glib_none(repo);
+    assert!(!stat.is_null());
-    let path: PathBuf = FromGlibPtrNone::from_glib_none(path);
+    let stat = &*stat;
-    let result = (*closure)(&repo, &path, &*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);
+    }
+}

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| {
+                filter: RepoCheckoutFilter::new(|_repo, path, _stat| {
-                    if let Some("testfile") = path.file_name().map(|s| s.to_str().unwrap()) {
+                    if path == PathBuf::from("/testdir/testfile") {
                         RepoCheckoutFilterResult::Skip
                     } else {
                         RepoCheckoutFilterResult::Allow