Merge pull request #995 from HuijingHei/step2-ostree-boot

cgwalters · web-flow · commit 9e102d76f405 · 2025-09-28T08:28:43.000-04:00
efi: transfer `usr/lib/ostree-boot` to `usr/lib/efi`
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -99,4 +99,4 @@ jobs:
       - name: bootupctl generate-update-metadata
         run: |
           set -xeuo pipefail
-          sudo podman run --rm -v $PWD:/run/src -w /run/src --privileged localhost/bootupd:latest tests/tests/move-content-to-usr.sh
+          sudo podman run --rm -v $PWD:/run/src -w /run/src --privileged localhost/bootupd:latest tests/tests/generate-update-metadata.sh
diff --git a/src/component.rs b/src/component.rs
@@ -112,6 +112,7 @@ pub(crate) fn component_updatedirname(component: &dyn Component) -> PathBuf {
 
 /// Returns the path to the payload directory for an available update for
 /// a component.
+#[allow(dead_code)]
 #[cfg(any(
     target_arch = "x86_64",
     target_arch = "aarch64",
diff --git a/src/efi.rs b/src/efi.rs
@@ -14,6 +14,7 @@ use bootc_internal_utils::CommandRunExt;
 use camino::{Utf8Path, Utf8PathBuf};
 use cap_std::fs::Dir;
 use cap_std_ext::cap_std;
+use cap_std_ext::dirext::CapStdExtDirExt;
 use chrono::prelude::*;
 use fn_error_context::context;
 use openat_ext::OpenatDirExt;
@@ -461,18 +462,34 @@ impl Component for Efi {
     }
 
     fn generate_update_metadata(&self, sysroot: &str) -> Result<Option<ContentMetadata>> {
-        let sysroot_path = Utf8Path::new(sysroot);
+        let sysroot_path = Path::new(sysroot);
+        let sysroot_dir = Dir::open_ambient_dir(sysroot_path, cap_std::ambient_authority())?;
+
+        if let Some(ostreeboot) = sysroot_dir
+            .open_dir_optional(ostreeutil::BOOT_PREFIX)
+            .context("Opening usr/lib/ostree-boot")?
+        {
+            let cruft = ["loader", "grub2"];
+            for p in cruft.iter() {
+                ostreeboot.remove_all_optional(p)?;
+            }
+            // Transfer ostree-boot EFI files to usr/lib/efi
+            transfer_ostree_boot_to_usr(sysroot_path)?;
+
+            // Remove usr/lib/ostree-boot/efi/EFI dir (after transfer) or if it is empty
+            ostreeboot.remove_all_optional("efi/EFI")?;
+        }
+
+        // copy EFI files from usr/lib/efi to updates dir
+        if let Some(efi_components) =
+            get_efi_component_from_usr(Utf8Path::from_path(sysroot_path).unwrap(), EFILIB)?
+        {
+            // Confirm EFI has at least two components grub2 & shim
+            // See https://github.com/coreos/bootupd/issues/994
+            assert!(efi_components.len() > 1);
 
-        // copy EFI files to updates dir from usr/lib/efi
-        let efilib_path = sysroot_path.join(EFILIB);
-        let meta = if efilib_path.exists() {
             let mut packages = Vec::new();
             let mut modules_vec: Vec<Module> = vec![];
-            let sysroot_dir = Dir::open_ambient_dir(sysroot_path, cap_std::ambient_authority())?;
-            let efi_components = get_efi_component_from_usr(&sysroot_path, EFILIB)?;
-            if efi_components.len() == 0 {
-                bail!("Failed to find EFI components from {efilib_path}");
-            }
             for efi in efi_components {
                 Command::new("cp")
                     .args(["-rp", "--reflink=auto"])
@@ -490,50 +507,16 @@ impl Component for Efi {
 
             // change to now to workaround https://github.com/coreos/bootupd/issues/933
             let timestamp = std::time::SystemTime::now();
-            ContentMetadata {
+            let meta = ContentMetadata {
                 timestamp: chrono::DateTime::<Utc>::from(timestamp),
                 version: packages.join(","),
                 versions: Some(modules_vec),
-            }
+            };
+            write_update_metadata(sysroot, self, &meta)?;
+            Ok(Some(meta))
         } else {
-            let ostreebootdir = sysroot_path.join(ostreeutil::BOOT_PREFIX);
-
-            // move EFI files to updates dir from /usr/lib/ostree-boot
-            if ostreebootdir.exists() {
-                let cruft = ["loader", "grub2"];
-                for p in cruft.iter() {
-                    let p = ostreebootdir.join(p);
-                    if p.exists() {
-                        std::fs::remove_dir_all(&p)?;
-                    }
-                }
-
-                let efisrc = ostreebootdir.join("efi/EFI");
-                if !efisrc.exists() {
-                    bail!("Failed to find {:?}", &efisrc);
-                }
-
-                let dest_efidir = component_updatedir(sysroot, self);
-                let dest_efidir =
-                    Utf8PathBuf::from_path_buf(dest_efidir).expect("Path is invalid UTF-8");
-                // Fork off mv() because on overlayfs one can't rename() a lower level
-                // directory today, and this will handle the copy fallback.
-                Command::new("mv").args([&efisrc, &dest_efidir]).run()?;
-
-                let efidir = openat::Dir::open(dest_efidir.as_std_path())
-                    .with_context(|| format!("Opening {}", dest_efidir))?;
-                let files = crate::util::filenames(&efidir)?.into_iter().map(|mut f| {
-                    f.insert_str(0, "/boot/efi/EFI/");
-                    f
-                });
-                query_files(sysroot, files)?
-            } else {
-                anyhow::bail!("Failed to find {ostreebootdir}");
-            }
-        };
-
-        write_update_metadata(sysroot, self, &meta)?;
-        Ok(Some(meta))
+            anyhow::bail!("Failed to find EFI components");
+        }
     }
 
     fn query_update(&self, sysroot: &openat::Dir) -> Result<Option<ContentMetadata>> {
@@ -730,7 +713,7 @@ pub struct EFIComponent {
 fn get_efi_component_from_usr<'a>(
     sysroot: &'a Utf8Path,
     usr_path: &'a str,
-) -> Result<Vec<EFIComponent>> {
+) -> Result<Option<Vec<EFIComponent>>> {
     let efilib_path = sysroot.join(usr_path);
     let skip_count = Utf8Path::new(usr_path).components().count();
 
@@ -761,9 +744,65 @@ fn get_efi_component_from_usr<'a>(
         })
         .collect();
 
+    if components.len() == 0 {
+        return Ok(None);
+    }
     components.sort_by(|a, b| a.name.cmp(&b.name));
 
-    Ok(components)
+    Ok(Some(components))
+}
+
+/// Copy files from usr/lib/ostree-boot/efi/EFI to /usr/lib/efi/<component>/<evr>/
+fn transfer_ostree_boot_to_usr(sysroot: &Path) -> Result<()> {
+    let ostreeboot_efi = Path::new(ostreeutil::BOOT_PREFIX).join("efi");
+    let ostreeboot_efi_path = sysroot.join(&ostreeboot_efi);
+
+    let efi = ostreeboot_efi_path.join("EFI");
+    if !efi.exists() {
+        return Ok(());
+    }
+    for entry in WalkDir::new(&efi) {
+        let entry = entry?;
+
+        if entry.file_type().is_file() {
+            let entry_path = entry.path();
+
+            // get path EFI/{BOOT,<vendor>}/<file>
+            let filepath = entry_path.strip_prefix(&ostreeboot_efi_path)?;
+            // get path /boot/efi/EFI/{BOOT,<vendor>}/<file>
+            let boot_filepath = Path::new("/boot/efi").join(filepath);
+
+            // Run `rpm -qf <filepath>`
+            let pkg = crate::packagesystem::query_file(
+                sysroot.to_str().unwrap(),
+                boot_filepath.to_str().unwrap(),
+            )?;
+
+            let (name, evr) = pkg.split_once(' ').unwrap();
+            let component = name.split('-').next().unwrap_or("");
+            // get path usr/lib/efi/<component>/<evr>
+            let efilib_path = Path::new(EFILIB).join(component).join(evr);
+
+            let sysroot_dir = openat::Dir::open(sysroot)?;
+            // Ensure dest parent directory exists
+            if let Some(parent) = efilib_path.join(filepath).parent() {
+                sysroot_dir.ensure_dir_all(parent, 0o755)?;
+            }
+
+            // Source dir is usr/lib/ostree-boot/efi
+            let src = sysroot_dir
+                .sub_dir(&ostreeboot_efi)
+                .context("Opening ostree-boot dir")?;
+            // Dest dir is usr/lib/efi/<component>/<evr>
+            let dest = sysroot_dir
+                .sub_dir(&efilib_path)
+                .context("Opening usr/lib/efi dir")?;
+            // Copy file from ostree-boot to usr/lib/efi
+            src.copy_file_at(filepath, &dest, filepath)
+                .context("Copying file to usr/lib/efi")?;
+        }
+    }
+    Ok(())
 }
 
 #[cfg(test)]
@@ -900,7 +939,7 @@ Boot0003* test";
         let efi_comps = get_efi_component_from_usr(utf8_tpath, EFILIB)?;
         assert_eq!(
             efi_comps,
-            vec![
+            Some(vec![
                 EFIComponent {
                     name: "BAR".to_string(),
                     version: "1.1".to_string(),
@@ -911,12 +950,12 @@ Boot0003* test";
                     version: "1.1".to_string(),
                     path: Utf8PathBuf::from("usr/lib/efi/FOO/1.1/EFI"),
                 },
-            ]
+            ])
         );
         std::fs::remove_dir_all(efi_path.join("BAR/1.1/EFI"))?;
         std::fs::remove_dir_all(efi_path.join("FOO/1.1/EFI"))?;
         let efi_comps = get_efi_component_from_usr(utf8_tpath, EFILIB)?;
-        assert_eq!(efi_comps, []);
+        assert_eq!(efi_comps, None);
         Ok(())
     }
 }
diff --git a/src/packagesystem.rs b/src/packagesystem.rs
@@ -115,6 +115,21 @@ fn split_name_version(input: &str) -> Option<(String, String)> {
     Some((name.to_string(), format!("{version}-{release}")))
 }
 
+/// Query the rpm database and get package "<name> <evr>"
+pub(crate) fn query_file(sysroot_path: &str, path: &str) -> Result<String> {
+    let mut c = ostreeutil::rpm_cmd(sysroot_path)?;
+    c.args(["-q", "--queryformat", "%{NAME} %{EVR}", "-f", path]);
+
+    let rpmout = c.output()?;
+    if !rpmout.status.success() {
+        std::io::stderr().write_all(&rpmout.stderr)?;
+        bail!("Failed to invoke rpm -qf");
+    }
+
+    let output = String::from_utf8(rpmout.stdout)?;
+    Ok(output.trim().to_string())
+}
+
 fn parse_evr(pkg: &str) -> Module {
     // assume it is "grub2-1:2.12-28.fc42" (from usr/lib/efi)
     if !pkg.ends_with(std::env::consts::ARCH) {
diff --git a/src/util.rs b/src/util.rs
@@ -19,6 +19,7 @@ pub(crate) fn getenv_utf8(n: &str) -> Result<Option<String>> {
     }
 }
 
+#[allow(dead_code)]
 pub(crate) fn filenames(dir: &openat::Dir) -> Result<HashSet<String>> {
     let mut ret = HashSet::new();
     for entry in dir.list_dir(".")? {
diff --git a/tests/e2e-update/e2e-update-in-vm.sh b/tests/e2e-update/e2e-update-in-vm.sh
@@ -18,9 +18,9 @@ if ! test -f ${stampfile}; then
         fatal "already at ${TARGET_COMMIT}"
     fi
 
-    current_grub=$(rpm -q --queryformat='%{nevra}\n' ${TARGET_GRUB_NAME})
-    if test "${current_grub}" == "${TARGET_GRUB_PKG}"; then
-        fatal "Current grub ${current_grub} is same as target ${TARGET_GRUB_PKG}"
+    current_grub=$(rpm -q --queryformat='%{evr}\n' ${TARGET_GRUB_NAME})
+    if test "${current_grub}" == "${TARGET_GRUB_EVR}"; then
+        fatal "Current grub ${current_grub} is same as target ${TARGET_GRUB_EVR}"
     fi
 
     # FIXME
@@ -44,20 +44,20 @@ if test "$semode" != Enforcing; then
     fatal "SELinux mode is ${semode}"
 fi
 
-if ! test -n "${TARGET_GRUB_PKG}"; then
-    fatal "Missing TARGET_GRUB_PKG"
+if ! test -n "${TARGET_GRUB_EVR}"; then
+    fatal "Missing TARGET_GRUB_EVR"
 fi
 
 bootupctl validate
 ok validate
 
 bootupctl status | tee out.txt
 assert_file_has_content_literal out.txt 'Component EFI'
-assert_file_has_content_literal out.txt '  Installed: grub2-efi-x64-'
-assert_not_file_has_content out.txt '  Installed:.*test-bootupd-payload'
-assert_not_file_has_content out.txt '  Installed:.*'"${TARGET_GRUB_PKG}"
-assert_file_has_content out.txt 'Update: Available:.*'"${TARGET_GRUB_PKG}"
-assert_file_has_content out.txt 'Update: Available:.*test-bootupd-payload-1.0'
+assert_file_has_content_literal out.txt '  Installed: grub2-1:'
+assert_not_file_has_content out.txt '  Installed:.*test_bootupd_payload'
+assert_not_file_has_content out.txt '  Installed:.*'"${TARGET_GRUB_EVR}"
+assert_file_has_content out.txt 'Update: Available:.*'"${TARGET_GRUB_EVR}"
+assert_file_has_content out.txt 'Update: Available:.*test_bootupd_payload-1.0'
 bootupctl status --print-if-available > out.txt
 assert_file_has_content_literal 'out.txt' 'Updates available: BIOS EFI'
 ok update avail
@@ -74,7 +74,7 @@ assert_file_has_content err.txt "error: .*synthetic failpoint"
 
 bootupctl update -vvv | tee out.txt
 assert_file_has_content out.txt "Previous EFI: .*"
-assert_file_has_content out.txt "Updated EFI: ${TARGET_GRUB_PKG}.*,test-bootupd-payload-1.0"
+assert_file_has_content out.txt "Updated EFI: .*${TARGET_GRUB_EVR}.*,test_bootupd_payload-1.0"
 
 assert_file_has_content ${tmpefimount}/EFI/fedora/test-bootupd.efi test-payload
 
diff --git a/tests/e2e-update/e2e-update.sh b/tests/e2e-update/e2e-update.sh
@@ -25,7 +25,7 @@ export test_tmpdir=${testtmp}
 # This is new content for our update
 test_bootupd_payload_file=/boot/efi/EFI/fedora/test-bootupd.efi
 test_bootupd_payload_file1=/boot/efi/EFI/BOOT/test-bootupd1.efi
-build_rpm test-bootupd-payload \
+build_rpm test_bootupd_payload \
   files "${test_bootupd_payload_file}
          ${test_bootupd_payload_file1}" \
   install "mkdir -p %{buildroot}/$(dirname ${test_bootupd_payload_file})
@@ -56,9 +56,9 @@ if test -z "${e2e_skip_build:-}"; then
     runv cosa build
     runv cosa osbuild qemu
     prev_image=$(runv cosa meta --image-path qemu)
-    # Modify manifest to include `test-bootupd-payload` RPM
+    # Modify manifest to include `test_bootupd_ayload` RPM
     runv git -C src/config checkout manifest.yaml # first make sure it's clean
-    echo "packages: [test-bootupd-payload]" >> src/config/manifest.yaml
+    echo "packages: [test_bootupd_payload]" >> src/config/manifest.yaml
     rm -f ${overrides}/rpm/*.rpm
     echo "Building update ostree"
     # Latest (current) version in F42
@@ -77,7 +77,7 @@ case $(arch) in
     *) fatal "Unhandled arch $(arch)";;
 esac
 target_grub_name=grub2-efi-${grubarch}
-target_grub_pkg=$(rpm -qp --queryformat='%{nevra}\n' ${overrides}/rpm/${target_grub_name}-2*.rpm)
+target_grub_evr=$(rpm -qp --queryformat='%{evr}\n' ${overrides}/rpm/${target_grub_name}-2*.rpm)
 target_commit=$(cosa meta --get-value ostree-commit)
 echo "Target commit: ${target_commit}"
 # For some reason 9p can't write to tmpfs
@@ -97,7 +97,7 @@ systemd:
         RemainAfterExit=yes
         Environment=TARGET_COMMIT=${target_commit}
         Environment=TARGET_GRUB_NAME=${target_grub_name}
-        Environment=TARGET_GRUB_PKG=${target_grub_pkg}
+        Environment=TARGET_GRUB_EVR=${target_grub_evr}
         Environment=SRCDIR=/run/bootupd-source
         # Run via shell because selinux denies systemd writing to 9p apparently
         ExecStart=/bin/sh -c '/run/bootupd-source/${testprefix}/e2e-update-in-vm.sh &>>/run/testtmp/out.txt; test -f /run/rebooting || poweroff -ff'
diff --git a/tests/kola/test-bootupd b/tests/kola/test-bootupd
diff --git a/tests/tests/generate-update-metadata.sh b/tests/tests/generate-update-metadata.sh
diff --git a/tests/tests/move-content-to-usr.sh b/tests/tests/move-content-to-usr.sh

Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@ pub(crate) fn getenv_utf8(n: &str) -> Result<Option<String>> {`
`19`	`19`	`}`
`20`	`20`	`}`
`21`	`21`
	`22`	`+#[allow(dead_code)]`
`22`	`23`	`pub(crate) fn filenames(dir: &openat::Dir) -> Result<HashSet<String>> {`
`23`	`24`	`let mut ret = HashSet::new();`
`24`	`25`	`for entry in dir.list_dir(".")? {`