From 3e839cb5ca531334674949e5362bf1fa2762720c Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Thu, 29 Oct 2026 22:52:08 +0100 Subject: [PATCH] refactor(rootfs): split the tarball packer into pack.rs rootfs.rs mixed the cache lifecycle with the tree-to-tarball writer that persisting relies on. The packer moves to pack.rs beside extract.rs: pack_rootfs becomes crate-internal, its partial-cleanup test moves with it. No behavior change. --- AGENTS.md | 1 + crates/ecrou/src/lib.rs | 1 + crates/ecrou/src/pack.rs | 125 +++++++++++++++++++++++++++++++++++++ crates/ecrou/src/rootfs.rs | 119 +---------------------------------- 4 files changed, 130 insertions(+), 116 deletions(-) create mode 100644 crates/ecrou/src/pack.rs diff --git a/AGENTS.md b/AGENTS.md index 485619b..a689c23 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,6 +35,7 @@ Rules: - `guest` - the guest init script (`crates/ecrou/src/guest.rs`) - `rootfs` - cache-aware preparation and persistence (`crates/ecrou/src/rootfs.rs`) + - `pack` - rootfs tarball packing (`crates/ecrou/src/pack.rs`) - `distro` - distro definitions and mirrors (`crates/ecrou/src/distro.rs`) - `download` - image download (`crates/ecrou/src/download.rs`) - `extract` - rootfs extraction (`crates/ecrou/src/extract.rs`) diff --git a/crates/ecrou/src/lib.rs b/crates/ecrou/src/lib.rs index 1b74a4b..799ca03 100644 --- a/crates/ecrou/src/lib.rs +++ b/crates/ecrou/src/lib.rs @@ -37,6 +37,7 @@ pub mod error; pub mod extract; pub mod guest; pub mod krun; +mod pack; pub mod progress; mod registry; pub mod rootfs; diff --git a/crates/ecrou/src/pack.rs b/crates/ecrou/src/pack.rs new file mode 100644 index 0000000..1d19817 --- /dev/null +++ b/crates/ecrou/src/pack.rs @@ -0,0 +1,125 @@ +//! Packing a rootfs tree into the compressed tarball a cache entry +//! stores, so a provisioned tree persists and later boots skip the +//! download. The write lands in a unique partial file and renames +//! atomically; compression follows the destination's extension. + +use anyhow::{anyhow, Context}; +use std::path::Path; + +/// Write a tarball of `rootfs` at `dest`, compressed according to dest's +/// extension (.tar.gz, .tar.xz, .tar.zst, plain .tar), atomically via a +/// unique partial file ([`crate::utils::partial_path`]). +pub(crate) fn pack_rootfs(rootfs: &Path, dest: &Path) -> anyhow::Result<()> { + // Packing the tree into a tarball placed inside it would recurse + // forever: the walker keeps seeing the archive grow. + let rootfs_canonical = rootfs + .canonicalize() + .with_context(|| format!("Failed to resolve rootfs directory {}", rootfs.display()))?; + if let Some(parent) = dest.parent() { + if let Ok(parent_canonical) = parent.canonicalize() { + if parent_canonical.starts_with(&rootfs_canonical) { + return Err(anyhow!( + "Cache entry {} must live outside the rootfs", + dest.display() + )); + } + } + } + + let filename = dest.file_name().and_then(|n| n.to_str()).unwrap_or(""); + let partial = crate::utils::partial_path(dest); + + let file = std::fs::File::create(&partial) + .with_context(|| format!("Failed to create {}", partial.display()))?; + + let result: anyhow::Result<()> = (|| { + if filename.ends_with(".tar.gz") || filename.ends_with(".tgz") { + let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::default()); + let encoder = write_rootfs_tar(rootfs, encoder)?; + encoder.finish().context("Failed to finalise gzip stream")?; + } else if filename.ends_with(".tar.xz") || filename.ends_with(".txz") { + let encoder = xz2::write::XzEncoder::new(file, 6); + let encoder = write_rootfs_tar(rootfs, encoder)?; + encoder.finish().context("Failed to finalise xz stream")?; + } else if filename.ends_with(".tar.zst") || filename.ends_with(".tar.zstd") { + let encoder = zstd::stream::write::Encoder::new(file, 0) + .context("Failed to create zstd encoder")?; + let encoder = write_rootfs_tar(rootfs, encoder)?; + encoder.finish().context("Failed to finalise zstd stream")?; + } else { + write_rootfs_tar(rootfs, file)?; + } + Ok(()) + })(); + + if result.is_err() { + // Best-effort cleanup; ignore errors (file may not exist if creation failed). + let _ = std::fs::remove_file(&partial); + return result; + } + + std::fs::rename(&partial, dest) + .with_context(|| format!("Failed to move packed rootfs to {}", dest.display())) +} + +/// Write the rootfs tree into a tar archive streamed to `writer`, returning +/// the writer so the caller can finalise the compression stream. +fn write_rootfs_tar(rootfs: &Path, writer: W) -> anyhow::Result { + let mut builder = tar::Builder::new(writer); + // Store symlinks as symlinks; following them would duplicate the target's + // contents into the archive (and recurse into symlinked directories). + builder.follow_symlinks(false); + + let entries = std::fs::read_dir(rootfs) + .with_context(|| format!("Failed to read {}", rootfs.display()))?; + + for entry in entries { + let entry = + entry.with_context(|| format!("Failed to read entry in {}", rootfs.display()))?; + let path = entry.path(); + let name = entry.file_name(); + + // symlink_metadata: never follow symlinks — a top-level symlink to a + // directory must be stored as a link, not recursed into. + let meta = std::fs::symlink_metadata(&path) + .with_context(|| format!("Failed to stat {}", path.display()))?; + + if meta.is_dir() { + builder + .append_dir_all(&name, &path) + .with_context(|| format!("Failed to archive {}", path.display()))?; + } else { + builder + .append_path_with_name(&path, &name) + .with_context(|| format!("Failed to archive {}", path.display()))?; + } + } + + builder + .into_inner() + .context("Failed to finalise tar archive") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pack_failure_leaves_no_partial_file() { + // A nonexistent rootfs must fail cleanly and clean up its + // partial file, whatever unique name it carried. + let cache = tempfile::tempdir().unwrap(); + let dest = cache.path().join("x.tar.gz"); + assert!(pack_rootfs(Path::new("/nonexistent-rootfs"), &dest).is_err()); + assert!(!dest.exists()); + let leftovers: Vec = std::fs::read_dir(cache.path()) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.contains("partial")) + .collect(); + assert!( + leftovers.is_empty(), + "partial files left behind: {leftovers:?}" + ); + } +} diff --git a/crates/ecrou/src/rootfs.rs b/crates/ecrou/src/rootfs.rs index b67ef6a..9d1be74 100644 --- a/crates/ecrou/src/rootfs.rs +++ b/crates/ecrou/src/rootfs.rs @@ -385,7 +385,7 @@ impl PreparedRootfs { /// layer lands under its own key while the prepared scratch still /// carries the base's own cache path. pub fn persist_to(&self, cache_path: &Path) -> Result<()> { - pack_rootfs(self.dir(), cache_path)?; + crate::pack::pack_rootfs(self.dir(), cache_path)?; std::fs::write(sidecar(cache_path, PROVISIONED_SUFFIX), b"").with_context(|| { format!( "Failed to write provisioned marker for {}", @@ -501,100 +501,6 @@ fn drop_loopback_nameservers(config: &str) -> String { }) } -/// Write a tarball of `rootfs` at `dest`, compressed according to dest's -/// extension (.tar.gz, .tar.xz, .tar.zst, plain .tar), atomically via a -/// unique partial file ([`crate::utils::partial_path`]). -fn pack_rootfs(rootfs: &Path, dest: &Path) -> anyhow::Result<()> { - // Packing the tree into a tarball placed inside it would recurse - // forever: the walker keeps seeing the archive grow. - let rootfs_canonical = rootfs - .canonicalize() - .with_context(|| format!("Failed to resolve rootfs directory {}", rootfs.display()))?; - if let Some(parent) = dest.parent() { - if let Ok(parent_canonical) = parent.canonicalize() { - if parent_canonical.starts_with(&rootfs_canonical) { - return Err(anyhow!( - "Cache entry {} must live outside the rootfs", - dest.display() - )); - } - } - } - - let filename = dest.file_name().and_then(|n| n.to_str()).unwrap_or(""); - let partial = crate::utils::partial_path(dest); - - let file = std::fs::File::create(&partial) - .with_context(|| format!("Failed to create {}", partial.display()))?; - - let result: anyhow::Result<()> = (|| { - if filename.ends_with(".tar.gz") || filename.ends_with(".tgz") { - let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::default()); - let encoder = write_rootfs_tar(rootfs, encoder)?; - encoder.finish().context("Failed to finalise gzip stream")?; - } else if filename.ends_with(".tar.xz") || filename.ends_with(".txz") { - let encoder = xz2::write::XzEncoder::new(file, 6); - let encoder = write_rootfs_tar(rootfs, encoder)?; - encoder.finish().context("Failed to finalise xz stream")?; - } else if filename.ends_with(".tar.zst") || filename.ends_with(".tar.zstd") { - let encoder = zstd::stream::write::Encoder::new(file, 0) - .context("Failed to create zstd encoder")?; - let encoder = write_rootfs_tar(rootfs, encoder)?; - encoder.finish().context("Failed to finalise zstd stream")?; - } else { - write_rootfs_tar(rootfs, file)?; - } - Ok(()) - })(); - - if result.is_err() { - // Best-effort cleanup; ignore errors (file may not exist if creation failed). - let _ = std::fs::remove_file(&partial); - return result; - } - - std::fs::rename(&partial, dest) - .with_context(|| format!("Failed to move packed rootfs to {}", dest.display())) -} - -/// Write the rootfs tree into a tar archive streamed to `writer`, returning -/// the writer so the caller can finalise the compression stream. -fn write_rootfs_tar(rootfs: &Path, writer: W) -> anyhow::Result { - let mut builder = tar::Builder::new(writer); - // Store symlinks as symlinks; following them would duplicate the target's - // contents into the archive (and recurse into symlinked directories). - builder.follow_symlinks(false); - - let entries = std::fs::read_dir(rootfs) - .with_context(|| format!("Failed to read {}", rootfs.display()))?; - - for entry in entries { - let entry = - entry.with_context(|| format!("Failed to read entry in {}", rootfs.display()))?; - let path = entry.path(); - let name = entry.file_name(); - - // symlink_metadata: never follow symlinks — a top-level symlink to a - // directory must be stored as a link, not recursed into. - let meta = std::fs::symlink_metadata(&path) - .with_context(|| format!("Failed to stat {}", path.display()))?; - - if meta.is_dir() { - builder - .append_dir_all(&name, &path) - .with_context(|| format!("Failed to archive {}", path.display()))?; - } else { - builder - .append_path_with_name(&path, &name) - .with_context(|| format!("Failed to archive {}", path.display()))?; - } - } - - builder - .into_inner() - .context("Failed to finalise tar archive") -} - /// Generate a cache filename based on the image source fn generate_cache_filename(source: &ImageSource, arch: &str) -> String { match source { @@ -763,7 +669,7 @@ mod tests { let cache = tempfile::tempdir().unwrap(); let dest = cache.path().join(dest_name); - pack_rootfs(src.path(), &dest).unwrap(); + crate::pack::pack_rootfs(src.path(), &dest).unwrap(); let out = tempfile::tempdir().unwrap(); crate::extract::extract_tarball(&dest, out.path(), &NoProgress).unwrap(); @@ -826,25 +732,6 @@ mod tests { assert_eq!(std::fs::read(out.path().join("file.txt")).unwrap(), b"data"); } - #[test] - fn pack_failure_leaves_no_partial_file() { - // A nonexistent rootfs must fail cleanly and clean up its - // partial file, whatever unique name it carried. - let cache = tempfile::tempdir().unwrap(); - let dest = cache.path().join("x.tar.gz"); - assert!(pack_rootfs(Path::new("/nonexistent-rootfs"), &dest).is_err()); - assert!(!dest.exists()); - let leftovers: Vec = std::fs::read_dir(cache.path()) - .unwrap() - .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) - .filter(|name| name.contains("partial")) - .collect(); - assert!( - leftovers.is_empty(), - "partial files left behind: {leftovers:?}" - ); - } - #[test] fn a_local_archive_prepares_without_touching_the_cache() { // A rootfs tree, packed next to itself as ./rootfs.tar.gz, the @@ -854,7 +741,7 @@ mod tests { std::fs::create_dir_all(&tree).unwrap(); std::fs::write(tree.join("etc-marker"), b"local").unwrap(); let tarball = work.path().join("rootfs.tar.gz"); - pack_rootfs(&tree, &tarball).unwrap(); + crate::pack::pack_rootfs(&tree, &tarball).unwrap(); let cache_dir = tempfile::tempdir().unwrap(); let cache = RootfsCache::new(cache_dir.path().to_path_buf());