cli: Un-hide bootupctl backend and document its verbs

.github/workflows/ci.yml

@@ -161,4 +161,10 @@ jobs:
         run: |
           set -xeuo pipefail
           sudo ./scripts/test-uefi-vm-boot.sh "$IMG_NAME" "ostree"
-          sudo ./scripts/test-uefi-vm-boot.sh "$IMG_NAME" "composefs"
+
+          # TODO: Skip composefs tests for C10S as bootc 1.16.3 uses composefs.digest=
+          # cmdline, but for some reason that cmdline is not mentioned in 
+          # bootc-root-setup.service
+          if [[ "${{ matrix.stream }}" != "10" ]]; then
+            sudo ./scripts/test-uefi-vm-boot.sh "$IMG_NAME" "composefs"
+          fi

src/aleph.rs

@@ -12,6 +12,7 @@
 
 use anyhow::{Context, Result};
 use chrono::prelude::*;
+use fn_error_context::context;
 use serde::{Deserialize, Serialize};
 use std::fs::File;
 use std::path::Path;
@@ -36,6 +37,7 @@ const ALEPH_PATHS: &[&str] = &[
     "sysroot/.bootc-aleph.json",
 ];
 
+#[context("Getting aleph version")]
 pub(crate) fn get_aleph_version(root: &Path) -> Result<Option<AlephWithTimestamp>> {
     for aleph_path in ALEPH_PATHS {
         let path = &root.join(aleph_path);

src/cli/bootupctl.rs

@@ -49,10 +49,6 @@ impl CtlCommand {
 /// CLI sub-commands.
 #[derive(Debug, Parser)]
 pub enum CtlVerb {
-    // FIXME(lucab): drop this after refreshing
-    // https://github.com/coreos/fedora-coreos-config/pull/595
-    #[clap(name = "backend", hide = true, subcommand)]
-    Backend(CtlBackend),
     #[clap(name = "status", about = "Show components status")]
     Status(StatusOpts),
     #[clap(name = "update", about = "Update all components")]
@@ -61,6 +57,14 @@ pub enum CtlVerb {
     AdoptAndUpdate(AdoptAndUpdateOpts),
     #[clap(name = "validate", about = "Validate system state")]
     Validate,
+    /// Low-level commands for building and installing OS images
+    ///
+    /// These act on an OS tree or target root rather than on the running
+    /// system, and are intended for image build and installation tooling
+    /// (e.g. `bootc install` or disk image builders). To update the
+    /// bootloader of a booted system, use `update` or `adopt-and-update`.
+    #[clap(name = "backend", subcommand)]
+    Backend(super::bootupd::DVerb),
     #[clap(
         name = "migrate-static-grub-config",
         hide = true,
@@ -69,17 +73,6 @@ pub enum CtlVerb {
     MigrateStaticGrubConfig,
 }
 
-#[derive(Debug, Parser)]
-pub enum CtlBackend {
-    #[clap(name = "generate-update-metadata", hide = true)]
-    Generate(super::bootupd::GenerateOpts),
-    #[clap(name = "install", hide = true)]
-    Install(super::bootupd::InstallOpts),
-    #[clap(hide = true)]
-    #[cfg(efi_arch)]
-    SetDefaultBootloader(super::bootupd::DefaultBootloaderOpts),
-}
-
 #[derive(Debug, Parser)]
 pub struct StatusOpts {
     /// If there are updates available, output `Updates available: ` to standard output;
@@ -123,16 +116,7 @@ impl CtlCommand {
             CtlVerb::Update(opts) => Self::run_update(opts),
             CtlVerb::AdoptAndUpdate(opts) => Self::run_adopt_and_update(opts),
             CtlVerb::Validate => Self::run_validate(),
-            CtlVerb::Backend(CtlBackend::Generate(opts)) => {
-                super::bootupd::DCommand::run_generate_meta(opts)
-            }
-            CtlVerb::Backend(CtlBackend::Install(opts)) => {
-                super::bootupd::DCommand::run_install(opts)
-            }
-            #[cfg(efi_arch)]
-            CtlVerb::Backend(CtlBackend::SetDefaultBootloader(opts)) => {
-                super::bootupd::DCommand::set_default_bootloader(opts)
-            }
+            CtlVerb::Backend(verb) => verb.run(),
             CtlVerb::MigrateStaticGrubConfig => Self::run_migrate_static_grub_config(),
         }
     }

src/cli/bootupd.rs

@@ -36,10 +36,21 @@ impl DCommand {
 /// CLI sub-commands.
 #[derive(Debug, Parser)]
 pub enum DVerb {
-    #[clap(name = "generate-update-metadata", about = "Generate metadata")]
+    /// Generate update metadata from the bootloader files in an OS tree
+    ///
+    /// Run this at image build time. It records the bootloader component
+    /// versions (e.g. from the RPM database) under /usr/lib/bootupd/updates,
+    /// which `install` and `update` later use as their source.
+    #[clap(name = "generate-update-metadata")]
     GenerateUpdateMetadata(GenerateOpts),
-    #[clap(name = "install", about = "Install components")]
+    /// Install bootloader components to a target root
+    ///
+    /// Used when creating a disk image or installing a system: copies the
+    /// update payload from the source root into the target's ESP and/or
+    /// boot partition, and installs to the target block device(s) as needed.
+    #[clap(name = "install")]
     Install(InstallOpts),
+    /// Set the default bootloader on EFI systems
     #[cfg(efi_arch)]
     SetDefaultBootloader(DefaultBootloaderOpts),
 }
@@ -94,7 +105,7 @@ pub(crate) struct InstallOpts {
 
 #[derive(Debug, Parser)]
 pub struct GenerateOpts {
-    /// Physical root mountpoint
+    /// Root of the OS tree (only `/` is currently supported)
     #[clap(value_parser)]
     sysroot: Option<String>,
 }
@@ -108,15 +119,22 @@ pub struct DefaultBootloaderOpts {
     pub(crate) bootloader: Bootloader,
 }
 
+impl DVerb {
+    /// Run this verb; shared by `bootupd` and `bootupctl backend`.
+    pub(crate) fn run(self) -> Result<()> {
+        match self {
+            DVerb::Install(opts) => DCommand::run_install(opts),
+            DVerb::GenerateUpdateMetadata(opts) => DCommand::run_generate_meta(opts),
+            #[cfg(efi_arch)]
+            DVerb::SetDefaultBootloader(opts) => DCommand::set_default_bootloader(opts),
+        }
+    }
+}
+
 impl DCommand {
     /// Run CLI application.
     pub fn run(self) -> Result<()> {
-        match self.cmd {
-            DVerb::Install(opts) => Self::run_install(opts),
-            DVerb::GenerateUpdateMetadata(opts) => Self::run_generate_meta(opts),
-            #[cfg(efi_arch)]
-            DVerb::SetDefaultBootloader(opts) => Self::set_default_bootloader(opts),
-        }
+        self.cmd.run()
     }
 
     /// Runner for `generate-install-metadata` verb.

src/cli/mod.rs

@@ -58,6 +58,23 @@ mod tests {
         bootupd::DCommand::command().debug_assert();
     }
 
+    #[test]
+    fn backend_is_documented() {
+        use clap::CommandFactory;
+        let ctl = bootupctl::CtlCommand::command();
+        let backend = ctl.find_subcommand("backend").unwrap();
+        assert!(!backend.is_hide_set());
+        assert!(backend.get_about().is_some());
+        for verb in backend.get_subcommands() {
+            assert!(!verb.is_hide_set(), "{} is hidden", verb.get_name());
+            assert!(
+                verb.get_about().is_some(),
+                "{} has no help",
+                verb.get_name()
+            );
+        }
+    }
+
     #[test]
     fn test_multicall_dispatch() {
         {

src/grubconfigs.rs

@@ -174,27 +174,41 @@ fn write_grubenv(grubdir: &Dir) -> Result<()> {
         .run_inherited_with_cmd_context()
 }
 
-#[context("Ensure file permissions are 0600")]
+#[context("Ensure file permissions are {GRUBCONFIG_FILE_MODE:04o}")]
 fn ensure_file_permissions(target_dir: &Dir, target_file: &str) -> Result<()> {
-    let metadata = match target_dir.metadata(target_file) {
+    use std::os::unix::fs::PermissionsExt;
+
+    // grub.cfg may be a symlink pointing outside this Dir cap_std refuses to traverse
+    // such symlinks via openat, so resolve through /proc/self/fd
+    let absolute_path = format!("/proc/self/fd/{}/{target_file}", target_dir.as_raw_fd());
+
+    let metadata = match std::fs::metadata(&absolute_path) {
         Ok(metadata) => metadata,
         Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
             // Silently return if file doesn't exist
+            log::debug!("File {target_file} not found");
             return Ok(());
         }
         Err(e) => return Err(e).context(format!("Reading {} metadata", target_file)),
     };
 
     let mode = metadata.permissions().mode() & 0o777;
     if mode != GRUBCONFIG_FILE_MODE {
-        target_dir
-            .set_permissions(target_file, Permissions::from_mode(GRUBCONFIG_FILE_MODE))
-            .with_context(|| format!("Setting {} permissions to 0600", target_file))?;
+        std::fs::set_permissions(
+            absolute_path,
+            std::fs::Permissions::from_mode(GRUBCONFIG_FILE_MODE),
+        )
+        .with_context(|| {
+            format!(
+                "Setting {} permissions to {GRUBCONFIG_FILE_MODE:04o}",
+                target_file
+            )
+        })?;
     }
     Ok(())
 }
 
-#[context("Ensure grub files permissions are 0600")]
+#[context("Ensure grub files permissions are {GRUBCONFIG_FILE_MODE:04o}")]
 pub(crate) fn ensure_grub_permissions(grub_dir: &Dir) -> Result<()> {
     for file_name in GRUB_FILES.iter() {
         ensure_file_permissions(&grub_dir, file_name)
@@ -251,7 +265,7 @@ mod tests {
         {
             let metadata = td.metadata("grubenv")?;
             let mode = metadata.permissions().mode() & 0o777;
-            assert_eq!(mode, 0o600);
+            assert_eq!(mode, GRUBCONFIG_FILE_MODE);
         }
         Ok(())
     }
@@ -285,11 +299,29 @@ mod tests {
 
         // Verify grubenv was fixed to 0o600
         let metadata = grub_dir.metadata("grubenv")?;
-        assert_eq!(metadata.permissions().mode() & 0o777, 0o600);
+        assert_eq!(metadata.permissions().mode() & 0o777, GRUBCONFIG_FILE_MODE);
 
         // Verify grub.cfg was fixed 0o600
         let metadata = grub_dir.metadata("grub.cfg")?;
-        assert_eq!(metadata.permissions().mode() & 0o777, 0o600);
+        assert_eq!(metadata.permissions().mode() & 0o777, GRUBCONFIG_FILE_MODE);
+
+        use std::os::unix::fs::PermissionsExt;
+
+        // Test it with symlinks pointing outside the grub2 dir
+        // Create the symlink target outside the grub2 dir, with wrong perms
+        let external_target = tdp.join("external-grub.cfg");
+        std::fs::write(&external_target, "external grub.cfg content")?;
+        std::fs::set_permissions(&external_target, std::fs::Permissions::from_mode(0o644))?;
+
+        grub_dir.remove_file("grub.cfg")?;
+        std::os::unix::fs::symlink(&external_target, grub.join("grub.cfg"))?;
+
+        ensure_grub_permissions(&grub_dir)?;
+
+        // Verify grub.cfg was fixed 0o600
+        let metadata = std::fs::metadata(&external_target)?;
+        assert_eq!(metadata.permissions().mode() & 0o777, GRUBCONFIG_FILE_MODE);
+
         Ok(())
     }
 }