copy-to-storage: Refactor tar parsing

Inline the tar parsing/unpacking
Check for two NULL 512 blocks instead of just one
Share source image and target image generating code between composefs
and ostree

Signed-off-by: Pragyan Poudyal <[email protected]>

Author: Johan-Liebert1

crates/lib/src/bootc_composefs/export.rs

@@ -1,57 +1,22 @@
-use std::{fs::File, io::Read, os::fd::AsRawFd};
+use std::{fs::File, os::fd::AsRawFd};
 
 use anyhow::{Context, Result};
 use cap_std_ext::cap_std::{ambient_authority, fs::Dir};
-use composefs::{
-    fsverity::FsVerityHashValue,
-    splitstream::{SplitStreamData, SplitStreamReader},
-    tree::{LeafContent, RegularFile},
-};
-use composefs_oci::tar::TarItem;
+use composefs::splitstream::SplitStreamData;
 use ocidir::{oci_spec::image::Platform, OciDir};
 use ostree_ext::container::skopeo;
 use ostree_ext::{container::Transport, oci_spec::image::ImageConfiguration};
-use tar::{EntryType, Header};
+use tar::EntryType;
 
+use crate::image::get_imgrefs_for_copy;
 use crate::{
     bootc_composefs::{
         status::{get_composefs_status, get_imginfo},
         update::str_to_sha256digest,
     },
-    image::IMAGE_DEFAULT,
     store::{BootedComposefs, Storage},
 };
 
-fn get_entry_with_header<R: Read, ObjectID: FsVerityHashValue>(
-    reader: &mut SplitStreamReader<R, ObjectID>,
-) -> anyhow::Result<Option<(Header, TarItem<ObjectID>)>> {
-    let mut buf = [0u8; 512];
-    if !reader.read_inline_exact(&mut buf)? || buf == [0u8; 512] {
-        return Ok(None);
-    }
-
-    let header = tar::Header::from_byte_slice(&buf);
-
-    let size = header.entry_size()?;
-
-    let item = match reader.read_exact(size as usize, ((size + 511) & !511) as usize)? {
-        SplitStreamData::External(id) => match header.entry_type() {
-            EntryType::Regular | EntryType::Continuous => {
-                TarItem::Leaf(LeafContent::Regular(RegularFile::External(id, size)))
-            }
-            _ => anyhow::bail!("Unsupported external-chunked entry {header:?} {id:?}"),
-        },
-
-        SplitStreamData::Inline(content) => match header.entry_type() {
-            EntryType::Directory => TarItem::Directory,
-            // We do not care what the content is as we're re-archiving it anyway
-            _ => TarItem::Leaf(LeafContent::Regular(RegularFile::Inline(content))),
-        },
-    };
-
-    return Ok(Some((header.clone(), item)));
-}
-
 /// Exports a composefs repository to a container image in containers-storage:
 pub async fn export_repo_to_image(
     storage: &Storage,
@@ -61,37 +26,7 @@ pub async fn export_repo_to_image(
 ) -> Result<()> {
     let host = get_composefs_status(storage, booted_cfs).await?;
 
-    let booted_image = host
-        .status
-        .booted
-        .as_ref()
-        .ok_or_else(|| anyhow::anyhow!("Booted deployment not found"))?
-        .image
-        .as_ref()
-        .unwrap();
-
-    // If the target isn't specified, push to containers-storage + our default image
-    let dest_imgref = match target {
-        Some(target) => ostree_ext::container::ImageReference {
-            transport: Transport::ContainerStorage,
-            name: target.to_owned(),
-        },
-        None => ostree_ext::container::ImageReference {
-            transport: Transport::ContainerStorage,
-            name: IMAGE_DEFAULT.into(),
-        },
-    };
-
-    // If the source isn't specified, we use the booted image
-    let source = match source {
-        Some(source) => ostree_ext::container::ImageReference::try_from(source)
-            .context("Parsing source image")?,
-
-        None => ostree_ext::container::ImageReference {
-            transport: Transport::try_from(booted_image.image.transport.as_str()).unwrap(),
-            name: booted_image.image.image.clone(),
-        },
-    };
+    let (source, dest_imgref) = get_imgrefs_for_copy(source, target).await?;
 
     let mut depl_verity = None;
 
@@ -144,6 +79,8 @@ pub async fn export_repo_to_image(
     let mut new_manifest = imginfo.manifest.clone();
     new_manifest.layers_mut().clear();
 
+    let total_layers = config.rootfs().diff_ids().len();
+
     for (idx, old_diff_id) in config.rootfs().diff_ids().iter().enumerate() {
         let layer_sha256 = str_to_sha256digest(old_diff_id)?;
         let layer_verity = config_stream.lookup(&layer_sha256)?;
@@ -155,37 +92,60 @@ pub async fn export_repo_to_image(
         let mut layer_writer = oci_dir.create_layer(None)?;
         layer_writer.follow_symlinks(false);
 
-        while let Some((header, entry)) = get_entry_with_header(&mut layer_stream)? {
-            let hsize = header.size()? as usize;
-            let mut v = vec![0; hsize];
-
-            match &entry {
-                TarItem::Leaf(leaf_content) => {
-                    match &leaf_content {
-                        LeafContent::Regular(reg) => match reg {
-                            RegularFile::Inline(items) => {
-                                v[..hsize].copy_from_slice(items);
-                            }
-
-                            RegularFile::External(obj_id, ..) => {
-                                let mut file = File::from(booted_cfs.repo.open_object(obj_id)?);
-                                file.read_exact(&mut v)?;
-                            }
-                        },
-
-                        // we don't need to write the data for symlinks.
-                        // Same goes for devices, fifos and sockets
-                        _ => {}
+        let mut got_zero_block = false;
+
+        loop {
+            let mut buf = [0u8; 512];
+
+            if !layer_stream
+                .read_inline_exact(&mut buf)
+                .context("Reading into buffer")?
+            {
+                break;
+            }
+
+            let all_zeroes = buf.iter().all(|x| *x == 0);
+
+            // EOF for tar
+            if all_zeroes && got_zero_block {
+                break;
+            } else if all_zeroes {
+                got_zero_block = true;
+                continue;
+            }
+
+            got_zero_block = false;
+
+            let header = tar::Header::from_byte_slice(&buf);
+
+            let size = header.entry_size()?;
+
+            match layer_stream.read_exact(size as usize, ((size + 511) & !511) as usize)? {
+                SplitStreamData::External(obj_id) => match header.entry_type() {
+                    EntryType::Regular | EntryType::Continuous => {
+                        let file = File::from(booted_cfs.repo.open_object(&obj_id)?);
+
+                        layer_writer
+                            .append(&header, file)
+                            .context("Failed to write external entry")?;
                     }
-                }
 
-                // we don't need to write the data for hardlinks/dirs
-                TarItem::Directory | TarItem::Hardlink(..) => {}
-            };
+                    _ => anyhow::bail!("Unsupported external-chunked entry {header:?} {obj_id:?}"),
+                },
+
+                SplitStreamData::Inline(content) => match header.entry_type() {
+                    EntryType::Directory => {
+                        layer_writer.append(&header, std::io::empty())?;
+                    }
 
-            layer_writer
-                .append(&header, v.as_slice())
-                .context("Failed to write entry")?;
+                    // We do not care what the content is as we're re-archiving it anyway
+                    _ => {
+                        layer_writer
+                            .append(&header, &*content)
+                            .context("Failed to write inline entry")?;
+                    }
+                },
+            };
         }
 
         layer_writer.finish()?;
@@ -196,7 +156,11 @@ pub async fn export_repo_to_image(
             .complete()
             .context("Writing layer to disk")?;
 
-        tracing::debug!("Wrote layer: {}", layer.uncompressed_sha256_as_digest());
+        tracing::debug!(
+            "Wrote layer: {layer_sha} #{layer_num}/{total_layers}",
+            layer_sha = layer.uncompressed_sha256_as_digest(),
+            layer_num = idx + 1,
+        );
 
         let previous_annotations = imginfo
             .manifest

crates/lib/src/cli.rs

@@ -1592,7 +1592,12 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
 
                 match storage.kind()? {
                     BootedStorageKind::Ostree(..) => {
-                        crate::image::push_entrypoint(source.as_deref(), target.as_deref()).await
+                        crate::image::push_entrypoint(
+                            &storage,
+                            source.as_deref(),
+                            target.as_deref(),
+                        )
+                        .await
                     }
                     BootedStorageKind::Composefs(booted) => {
                         bootc_composefs::export::export_repo_to_image(

crates/lib/src/image.rs

@@ -15,6 +15,8 @@ use crate::{
     boundimage::query_bound_images,
     cli::{ImageListFormat, ImageListType},
     podstorage::CStorage,
+    status::get_host,
+    store::Storage,
     utils::async_task_with_spinner,
 };
 
@@ -139,43 +141,65 @@ pub(crate) async fn list_entrypoint(
     Ok(())
 }
 
-/// Implementation of `bootc image push-to-storage`.
-#[context("Pushing image")]
-pub(crate) async fn push_entrypoint(source: Option<&str>, target: Option<&str>) -> Result<()> {
+/// Returns the source and target ImageReference
+/// If the source isn't specified, we use booted image
+/// If the target isn't specified, we push to containers-storage with our default image
+pub(crate) async fn get_imgrefs_for_copy(
+    source: Option<&str>,
+    target: Option<&str>,
+) -> Result<(ImageReference, ImageReference)> {
     // Initialize floating c_storage early - needed for container operations
     crate::podstorage::ensure_floating_c_storage_initialized();
 
-    let transport = Transport::ContainerStorage;
-    let sysroot = crate::cli::get_storage().await?;
-    let ostree = sysroot.get_ostree()?;
-    let repo = &ostree.repo();
-
     // If the target isn't specified, push to containers-storage + our default image
-    let target = if let Some(target) = target {
-        ImageReference {
-            transport,
+    let dest_imgref = match target {
+        Some(target) => ostree_ext::container::ImageReference {
+            transport: Transport::ContainerStorage,
             name: target.to_owned(),
-        }
-    } else {
-        ImageReference {
+        },
+        None => ostree_ext::container::ImageReference {
             transport: Transport::ContainerStorage,
-            name: IMAGE_DEFAULT.to_string(),
-        }
+            name: IMAGE_DEFAULT.into(),
+        },
     };
 
     // If the source isn't specified, we use the booted image
-    let source = if let Some(source) = source {
-        ImageReference::try_from(source).context("Parsing source image")?
-    } else {
-        let status = crate::status::get_status_require_booted(&ostree)?;
-        // SAFETY: We know it's booted
-        let booted = status.2.status.booted.unwrap();
-        let booted_image = booted.image.unwrap().image;
-        ImageReference {
-            transport: Transport::try_from(booted_image.transport.as_str()).unwrap(),
-            name: booted_image.image,
+    let src_imgref = match source {
+        Some(source) => ostree_ext::container::ImageReference::try_from(source)
+            .context("Parsing source image")?,
+
+        None => {
+            let host = get_host().await?;
+
+            let booted = host
+                .status
+                .booted
+                .ok_or_else(|| anyhow::anyhow!("Booted deployment not found"))?;
+
+            let booted_image = booted.image.unwrap().image;
+
+            ImageReference {
+                transport: Transport::try_from(booted_image.transport.as_str()).unwrap(),
+                name: booted_image.image,
+            }
         }
     };
+
+    return Ok((src_imgref, dest_imgref));
+}
+
+/// Implementation of `bootc image push-to-storage`.
+#[context("Pushing image")]
+pub(crate) async fn push_entrypoint(
+    storage: &Storage,
+    source: Option<&str>,
+    target: Option<&str>,
+) -> Result<()> {
+    let (source, target) = get_imgrefs_for_copy(source, target).await?;
+
+    let ostree = storage.get_ostree()?;
+    let repo = &ostree.repo();
+
     let mut opts = ostree_ext::container::store::ExportToOCIOpts::default();
     opts.progress_to_stdout = true;
     println!("Copying local image {source} to {target} ...");

crates/lib/src/status.rs

@@ -430,7 +430,7 @@ pub(crate) fn get_status(
     Ok((deployments, host))
 }
 
-async fn get_host() -> Result<Host> {
+pub(crate) async fn get_host() -> Result<Host> {
     let env = crate::store::Environment::detect()?;
     if env.needs_mount_namespace() {
         crate::cli::prepare_for_write()?;