composefs/bls: Use custom prefix for BLS binaris dir

Until now we were using only the deployment verity for the name of the
directory where we store BLS Type1 boot binary (vmlinuz + initrd) which
made it ambiguous when trying to seach for them while GC-ing.

Introduce a custom prefix `bootc_composefs-` to all such directories
which makes it easier for us to find them

Also, update test to reflect this change

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

composefs/gc: Refactor

Remove `println!` from Non-CLI code and move it to the CLI

Remove `dry_run` argument from DeleteDeployment because we delete the
image anyway so it isn't really a "dry run"

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

Author: Johan-Liebert1

crates/lib/src/bootc_composefs/boot.rs

@@ -91,7 +91,6 @@ use rustix::{mount::MountFlags, path::Arg};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
-use crate::bootc_composefs::state::{get_booted_bls, write_composefs_state};
 use crate::parsers::bls_config::{BLSConfig, BLSConfigType};
 use crate::task::Task;
 use crate::{
@@ -102,6 +101,10 @@ use crate::{
     bootc_composefs::repo::open_composefs_repo,
     store::{ComposefsFilesystem, Storage},
 };
+use crate::{
+    bootc_composefs::state::{get_booted_bls, write_composefs_state},
+    composefs_consts::TYPE1_BOOT_DIR_PREFIX,
+};
 use crate::{
     bootc_composefs::status::get_container_manifest_and_config, bootc_kargs::compute_new_kargs,
 };
@@ -270,6 +273,11 @@ pub(crate) fn secondary_sort_key(os_id: &str) -> String {
     format!("bootc-{os_id}-{SORTKEY_PRIORITY_SECONDARY}")
 }
 
+/// Returns the name of the directory where we store Type1 boot entries
+pub(crate) fn get_type1_dir_name(depl_verity: &String) -> String {
+    format!("{TYPE1_BOOT_DIR_PREFIX}{depl_verity}")
+}
+
 /// Compute SHA256Sum of VMlinuz + Initrd
 ///
 /// # Arguments
@@ -381,10 +389,10 @@ fn write_bls_boot_entries_to_disk(
     entry: &UsrLibModulesVmlinuz<Sha512HashValue>,
     repo: &crate::store::ComposefsRepository,
 ) -> Result<()> {
-    let id_hex = deployment_id.to_hex();
+    let dir_name = get_type1_dir_name(&deployment_id.to_hex());
 
-    // Write the initrd and vmlinuz at /boot/<id>/
-    let path = boot_dir.join(&id_hex);
+    // Write the initrd and vmlinuz at /boot/composefs-<id>/
+    let path = boot_dir.join(&dir_name);
     create_dir_all(&path)?;
 
     let entries_dir = Dir::open_ambient_dir(&path, ambient_authority())
@@ -496,6 +504,7 @@ pub(crate) fn setup_composefs_bls_boot(
 
             cmdline_options.extend(&root_setup.kargs);
 
+            // TODO(Johan-Liebert1): Use ComposefsCmdline
             let composefs_cmdline = if state.composefs_options.allow_missing_verity {
                 format!("{COMPOSEFS_CMDLINE}=?{id_hex}")
             } else {
@@ -648,13 +657,18 @@ pub(crate) fn setup_composefs_bls_boot(
 
             let mut bls_config = BLSConfig::default();
 
+            let entries_dir = get_type1_dir_name(&id_hex);
+
             bls_config
                 .with_title(title)
                 .with_version(version)
                 .with_sort_key(sort_key)
                 .with_cfg(BLSConfigType::NonEFI {
-                    linux: entry_paths.abs_entries_path.join(&id_hex).join(VMLINUZ),
-                    initrd: vec![entry_paths.abs_entries_path.join(&id_hex).join(INITRD)],
+                    linux: entry_paths
+                        .abs_entries_path
+                        .join(&entries_dir)
+                        .join(VMLINUZ),
+                    initrd: vec![entry_paths.abs_entries_path.join(&entries_dir).join(INITRD)],
                     options: Some(cmdline_refs),
                 });
 
@@ -679,7 +693,16 @@ pub(crate) fn setup_composefs_bls_boot(
                         // We shouldn't error here as all our file names are UTF-8 compatible
                         let ent_name = ent.file_name()?;
 
-                        if shared_entries.contains(&ent_name) {
+                        let Some(entry_verity_part) = ent_name.strip_prefix(TYPE1_BOOT_DIR_PREFIX)
+                        else {
+                            // Not our directory
+                            continue;
+                        };
+
+                        if shared_entries
+                            .iter()
+                            .any(|shared_ent| shared_ent == entry_verity_part)
+                        {
                             shared_entry = Some(ent_name);
                             break;
                         }

crates/lib/src/bootc_composefs/delete.rs

@@ -221,7 +221,6 @@ pub(crate) async fn delete_composefs_deployment(
     deployment_id: &str,
     storage: &Storage,
     booted_cfs: &BootedComposefs,
-    dry_run: bool,
 ) -> Result<()> {
     const COMPOSEFS_DELETE_JOURNAL_ID: &str = "2a1f0e9d8c7b6a5f4e3d2c1b0a9f8e7d6";
 
@@ -276,7 +275,7 @@ pub(crate) async fn delete_composefs_deployment(
 
     delete_depl_boot_entries(&depl_to_del, &storage, deleting_staged)?;
 
-    composefs_gc(storage, booted_cfs, dry_run).await?;
+    composefs_gc(storage, booted_cfs, true).await?;
 
     Ok(())
 }

crates/lib/src/bootc_composefs/gc.rs

@@ -4,20 +4,19 @@
 //! - We delete the bootloader entry but fail to delete image
 //! - We delete bootloader + image but fail to delete the state/unrefenced objects etc
 
-use std::os::fd::AsRawFd;
-
 use anyhow::{Context, Result};
 use cap_std_ext::{cap_std::fs::Dir, dirext::CapStdExtDirExt};
+use composefs::repository::GcResult;
 use composefs_boot::bootloader::{EFI_ADDON_DIR_EXT, EFI_EXT};
-use rustix::fs::readlink;
 
 use crate::{
+    bootc_composefs::boot::get_type1_dir_name,
     bootc_composefs::{
-        boot::{BOOTC_UKI_DIR, BootType, VMLINUZ},
+        boot::{BOOTC_UKI_DIR, BootType},
         delete::{delete_image, delete_staged, delete_state_dir},
         status::{get_composefs_status, get_imginfo, list_bootloader_entries},
     },
-    composefs_consts::STATE_DIR_RELATIVE,
+    composefs_consts::{STATE_DIR_RELATIVE, TYPE1_BOOT_DIR_PREFIX},
     store::{BootedComposefs, Storage},
 };
 
@@ -63,7 +62,7 @@ type BootBinary = (BootType, String);
 
 /// Collect all BLS Type1 boot binaries and UKI binaries by scanning filesystem
 ///
-/// Returns a vector of binary type (UKI/Type1) + verity digest of all boot binaries
+/// Returns a vector of binary type (UKI/Type1) + name of all boot binaries
 #[fn_error_context::context("Collecting boot binaries")]
 fn collect_boot_binaries(storage: &Storage) -> Result<Vec<BootBinary>> {
     let mut boot_binaries = Vec::new();
@@ -99,62 +98,44 @@ fn collect_uki_binaries(boot_dir: &Dir, boot_binaries: &mut Vec<BootBinary>) ->
     Ok(())
 }
 
-/// Scan for Type1 boot binaries (kernels + initrds) by looking for directories with verity hashes
+/// Scan for Type1 boot binaries (kernels + initrds) by looking for directories with
+/// that start with bootc_composefs-
+///
+/// Strips the prefix and returns the rest of the string
 #[fn_error_context::context("Collecting Type1 boot binaries")]
 fn collect_type1_boot_binaries(boot_dir: &Dir, boot_binaries: &mut Vec<BootBinary>) -> Result<()> {
     for entry in boot_dir.entries_utf8()? {
         let entry = entry?;
-        let dir_path = entry.file_name()?;
+        let dir_name = entry.file_name()?;
 
         if !entry.file_type()?.is_dir() {
             continue;
         }
 
-        // Check if the directory name looks like a verity hash
-        // 128 hex chars for SHA512, 64 hex chars for SHA256
-        // TODO: There might be a better way to do this
-        let valid_len = matches!(dir_path.len(), 64 | 128);
-        let valid_hex = dir_path.bytes().all(|b| b.is_ascii_hexdigit());
-
-        if !(valid_len && valid_hex) {
+        let Some(verity) = dir_name.strip_prefix(TYPE1_BOOT_DIR_PREFIX) else {
             continue;
-        }
-
-        let verity_dir = boot_dir
-            .open_dir(&dir_path)
-            .with_context(|| format!("Opening {dir_path}"))?;
+        };
 
-        // Scan inside this directory for kernel and initrd files
-        for file_entry in verity_dir.entries_utf8()? {
-            let file_entry = file_entry?;
-            let file_name = file_entry.file_name()?;
-
-            // Look for kernel and initrd files
-            if file_name.starts_with(VMLINUZ) {
-                boot_binaries.push((BootType::Bls, dir_path.clone()));
-            }
-        }
+        // The directory name starts with our custom prefix
+        boot_binaries.push((BootType::Bls, verity.to_string()));
     }
 
     Ok(())
 }
 
 #[fn_error_context::context("Deleting kernel and initrd")]
-fn delete_kernel_initrd(storage: &Storage, entry_to_delete: &str, dry_run: bool) -> Result<()> {
+fn delete_kernel_initrd(storage: &Storage, dir_to_delete: &str, dry_run: bool) -> Result<()> {
     let boot_dir = storage.require_boot_dir()?;
 
-    let path = readlink(format!("/proc/self/fd/{}", boot_dir.as_raw_fd()), [])
-        .context("Getting kernel path")?;
-
-    tracing::debug!("Deleting {path:?}/{entry_to_delete}");
+    tracing::debug!("Deleting Type1 entry {dir_to_delete}");
 
     if dry_run {
         return Ok(());
     }
 
     boot_dir
-        .remove_dir_all(entry_to_delete)
-        .with_context(|| anyhow::anyhow!("Deleting {entry_to_delete}"))
+        .remove_dir_all(dir_to_delete)
+        .with_context(|| anyhow::anyhow!("Deleting {dir_to_delete}"))
 }
 
 /// Deletes the UKI `uki_id` and any addons specific to it
@@ -217,7 +198,7 @@ pub(crate) async fn composefs_gc(
     storage: &Storage,
     booted_cfs: &BootedComposefs,
     dry_run: bool,
-) -> Result<()> {
+) -> Result<GcResult> {
     const COMPOSEFS_GC_JOURNAL_ID: &str = "3b2a1f0e9d8c7b6a5f4e3d2c1b0a9f8e7";
 
     tracing::info!(
@@ -238,7 +219,7 @@ pub(crate) async fn composefs_gc(
     tracing::debug!("bootloader_entries: {bootloader_entries:?}");
 
     // Bootloader entry is deleted, but the binary (UKI/kernel+initrd) still exists
-    let unrefenced_boot_binaries = boot_binaries
+    let unreferenced_boot_binaries = boot_binaries
         .iter()
         .filter(|bin_path| {
             // We reuse kernel + initrd if they're the same for two deployments
@@ -248,13 +229,13 @@ pub(crate) async fn composefs_gc(
             // filter the ones that are not referenced by any bootloader entry
             !bootloader_entries
                 .iter()
-                .any(|boot_entry| bin_path.1.contains(boot_entry))
+                .any(|boot_entry| bin_path.1 == *boot_entry)
         })
         .collect::<Vec<_>>();
 
-    tracing::debug!("unrefenced_boot_binaries: {unrefenced_boot_binaries:?}");
+    tracing::debug!("unreferenced_boot_binaries: {unreferenced_boot_binaries:?}");
 
-    if unrefenced_boot_binaries
+    if unreferenced_boot_binaries
         .iter()
         .find(|be| be.1 == booted_cfs_status.verity)
         .is_some()
@@ -265,9 +246,9 @@ pub(crate) async fn composefs_gc(
         )
     }
 
-    for (ty, verity) in unrefenced_boot_binaries {
+    for (ty, verity) in unreferenced_boot_binaries {
         match ty {
-            BootType::Bls => delete_kernel_initrd(storage, verity, dry_run)?,
+            BootType::Bls => delete_kernel_initrd(storage, &get_type1_dir_name(verity), dry_run)?,
             BootType::Uki => delete_uki(storage, verity, dry_run)?,
         }
     }
@@ -345,21 +326,5 @@ pub(crate) async fn composefs_gc(
         booted_cfs.repo.gc(&additional_roots)?
     };
 
-    if dry_run {
-        println!("Dry run (no files deleted)");
-    }
-
-    println!(
-        "Objects: {} removed ({} bytes)",
-        gc_result.objects_removed, gc_result.objects_bytes
-    );
-
-    if gc_result.images_pruned > 0 || gc_result.streams_pruned > 0 {
-        println!(
-            "Pruned symlinks: {} images, {} streams",
-            gc_result.images_pruned, gc_result.streams_pruned
-        );
-    }
-
-    Ok(())
+    Ok(gc_result)
 }

crates/lib/src/cli.rs

@@ -793,8 +793,6 @@ pub(crate) enum Opt {
     #[clap(hide = true)]
     DeleteDeployment {
         depl_id: String,
-        #[clap(long)]
-        dry_run: bool,
     },
 }
 
@@ -1917,7 +1915,25 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
                     }
 
                     BootedStorageKind::Composefs(booted_cfs) => {
-                        composefs_gc(storage, &booted_cfs, dry_run).await
+                        let gc_result = composefs_gc(storage, &booted_cfs, dry_run).await?;
+
+                        if dry_run {
+                            println!("Dry run (no files deleted)");
+                        }
+
+                        println!(
+                            "Objects: {} removed ({} bytes)",
+                            gc_result.objects_removed, gc_result.objects_bytes
+                        );
+
+                        if gc_result.images_pruned > 0 || gc_result.streams_pruned > 0 {
+                            println!(
+                                "Pruned symlinks: {} images, {} streams",
+                                gc_result.images_pruned, gc_result.streams_pruned
+                            );
+                        }
+
+                        Ok(())
                     }
                 }
             }
@@ -1955,14 +1971,14 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
             }
         }
 
-        Opt::DeleteDeployment { depl_id, dry_run } => {
+        Opt::DeleteDeployment { depl_id } => {
             let storage = &get_storage().await?;
             match storage.kind()? {
                 BootedStorageKind::Ostree(_) => {
                     anyhow::bail!("DeleteDeployment is only supported for composefs backend")
                 }
                 BootedStorageKind::Composefs(booted_cfs) => {
-                    delete_composefs_deployment(&depl_id, storage, &booted_cfs, dry_run).await
+                    delete_composefs_deployment(&depl_id, storage, &booted_cfs).await
                 }
             }
         }

crates/lib/src/composefs_consts.rs

@@ -1,5 +1,3 @@
-#![allow(dead_code)]
-
 /// composefs= parameter in kernel cmdline
 pub const COMPOSEFS_CMDLINE: &str = "composefs";
 
@@ -38,3 +36,6 @@ pub(crate) const TYPE1_ENT_PATH: &str = "loader/entries";
 pub(crate) const TYPE1_ENT_PATH_STAGED: &str = "loader/entries.staged";
 
 pub(crate) const BOOTC_FINALIZE_STAGED_SERVICE: &str = "bootc-finalize-staged.service";
+
+/// The prefix for the directories containing kernel + initrd
+pub(crate) const TYPE1_BOOT_DIR_PREFIX: &str = "bootc_composefs-";

tmt/tests/booted/test-composefs-gc.nu

@@ -10,6 +10,8 @@ use tap.nu
 let st = bootc status --json | from json
 let booted = $st.status.booted.image
 
+let dir_prefix = "bootc_composefs-"
+
 if not (tap is_composefs) or ($st.status.booted.composefs.bootType | str downcase) == "uki" {
     exit 0
 }
@@ -111,15 +113,17 @@ def third_boot [] {
         "/sysroot/boot"
     }
 
-    assert ($"($boot_dir)/($booted_verity)" | path exists)
+    print $"bootdir ($boot_dir)"
+
+    assert ($"($boot_dir)/($dir_prefix)($booted_verity)" | path exists)
 
     # This is for the rollback, but since the rollback and the very
     # first boot have the same kernel + initrd pair, and this rollback
     # was deployed after the first boot, we will still be using the very
     # first verity for the boot binary name
-    assert ($"($boot_dir)/(cat /var/first-verity)" | path exists)
+    assert ($"($boot_dir)/($dir_prefix)(cat /var/first-verity)" | path exists)
 
-    echo $"($boot_dir)/($booted_verity)" | save /var/to-be-deleted-kernel
+    echo $"($boot_dir)/($dir_prefix)(cat /var/first-verity)" | save /var/to-be-deleted-kernel
 
     # Now we create a new image derived from the current kernel + initrd
     # Switching to this and rebooting should remove the old kernel + initrd
@@ -135,6 +139,14 @@ def third_boot [] {
 }
 
 def fourth_boot [] {
+    let bootloader = (bootc status --json | from json).status.booted.composefs.bootloader
+
+    if ($bootloader | str downcase) == "systemd" {
+        # TODO: Some concrete API for this would be great
+        mkdir /var/tmp/efi
+        mount /dev/vda2 /var/tmp/efi
+    }
+
     assert equal $booted.image.image "localhost/bootc-final"
     assert (not ((cat /var/to-be-deleted-kernel | path exists)))