From 2fb154d098f5e5784f9aa70ed3b1b2350ca94638 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 12 Dec 2023 09:53:47 -0500 Subject: [PATCH 1/4] tmpfiles: Rename reader function --- rust/src/tmpfiles.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/src/tmpfiles.rs b/rust/src/tmpfiles.rs index 0c7af2e3d6..bcaaac5081 100644 --- a/rust/src/tmpfiles.rs +++ b/rust/src/tmpfiles.rs @@ -27,7 +27,7 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { let tmpfiles_dir = tmprootfs_dfd .open_dir(RPMOSTREE_TMPFILESD) .context("readdir {RPMOSTREE_TMPFILESD}")?; - let mut rpmostree_tmpfiles_entries = save_tmpfile_entries(&tmpfiles_dir)? + let mut rpmostree_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)? .map(|s| { let entry = tmpfiles_entry_get_path(&s.as_str())?; anyhow::Ok((entry.to_string(), s.to_string())) @@ -42,7 +42,7 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { if tmpfiles_dir.try_exists(AUTOVAR_PATH)? { tmpfiles_dir.remove_file(AUTOVAR_PATH)?; } - let system_tmpfiles_entries = save_tmpfile_entries(&tmpfiles_dir)? + let system_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)? .map(|s| { let entry = tmpfiles_entry_get_path(&s.as_str())?; anyhow::Ok(entry.to_string()) @@ -69,7 +69,7 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { } // #[context("Scan all tmpfiles conf and save entries")] -fn save_tmpfile_entries(tmpfiles_dir: &Dir) -> Result> { +fn read_tmpfiles(tmpfiles_dir: &Dir) -> Result> { let entries = tmpfiles_dir .entries()? .filter_map(|name| { From 36e935b65dc94949730b55c2aae896fe38349259 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 12 Dec 2023 10:09:16 -0500 Subject: [PATCH 2/4] tmpfiles: Change `read_tmpfiles` to return a direct hashmap While returning an iterator was more elegant, it makes error handling much more difficult *and* we had intermediate `collects()` that meant it wasn't really lazy. Trying to remove the intermediate `collect` runs into borrowing issues (on the input `&Dir`) and this isn't a high performance path so doing the more obvious simpler thing is better. --- rust/src/tmpfiles.rs | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/rust/src/tmpfiles.rs b/rust/src/tmpfiles.rs index bcaaac5081..75c3a78082 100644 --- a/rust/src/tmpfiles.rs +++ b/rust/src/tmpfiles.rs @@ -27,12 +27,7 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { let tmpfiles_dir = tmprootfs_dfd .open_dir(RPMOSTREE_TMPFILESD) .context("readdir {RPMOSTREE_TMPFILESD}")?; - let mut rpmostree_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)? - .map(|s| { - let entry = tmpfiles_entry_get_path(&s.as_str())?; - anyhow::Ok((entry.to_string(), s.to_string())) - }) - .collect::>>()?; + let mut rpmostree_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)?; // remove autovar.conf first, then scan all system entries and save let tmpfiles_dir = tmprootfs_dfd @@ -43,11 +38,9 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { tmpfiles_dir.remove_file(AUTOVAR_PATH)?; } let system_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)? - .map(|s| { - let entry = tmpfiles_entry_get_path(&s.as_str())?; - anyhow::Ok(entry.to_string()) - }) - .collect::>>()?; + .into_iter() + .map(|v| v.0) + .collect::>(); // remove duplicated entries in auto-generated tmpfiles.d, // which are already in system tmpfiles @@ -68,9 +61,11 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { Ok(()) } -// #[context("Scan all tmpfiles conf and save entries")] -fn read_tmpfiles(tmpfiles_dir: &Dir) -> Result> { - let entries = tmpfiles_dir +/// Read all tmpfiles.d entries in the target directory, and return a mapping +/// from (file path) => (single tmpfiles.d entry line) +#[context("Read systemd tmpfiles.d")] +fn read_tmpfiles(tmpfiles_dir: &Dir) -> Result> { + tmpfiles_dir .entries()? .filter_map(|name| { let name = name.unwrap().file_name(); @@ -92,9 +87,11 @@ fn read_tmpfiles(tmpfiles_dir: &Dir) -> Result> { ) }) .flatten() - .collect::>(); - - Ok(entries.into_iter()) + .map(|s| { + let entry = tmpfiles_entry_get_path(s.as_str())?; + anyhow::Ok((entry.to_string(), s)) + }) + .collect() } #[context("Scan tmpfiles entries and get path")] From 9bc1e35a23a1da8d36ceddb136681332727c2688 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 12 Dec 2023 10:14:45 -0500 Subject: [PATCH 3/4] tmpfiles: Collect into a BTreeMap for reproducibility I hit a test failure in one run because HashMap is not ordered predictably. --- rust/src/tmpfiles.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/src/tmpfiles.rs b/rust/src/tmpfiles.rs index 75c3a78082..d0d424a65a 100644 --- a/rust/src/tmpfiles.rs +++ b/rust/src/tmpfiles.rs @@ -10,7 +10,7 @@ use anyhow::{Context, Result}; use cap_std::fs::{Dir, Permissions}; use cap_std_ext::dirext::CapStdExtDirExt; use fn_error_context::context; -use std::collections::{HashMap, HashSet}; +use std::collections::BTreeMap; use std::fmt::Write; use std::os::unix::prelude::PermissionsExt; use std::path::Path; @@ -40,7 +40,7 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { let system_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)? .into_iter() .map(|v| v.0) - .collect::>(); + .collect::>(); // remove duplicated entries in auto-generated tmpfiles.d, // which are already in system tmpfiles @@ -64,7 +64,7 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { /// Read all tmpfiles.d entries in the target directory, and return a mapping /// from (file path) => (single tmpfiles.d entry line) #[context("Read systemd tmpfiles.d")] -fn read_tmpfiles(tmpfiles_dir: &Dir) -> Result> { +fn read_tmpfiles(tmpfiles_dir: &Dir) -> Result> { tmpfiles_dir .entries()? .filter_map(|name| { From 3cb6ebd02acd56a29d48f349df2207d1a5081c3e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 12 Dec 2023 10:16:19 -0500 Subject: [PATCH 4/4] tmpfiles: Drop intermediate re-allocation Since we're now returning a direct map we don't need to convert to a set, we can just ignore the values when iterating. --- rust/src/tmpfiles.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/rust/src/tmpfiles.rs b/rust/src/tmpfiles.rs index d0d424a65a..bec240d1a8 100644 --- a/rust/src/tmpfiles.rs +++ b/rust/src/tmpfiles.rs @@ -37,14 +37,11 @@ pub fn deduplicate_tmpfiles_entries(tmprootfs_dfd: i32) -> CxxResult<()> { if tmpfiles_dir.try_exists(AUTOVAR_PATH)? { tmpfiles_dir.remove_file(AUTOVAR_PATH)?; } - let system_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)? - .into_iter() - .map(|v| v.0) - .collect::>(); + let system_tmpfiles_entries = read_tmpfiles(&tmpfiles_dir)?; // remove duplicated entries in auto-generated tmpfiles.d, // which are already in system tmpfiles - for key in system_tmpfiles_entries.into_iter() { + for (key, _) in system_tmpfiles_entries { rpmostree_tmpfiles_entries.retain(|k, _value| k != &key); }