libvirt: Clean up connection URI handling

We were incorrectly trying to load the system libvirt state,
which we may not have permissions to do (e.g. default GHA on Ubuntu
setup).

Change all the code to consistently spawn `virsh` via a helper.

Signed-off-by: Colin Walters <[email protected]>

Author: cgwalters

crates/kit/src/libvirt/base_disks.rs

@@ -18,7 +18,7 @@ pub fn find_or_create_base_disk(
     image_digest: &str,
     install_options: &InstallOptions,
     kernel_args: &[String],
-    connect_uri: Option<&String>,
+    connect_uri: Option<&str>,
 ) -> Result<Utf8PathBuf> {
     let metadata = DiskImageMetadata::from(install_options, image_digest, kernel_args);
     let cache_hash = metadata.compute_cache_hash();
@@ -82,7 +82,7 @@ fn create_base_disk(
     image_digest: &str,
     install_options: &InstallOptions,
     kernel_args: &[String],
-    connect_uri: Option<&String>,
+    connect_uri: Option<&str>,
 ) -> Result<()> {
     use crate::run_ephemeral::CommonVmOpts;
     use crate::to_disk::{Format, ToDiskAdditionalOpts, ToDiskOpts};
@@ -166,10 +166,7 @@ fn create_base_disk(
             }
 
             // Refresh libvirt storage pool so the new disk is visible to virsh
-            let mut cmd = crate::hostexec::command("virsh", None)?;
-            if let Some(uri) = connect_uri {
-                cmd.arg("-c").arg(uri);
-            }
+            let mut cmd = super::run::virsh_command(connect_uri)?;
             cmd.args(&["pool-refresh", "default"]);
 
             if let Err(e) = cmd
@@ -200,7 +197,7 @@ fn create_base_disk(
 pub fn clone_from_base(
     base_disk_path: &Utf8Path,
     vm_name: &str,
-    connect_uri: Option<&String>,
+    connect_uri: Option<&str>,
 ) -> Result<Utf8PathBuf> {
     let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?;
 
@@ -209,20 +206,13 @@ pub fn clone_from_base(
     let vm_disk_path = pool_path.join(&vm_disk_name);
 
     // Refresh the storage pool so libvirt knows about all files
-    let mut refresh_cmd = crate::hostexec::command("virsh", None)?;
-    if let Some(uri) = connect_uri {
-        refresh_cmd.arg("-c").arg(uri);
-    }
+    let mut refresh_cmd = super::run::virsh_command(connect_uri)?;
     refresh_cmd.args(&["pool-refresh", "default"]);
     let _ = refresh_cmd.output(); // Ignore errors, pool might not exist yet
 
     // Try to delete the volume if it exists (either as a file or in libvirt's view)
     // This handles both cases: file exists but not tracked, or tracked by libvirt
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    if let Some(uri) = connect_uri {
-        cmd.arg("-c").arg(uri);
-    }
-
+    let mut cmd = super::run::virsh_command(connect_uri)?;
     cmd.args(&["vol-delete", "--pool", "default", &vm_disk_name]);
 
     let output = cmd
@@ -270,12 +260,7 @@ pub fn clone_from_base(
         color_eyre::eyre::eyre!("Base disk path has no filename: {:?}", base_disk_path)
     })?;
 
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-
-    if let Some(uri) = connect_uri {
-        cmd.arg("-c").arg(uri);
-    }
-
+    let mut cmd = super::run::virsh_command(connect_uri)?;
     cmd.args(&[
         "vol-create-as",
         "default",
@@ -309,7 +294,7 @@ pub fn clone_from_base(
 }
 
 /// List all base disks in the storage pool with reference counts
-pub fn list_base_disks(connect_uri: Option<&String>) -> Result<Vec<BaseDiskInfo>> {
+pub fn list_base_disks(connect_uri: Option<&str>) -> Result<Vec<BaseDiskInfo>> {
     use super::run::list_storage_pool_volumes;
 
     let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?;
@@ -372,7 +357,7 @@ pub struct BaseDiskInfo {
 }
 
 /// Prune unreferenced base disks
-pub fn prune_base_disks(connect_uri: Option<&String>, dry_run: bool) -> Result<Vec<Utf8PathBuf>> {
+pub fn prune_base_disks(connect_uri: Option<&str>, dry_run: bool) -> Result<Vec<Utf8PathBuf>> {
     use super::run::list_storage_pool_volumes;
 
     let base_disks = list_base_disks(connect_uri)?;
@@ -407,10 +392,7 @@ pub fn prune_base_disks(connect_uri: Option<&String>, dry_run: bool) -> Result<V
                     color_eyre::eyre::eyre!("Base disk path has no filename: {:?}", base_disk.path)
                 })?;
 
-                let mut cmd = crate::hostexec::command("virsh", None)?;
-                if let Some(uri) = connect_uri {
-                    cmd.arg("-c").arg(uri);
-                }
+                let mut cmd = super::run::virsh_command(connect_uri)?;
                 cmd.args(&["vol-delete", "--pool", "default", base_disk_name]);
 
                 let output = cmd.output().with_context(|| {

crates/kit/src/libvirt/base_disks_cli.rs

@@ -45,7 +45,7 @@ pub struct PruneOpts {
 
 /// Execute the base-disks command
 pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtBaseDisksOpts) -> Result<()> {
-    let connect_uri = global_opts.connect.as_ref();
+    let connect_uri = global_opts.connect.as_deref();
 
     match opts.command {
         BaseDisksSubcommand::List(list_opts) => run_list(connect_uri, list_opts),
@@ -54,7 +54,7 @@ pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtBaseDisksO
 }
 
 /// Execute the list subcommand
-fn run_list(connect_uri: Option<&String>, opts: ListOpts) -> Result<()> {
+fn run_list(connect_uri: Option<&str>, opts: ListOpts) -> Result<()> {
     let base_disks = list_base_disks(connect_uri)?;
 
     match opts.format {
@@ -115,7 +115,7 @@ fn run_list(connect_uri: Option<&String>, opts: ListOpts) -> Result<()> {
 }
 
 /// Execute the prune subcommand
-fn run_prune(connect_uri: Option<&String>, opts: PruneOpts) -> Result<()> {
+fn run_prune(connect_uri: Option<&str>, opts: PruneOpts) -> Result<()> {
     if opts.dry_run {
         println!("Dry run: showing base disks that would be removed");
     }

crates/kit/src/libvirt/run.rs

@@ -17,6 +17,15 @@ use crate::libvirt::domain::VirtiofsFilesystem;
 use crate::utils::parse_memory_to_mb;
 use crate::xml_utils;
 
+/// Create a virsh command with optional connection URI
+pub(super) fn virsh_command(connect_uri: Option<&str>) -> Result<std::process::Command> {
+    let mut cmd = crate::hostexec::command("virsh", None)?;
+    if let Some(uri) = connect_uri {
+        cmd.arg("-c").arg(uri);
+    }
+    Ok(cmd)
+}
+
 /// Firmware type for virtual machines
 #[derive(Debug, Clone, Copy, PartialEq, Eq, ValueEnum)]
 #[clap(rename_all = "kebab-case")]
@@ -117,8 +126,8 @@ pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtRunOpts) -
     // Validate labels don't contain commas
     opts.validate_labels()?;
 
-    let connect_uri = global_opts.connect.as_ref();
-    let lister = match connect_uri {
+    let connect_uri = global_opts.connect.as_deref();
+    let lister = match global_opts.connect.as_ref() {
         Some(uri) => DomainLister::with_connection(uri.clone()),
         None => DomainLister::new(),
     };
@@ -231,24 +240,38 @@ fn get_default_pool_path(connect_uri: &str) -> Utf8PathBuf {
 }
 
 /// Ensure the default libvirt storage pool exists, creating it if necessary
-fn ensure_default_pool(connect_uri: &str) -> Result<()> {
+fn ensure_default_pool(connect_uri: Option<&str>) -> Result<()> {
     // Check if default pool already exists
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    cmd.args(&["-c", connect_uri, "pool-info", "default"]);
+    let mut cmd = virsh_command(connect_uri)?;
+    cmd.args(&["pool-info", "default"]);
     let output = cmd
         .output()
         .with_context(|| "Failed to check for default pool")?;
 
     if output.status.success() {
         // Pool exists, make sure it's active
-        let mut cmd = crate::hostexec::command("virsh", None)?;
-        cmd.args(&["-c", connect_uri, "pool-start", "default"]);
+        let mut cmd = virsh_command(connect_uri)?;
+        cmd.args(&["pool-start", "default"]);
         let _ = cmd.output(); // Ignore errors if already started
         return Ok(());
     }
 
     // Pool doesn't exist, need to create it
-    let pool_path = get_default_pool_path(connect_uri);
+    // Determine the appropriate pool path based on the connection URI
+    let pool_path = if let Some(uri) = connect_uri {
+        get_default_pool_path(uri)
+    } else {
+        // If no URI specified, virsh will use its default connection
+        // We need to query what that is to determine the appropriate pool path
+        let mut cmd = virsh_command(None)?;
+        cmd.args(&["uri"]);
+        let output = cmd
+            .output()
+            .with_context(|| "Failed to query default libvirt URI")?;
+        let uri_str = String::from_utf8(output.stdout)
+            .with_context(|| "Invalid UTF-8 in virsh uri output")?;
+        get_default_pool_path(uri_str.trim())
+    };
     info!("Creating default storage pool at {:?}", pool_path);
 
     // Create the directory if it doesn't exist
@@ -271,8 +294,8 @@ fn ensure_default_pool(connect_uri: &str) -> Result<()> {
     std::fs::write(xml_path, &pool_xml).with_context(|| "Failed to write pool XML")?;
 
     // Define the pool
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    cmd.args(&["-c", connect_uri, "pool-define", xml_path]);
+    let mut cmd = virsh_command(connect_uri)?;
+    cmd.args(&["pool-define", xml_path]);
     let output = cmd.output().with_context(|| "Failed to define pool")?;
 
     if !output.status.success() {
@@ -285,13 +308,13 @@ fn ensure_default_pool(connect_uri: &str) -> Result<()> {
     }
 
     // Build the pool (creates directory structure)
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    cmd.args(&["-c", connect_uri, "pool-build", "default"]);
+    let mut cmd = virsh_command(connect_uri)?;
+    cmd.args(&["pool-build", "default"]);
     let _ = cmd.output(); // Directory might already exist
 
     // Start the pool
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    cmd.args(&["-c", connect_uri, "pool-start", "default"]);
+    let mut cmd = virsh_command(connect_uri)?;
+    cmd.args(&["pool-start", "default"]);
     let output = cmd.output().with_context(|| "Failed to start pool")?;
 
     if !output.status.success() {
@@ -304,8 +327,8 @@ fn ensure_default_pool(connect_uri: &str) -> Result<()> {
     }
 
     // Autostart the pool
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    cmd.args(&["-c", connect_uri, "pool-autostart", "default"]);
+    let mut cmd = virsh_command(connect_uri)?;
+    cmd.args(&["pool-autostart", "default"]);
     let _ = cmd.output(); // Not critical if this fails
 
     // Clean up temporary XML file
@@ -316,67 +339,27 @@ fn ensure_default_pool(connect_uri: &str) -> Result<()> {
 }
 
 /// Get the path of the default libvirt storage pool
-pub fn get_libvirt_storage_pool_path(connect_uri: Option<&String>) -> Result<Utf8PathBuf> {
-    // If a specific connection URI is provided, use it
-    if let Some(uri) = connect_uri {
-        // Ensure pool exists before querying
-        ensure_default_pool(uri)?;
+pub fn get_libvirt_storage_pool_path(connect_uri: Option<&str>) -> Result<Utf8PathBuf> {
+    // Ensure pool exists before querying
+    ensure_default_pool(connect_uri)?;
 
-        let mut cmd = crate::hostexec::command("virsh", None)?;
-        cmd.args(&["-c", uri, "pool-dumpxml", "default"]);
-        let output = cmd
-            .output()
-            .with_context(|| "Failed to query libvirt storage pool")?;
-
-        if output.status.success() {
-            let xml = String::from_utf8(output.stdout)
-                .with_context(|| "Invalid UTF-8 in virsh output")?;
-            let dom = xml_utils::parse_xml_dom(&xml)
-                .with_context(|| "Failed to parse storage pool XML")?;
-
-            if let Some(path_node) = dom.find("path") {
-                let path_str = path_node.text_content().trim();
-                if !path_str.is_empty() {
-                    return Ok(Utf8PathBuf::from(path_str));
-                }
-            }
-        }
-    }
-
-    // Try user session first (qemu:///session)
-    let session_uri = "qemu:///session";
-    ensure_default_pool(session_uri)?;
-
-    let mut cmd = crate::hostexec::command("virsh", None)?;
-    cmd.args(&["-c", session_uri, "pool-dumpxml", "default"]);
-    let output = cmd.output();
-
-    let (output, uri_used) = match output {
-        Ok(o) if o.status.success() => (o, session_uri),
-        _ => {
-            // Try system session (qemu:///system)
-            let system_uri = "qemu:///system";
-            ensure_default_pool(system_uri)?;
-
-            let mut cmd = crate::hostexec::command("virsh", None)?;
-            cmd.args(&["-c", system_uri, "pool-dumpxml", "default"]);
-            let out = cmd
-                .output()
-                .with_context(|| "Failed to query libvirt storage pool")?;
-            (out, system_uri)
-        }
-    };
+    let mut cmd = virsh_command(connect_uri)?;
+    cmd.args(&["pool-dumpxml", "default"]);
+    let output = cmd
+        .output()
+        .with_context(|| "Failed to query libvirt storage pool")?;
 
     if !output.status.success() {
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        let uri_desc = connect_uri.unwrap_or("default connection");
         return Err(color_eyre::eyre::eyre!(
-            "Failed to get default storage pool info for {}",
-            uri_used
+            "Failed to get default storage pool info for {}: {}",
+            uri_desc,
+            stderr
         ));
     }
 
     let xml = String::from_utf8(output.stdout).with_context(|| "Invalid UTF-8 in virsh output")?;
-
-    // Parse XML using DOM parser and extract path element
     let dom = xml_utils::parse_xml_dom(&xml).with_context(|| "Failed to parse storage pool XML")?;
 
     if let Some(path_node) = dom.find("path") {
@@ -432,7 +415,7 @@ fn generate_unique_vm_name(image: &str, existing_domains: &[String]) -> String {
 }
 
 /// List all volumes in the default storage pool
-pub fn list_storage_pool_volumes(connect_uri: Option<&String>) -> Result<Vec<Utf8PathBuf>> {
+pub fn list_storage_pool_volumes(connect_uri: Option<&str>) -> Result<Vec<Utf8PathBuf>> {
     // Get the storage pool path from XML
     let pool_path = get_libvirt_storage_pool_path(connect_uri)?;