Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dirext: Rewrite using cap_tempfile::TempFile #12

Merged
merged 6 commits into from May 4, 2022

Conversation

cgwalters
Copy link
Member

This needs a new cap_tempfile release with bytecodealliance/cap-std#239


Drop LinkableTempFile, use cap_tempfile::TempFile

Now that all the core logic has been drained into
cap_tempfile::TempFile in bytecodealliance/cap-std#239
we can just use it directly.

NOTE: This changes the semantics of some of the APIs; the previous
replace() API tried to do "reuse previous file mode if it existed"
but I think that's just a confusing trap. Anything writing the file
in the general case needs to know the permissions it wants to use.


dirext: Use "atomic" terminology

This more clearly says what we're doing; this is really the key property
versus "std" APIs like std::fs::write.
It helps match up with this crate:
https://lib.rs/crates/atomicwrites


dirext: Add a basic atomic_write

Now that we don't need to propagate Permissions everywhere,
add the obvious replacement for std::fs::write.


Drop atomic_replace_file_with_perms

This can be implemented in the closure-based API by just calling
set_permissions() alongside writes.


Build on Windows

Now that we use cap_tempfile which is portable, we can
with just a little bit of effort be portable too.


dirext: Improve and fix docs


@sunfishcode
Copy link
Collaborator

cap-std 0.24.3 is now released, with the TempFile API.

Now that all the core logic has been drained into
`cap_tempfile::TempFile` in bytecodealliance/cap-std#239
we can just use it directly.

NOTE: This changes the semantics of some of the APIs; the previous
`replace()` API tried to do "reuse previous file mode if it existed"
but I think that's just a confusing trap.  Anything writing the file
in the general case needs to know the permissions it wants to use.
This more clearly says what we're doing; this is really the key property
versus "std" APIs like `std::fs::write`.
It helps match up with this crate:
https://lib.rs/crates/atomicwrites
Now that we don't need to propagate `Permissions` everywhere,
add the obvious replacement for `std::fs::write`.
This can be implemented in the closure-based API by just calling
`set_permissions()` alongside writes.
Now that we use `cap_tempfile` which is portable, we can
with just a little bit of effort be portable too.
@cgwalters
Copy link
Member Author

cap-std 0.24.3 is now released, with the TempFile API.

Thanks! Rebased 🏄 and lifting draft.

@cgwalters cgwalters marked this pull request as ready for review May 2, 2022 20:45
@cgwalters cgwalters added the semver-break This requires a semantic version bump label May 2, 2022
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cgwalters
Copy link
Member Author

Patch to port rpm-ostree:

diff --git a/Cargo.lock b/Cargo.lock
index 159f3fe6..9c37fe46 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -173,9 +173,9 @@ dependencies = [
 
 [[package]]
 name = "cap-std"
-version = "0.24.2"
+version = "0.24.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3430d1b5ba78fa381eb227525578d2b85d77b42ff49be85d1e72a94f305e603c"
+checksum = "5d684df5773e4af5c343c466f47151db7e7a4366daab609b4a6bb7a75aecf732"
 dependencies = [
  "cap-primitives",
  "io-extras",
@@ -187,22 +187,21 @@ dependencies = [
 [[package]]
 name = "cap-std-ext"
 version = "0.24.3"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "35542c3139ae910606e9de752706b6ebd676ae31f35c963d6e64dd07f8f18c7c"
 dependencies = [
  "cap-std",
+ "cap-tempfile",
  "rustix 0.33.0",
- "uuid",
 ]
 
 [[package]]
 name = "cap-tempfile"
-version = "0.24.2"
+version = "0.24.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e1857f2acd81cd0dafd729ec386032900863bc0c542188df48f761b6fab5bb41"
+checksum = "1def2a81a97ba5f361944b55a96ab0ccf0b3b64bd829c12f20f4bf709ce2ab03"
 dependencies = [
  "cap-std",
  "rand",
+ "rustix 0.33.0",
  "uuid",
 ]
 
@@ -2743,9 +2742,9 @@ checksum = "55bcbb425141152b10d5693095950b51c3745d019363fc2929ffd8f61449b628"
 
 [[package]]
 name = "uuid"
-version = "0.8.2"
+version = "1.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bc5cf98d8186244414c848017f0e2676b3fcb46807f6668a97dfe67359a3c4b7"
+checksum = "8cfcd319456c4d6ea10087ed423473267e1a071f3bc0aa89f80d60997843c6f0"
 dependencies = [
  "getrandom",
 ]
diff --git a/Cargo.toml b/Cargo.toml
index ce493305..1bab5e90 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -109,3 +109,6 @@ bin-unit-tests = []
 sanitizers = []
 
 default = []
+
+[patch.crates-io]
+cap-std-ext = { path = "../../coreos/cap-std-ext" }
\ No newline at end of file
diff --git a/rust/src/builtins/compose/mod.rs b/rust/src/builtins/compose/mod.rs
index 298e456f..e4f03332 100644
--- a/rust/src/builtins/compose/mod.rs
+++ b/rust/src/builtins/compose/mod.rs
@@ -25,7 +25,7 @@ pub fn composeutil_legacy_prep_dev_and_run(rootfs_dfd: i32) -> CxxResult<()> {
     let rootfs = unsafe { crate::ffiutil::ffi_dirfd(rootfs_dfd)? };
     legacy_prepare_dev(&rootfs)?;
     rootfs.create_dir_with("run", DirBuilder::new().mode(0o755))?;
-    rootfs.replace_contents_with_perms(&OSTREE_BOOTED[1..], b"", Permissions::from_mode(0o755))?;
+    rootfs.atomic_write_with_perms(&OSTREE_BOOTED[1..], b"", Permissions::from_mode(0o755))?;
     Ok(())
 }
 
diff --git a/rust/src/cliwrap.rs b/rust/src/cliwrap.rs
index 3d350491..0d7b2153 100644
--- a/rust/src/cliwrap.rs
+++ b/rust/src/cliwrap.rs
@@ -106,7 +106,8 @@ fn write_one_wrapper(rootfs_dfd: &Dir, binpath: &Path, allow_noent: bool) -> Res
     if exists {
         let destpath = format!("{}/{}", CLIWRAP_DESTDIR, name);
         rootfs_dfd.rename(binpath, rootfs_dfd, destpath.as_str())?;
-        rootfs_dfd.replace_file_with_perms(binpath, perms, |w| {
+        rootfs_dfd.atomic_replace_with(binpath, |w| {
+            w.get_mut().as_file_mut().set_permissions(perms)?;
             indoc::writedoc! {w, r#"
 #!/bin/sh
 # Wrapper created by rpm-ostree to override
@@ -117,7 +118,8 @@ exec /usr/bin/rpm-ostree cliwrap $0 "$@"
 "#,  destpath }
         })?;
     } else {
-        rootfs_dfd.replace_file_with_perms(binpath, perms, |w| {
+        rootfs_dfd.atomic_replace_with(binpath, |w| {
+            w.get_mut().as_file_mut().set_permissions(perms)?;
             indoc::writedoc! {w, r#"
 #!/bin/sh
 # Wrapper created by rpm-ostree to implement this CLI interface.
diff --git a/rust/src/composepost.rs b/rust/src/composepost.rs
index ede71e11..2cf00d23 100644
--- a/rust/src/composepost.rs
+++ b/rust/src/composepost.rs
@@ -162,7 +162,8 @@ fn postprocess_useradd(rootfs_dfd: &cap_std::fs::Dir) -> Result<()> {
     let perms = cap_std::fs::Permissions::from_mode(0o644);
     if let Some(f) = rootfs_dfd.open_optional(path).context("opening")? {
         rootfs_dfd
-            .replace_file_with_perms(&path, perms, |bufw| -> Result<_> {
+            .atomic_replace_with(&path, |bufw| -> Result<_> {
+                bufw.get_mut().as_file_mut().set_permissions(perms)?;
                 let f = BufReader::new(&f);
                 for line in f.lines() {
                     let line = line?;
@@ -216,7 +217,8 @@ fn postprocess_rpm_macro(rootfs_dfd: &Dir) -> Result<()> {
     rootfs_dfd.create_dir_with(RPM_MACROS_DIR, &db)?;
     let rpm_macros_dfd = rootfs_dfd.open_dir(RPM_MACROS_DIR)?;
     let perms = cap_std::fs::Permissions::from_mode(0o644);
-    rpm_macros_dfd.replace_file_with_perms(&MACRO_FILENAME, perms, |w| -> Result<()> {
+    rpm_macros_dfd.atomic_replace_with(&MACRO_FILENAME, |w| -> Result<()> {
+        w.get_mut().as_file_mut().set_permissions(perms)?;
         w.write_all(b"%_dbpath /")?;
         w.write_all(RPMOSTREE_RPMDB_LOCATION.as_bytes())?;
         w.write_all(b"\n")?;
@@ -234,7 +236,8 @@ fn postprocess_subs_dist(rootfs_dfd: &Dir) -> Result<()> {
     let path = Path::new("usr/etc/selinux/targeted/contexts/files/file_contexts.subs_dist");
     if let Some(f) = rootfs_dfd.open_optional(path)? {
         let perms = cap_std::fs::Permissions::from_mode(0o644);
-        rootfs_dfd.replace_file_with_perms(&path, perms, |w| -> Result<()> {
+        rootfs_dfd.atomic_replace_with(&path, |w| -> Result<()> {
+            w.get_mut().as_file_mut().set_permissions(perms)?;
             let f = BufReader::new(&f);
             for line in f.lines() {
                 let line = line?;
diff --git a/rust/src/container.rs b/rust/src/container.rs
index 88fe552f..cc3bb423 100644
--- a/rust/src/container.rs
+++ b/rust/src/container.rs
@@ -350,7 +350,7 @@ pub async fn container_encapsulate(args: &[&str]) -> Result<()> {
     if let Some(v) = opt.write_contentmeta_json {
         let v = v.strip_prefix("/").unwrap_or(&v);
         let root = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
-        root.replace_file_with(v, |w| {
+        root.atomic_replace_with(v, |w| {
             serde_json::to_writer(w, &meta.sizes).map_err(anyhow::Error::msg)
         })?;
     }
diff --git a/rust/src/core.rs b/rust/src/core.rs
index f153d7c5..95a40034 100644
--- a/rust/src/core.rs
+++ b/rust/src/core.rs
@@ -143,7 +143,7 @@ impl FilesystemScriptPrep {
             let saved = &Self::saved_name(path);
             if rootfs.try_exists(path)? {
                 rootfs.rename(path, &rootfs, saved)?;
-                rootfs.replace_contents_with_perms(path, contents, mode)?;
+                rootfs.atomic_write_with_perms(path, contents, mode)?;
             }
         }
         Ok(Box::new(Self { rootfs }))
@@ -216,7 +216,7 @@ fn verify_kernel_hmac_impl(moddir: &Dir) -> Result<()> {
     }
 
     let perms = Permissions::from_mode(0o644);
-    moddir.replace_contents_with_perms(hmac_path, new_contents, perms)?;
+    moddir.atomic_write_with_perms(hmac_path, new_contents, perms)?;
 
     Ok(())
 }
@@ -265,9 +265,9 @@ mod test {
         db.recursive(true);
         d.ensure_dir_with("usr/bin", &db)?;
         d.ensure_dir_with("usr/sbin", &db)?;
-        d.replace_contents_with_perms(super::SSS_CACHE_PATH, "sss binary", mode.clone())?;
+        d.atomic_write_with_perms(super::SSS_CACHE_PATH, "sss binary", mode.clone())?;
         let original_systemctl = "original systemctl";
-        d.replace_contents_with_perms(super::SYSTEMCTL_PATH, original_systemctl, mode.clone())?;
+        d.atomic_write_with_perms(super::SYSTEMCTL_PATH, original_systemctl, mode.clone())?;
         // Replaced systemctl
         {
             assert!(d.try_exists(super::SSS_CACHE_PATH)?);
diff --git a/rust/src/countme/cookie.rs b/rust/src/countme/cookie.rs
index 29523bfc..d224613c 100644
--- a/rust/src/countme/cookie.rs
+++ b/rust/src/countme/cookie.rs
@@ -3,12 +3,10 @@
 use anyhow::{bail, Result};
 use cap_std::fs::Dir;
 use cap_std_ext::cap_std;
-use cap_std_ext::cap_std::fs::Permissions;
 use cap_std_ext::dirext::CapStdExtDirExt;
 use chrono::prelude::*;
 use serde::{Deserialize, Serialize};
 use std::io::Read;
-use std::os::unix::prelude::PermissionsExt;
 
 /// State directory used to store the countme cookie
 const STATE_DIR: &str = "var/lib/rpm-ostree-countme";
@@ -146,8 +144,7 @@ impl Cookie {
             window: self.now,
         };
         let statedir = Dir::open_ambient_dir(STATE_DIR, cap_std::ambient_authority())?;
-        let perms = Permissions::from_mode(0o644);
-        statedir.replace_file_with_perms(COUNTME_COOKIE, perms, |w| -> Result<_> {
+        statedir.atomic_replace_with(COUNTME_COOKIE, |w| -> Result<_> {
             Ok(serde_json::to_writer(w, &cookie)?)
         })
     }
diff --git a/rust/src/daemon.rs b/rust/src/daemon.rs
index fbe66e60..00370ded 100644
--- a/rust/src/daemon.rs
+++ b/rust/src/daemon.rs
@@ -354,7 +354,7 @@ fn get_cached_signatures_variant(
 
     let v = glib::Variant::from_array::<glib::Variant>(&sigs);
     let perms = cap_std::fs::Permissions::from_mode(0o600);
-    cachedir.replace_contents_with_perms(&cached_relpath, &v.data_as_bytes(), perms)?;
+    cachedir.atomic_write_with_perms(&cached_relpath, &v.data_as_bytes(), perms)?;
     Ok(v)
 }
 
diff --git a/rust/src/extensions.rs b/rust/src/extensions.rs
index 027c91f1..4a6e9956 100644
--- a/rust/src/extensions.rs
+++ b/rust/src/extensions.rs
@@ -166,7 +166,7 @@ impl Extensions {
     pub(crate) fn update_state_checksum(&self, chksum: &str, output_dir: &str) -> CxxResult<()> {
         let output_dir = Dir::open_ambient_dir(output_dir, cap_std::ambient_authority())?;
         Ok(output_dir
-            .replace_contents_with_perms(
+            .atomic_write_with_perms(
                 RPMOSTREE_EXTENSIONS_STATE_FILE,
                 chksum,
                 Permissions::from_mode(0o644),
@@ -177,11 +177,9 @@ impl Extensions {
     pub(crate) fn serialize_to_dir(&self, output_dir: &str) -> CxxResult<()> {
         let output_dir = Dir::open_ambient_dir(output_dir, cap_std::ambient_authority())?;
         Ok(output_dir
-            .replace_file_with_perms(
-                "extensions.json",
-                Permissions::from_mode(0o644),
-                |w| -> Result<_> { Ok(serde_json::to_writer_pretty(w, self)?) },
-            )
+            .atomic_replace_with("extensions.json", |w| -> Result<_> {
+                Ok(serde_json::to_writer_pretty(w, self)?)
+            })
             .context("while serializing")?)
     }
 
diff --git a/rust/src/live.rs b/rust/src/live.rs
index 8d67eb98..a8d55e02 100644
--- a/rust/src/live.rs
+++ b/rust/src/live.rs
@@ -12,7 +12,7 @@ use crate::ffi::LiveApplyState;
 use crate::isolation;
 use crate::progress::progress_task;
 use anyhow::{anyhow, Context, Result};
-use cap_std::fs::{Dir, Permissions};
+use cap_std::fs::Dir;
 use cap_std_ext::cap_std;
 use cap_std_ext::dirext::CapStdExtDirExt;
 use fn_error_context::context;
@@ -24,7 +24,6 @@ use ostree_ext::{gio, glib, ostree};
 use rayon::prelude::*;
 use std::borrow::Cow;
 use std::os::unix::io::AsRawFd;
-use std::os::unix::prelude::PermissionsExt;
 use std::path::{Path, PathBuf};
 use std::pin::Pin;
 
@@ -102,8 +101,7 @@ fn write_live_state(
 
     // Ensure the stamp file exists
     if !found_live_stamp && commit.or(inprogress_commit).is_some() {
-        let perms = Permissions::from_mode(0o644);
-        rundir.replace_contents_with_perms(LIVE_STATE_NAME, b"", perms)?;
+        rundir.atomic_write(LIVE_STATE_NAME, b"")?;
     }
 
     Ok(())
diff --git a/rust/src/lockfile.rs b/rust/src/lockfile.rs
index 7d9b9493..187a940c 100644
--- a/rust/src/lockfile.rs
+++ b/rust/src/lockfile.rs
@@ -13,7 +13,6 @@
 pub use self::ffi::*;
 use crate::utils;
 use anyhow::{anyhow, bail, Result};
-use cap_std::fs::Permissions;
 use cap_std_ext::cap_std;
 use cap_std_ext::dirext::CapStdExtDirExt;
 use chrono::prelude::*;
@@ -22,7 +21,6 @@ use std::collections::BTreeMap;
 use std::convert::TryInto;
 use std::io;
 use std::iter::Extend;
-use std::os::unix::fs::PermissionsExt;
 use std::path::Path;
 use std::pin::Pin;
 
@@ -420,7 +418,7 @@ pub(crate) fn lockfile_write(
 
     let (dir, path) =
         crate::capstdext::open_dir_of(Path::new(filename), cap_std::ambient_authority())?;
-    dir.replace_file_with_perms(path, Permissions::from_mode(0o644), |w| -> Result<()> {
+    dir.atomic_replace_with(path, |w| -> Result<()> {
         Ok(serde_json::to_writer_pretty(w, &lockfile)?)
     })?;
     Ok(())

@cgwalters cgwalters merged commit 5042352 into coreos:main May 4, 2022
jlebon added a commit to jlebon/ostree-rs-ext that referenced this pull request Jun 24, 2022
We use `atomic_write` which was added in 0.25:
coreos/cap-std-ext#12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-break This requires a semantic version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants