From e21149c7946f5670b4c159819fca5b6c6b9fdeab Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 22 Sep 2025 17:51:52 -0400 Subject: [PATCH] Switch to quick-xml Generating and parsing XML manually by scraping and gluing strings is obviously bad. Assisted-by: Claude Code Signed-off-by: Colin Walters --- Cargo.lock | 12 +- crates/kit/Cargo.toml | 1 + crates/kit/src/arch.rs | 139 ++++++--- crates/kit/src/domain_list.rs | 152 +++++----- crates/kit/src/libvirt/create.rs | 28 +- crates/kit/src/libvirt/domain.rs | 235 +++++++--------- crates/kit/src/libvirt/list_volumes.rs | 32 +-- crates/kit/src/libvirt/run.rs | 16 +- crates/kit/src/libvirt/ssh.rs | 62 ++-- crates/kit/src/libvirt_upload_disk.rs | 120 +++++--- crates/kit/src/main.rs | 1 + crates/kit/src/xml_utils.rs | 373 +++++++++++++++++++++++++ 12 files changed, 821 insertions(+), 350 deletions(-) create mode 100644 crates/kit/src/xml_utils.rs diff --git a/Cargo.lock b/Cargo.lock index 32d2489cb..f71acbd32 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -179,6 +179,7 @@ dependencies = [ "libc", "nix", "notify", + "quick-xml", "rand 0.9.1", "regex", "reqwest", @@ -1629,6 +1630,15 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "quick-xml" +version = "0.36.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f7649a7b4df05aed9ea7ec6f628c67c9953a43869b8bc50929569b2999d443fe" +dependencies = [ + "memchr", +] + [[package]] name = "quote" version = "1.0.40" @@ -2693,7 +2703,7 @@ version = "0.1.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0978bf7171b3d90bac376700cb56d606feb40f251a475a5d6634613564460b22" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] diff --git a/crates/kit/Cargo.toml b/crates/kit/Cargo.toml index 67de2c7ee..e00338cfc 100644 --- a/crates/kit/Cargo.toml +++ b/crates/kit/Cargo.toml @@ -44,6 +44,7 @@ libc = "0.2" camino = "1.1.12" comfy-table = "7.1" strum = { version = "0.26", features = ["derive"] } +quick-xml = "0.36" [dev-dependencies] similar-asserts = "1.5" diff --git a/crates/kit/src/arch.rs b/crates/kit/src/arch.rs index 6119d612b..9d37bdf98 100644 --- a/crates/kit/src/arch.rs +++ b/crates/kit/src/arch.rs @@ -3,6 +3,7 @@ //! This module provides cross-architecture support for libvirt domain creation //! and QEMU emulator selection, avoiding hardcoded architecture assumptions. +use crate::xml_utils::XmlWriter; use color_eyre::Result; /// Architecture configuration for libvirt domains and QEMU @@ -44,52 +45,33 @@ impl ArchConfig { } } - /// Get architecture-specific XML features for libvirt - pub fn xml_features(&self) -> &'static str { - match self.arch { - "x86_64" => { - r#" - - - - - "# - } - "aarch64" => { - r#" - - - - "# - } - _ => { - r#" - - - - "# - } + /// Generate architecture-specific XML features for libvirt + pub fn write_features(&self, writer: &mut XmlWriter) -> Result<()> { + writer.start_element("features", &[])?; + writer.write_empty_element("acpi", &[])?; + writer.write_empty_element("apic", &[])?; + + // Add x86_64-specific features + if self.arch == "x86_64" { + writer.write_empty_element("vmport", &[("state", "off")])?; } + + writer.end_element("features")?; + Ok(()) } - /// Get architecture-specific timer configuration - pub fn xml_timers(&self) -> &'static str { - match self.arch { - "x86_64" => { - r#" - - - "# - } - "aarch64" => { - r#" - "# - } - _ => { - r#" - "# - } + /// Generate architecture-specific timer configuration + pub fn write_timers(&self, writer: &mut XmlWriter) -> Result<()> { + // RTC timer is common to all architectures + writer.write_empty_element("timer", &[("name", "rtc"), ("tickpolicy", "catchup")])?; + + // Add x86_64-specific timers + if self.arch == "x86_64" { + writer.write_empty_element("timer", &[("name", "pit"), ("tickpolicy", "delay")])?; + writer.write_empty_element("timer", &[("name", "hpet"), ("present", "no")])?; } + + Ok(()) } /// Check if this architecture supports VMport (x86_64 specific feature) @@ -146,9 +128,20 @@ mod tests { fn test_arch_specific_features() { let arch_config = ArchConfig::detect().unwrap(); - // All architectures should have some features - assert!(!arch_config.xml_features().is_empty()); - assert!(!arch_config.xml_timers().is_empty()); + // Test that we can generate features XML without errors + let mut writer = XmlWriter::new(); + arch_config.write_features(&mut writer).unwrap(); + let features_xml = writer.into_string().unwrap(); + assert!(features_xml.contains("")); + assert!(features_xml.contains("")); + assert!(features_xml.contains("")); + + // Test that we can generate timers XML without errors + let mut writer = XmlWriter::new(); + arch_config.write_timers(&mut writer).unwrap(); + let timers_xml = writer.into_string().unwrap(); + assert!(timers_xml.contains("timer")); + assert!(timers_xml.contains("rtc")); // CPU mode should be valid assert!(!arch_config.cpu_mode().is_empty()); @@ -177,4 +170,60 @@ mod tests { // Should be mutually exclusive assert!(!(is_x86_64() && is_aarch64())); } + + /// Helper function to generate XML for testing + fn generate_xml(config: &ArchConfig, writer_fn: F) -> String + where + F: FnOnce(&ArchConfig, &mut XmlWriter) -> Result<()>, + { + let mut writer = XmlWriter::new(); + writer_fn(config, &mut writer).unwrap(); + writer.into_string().unwrap() + } + + #[test] + fn test_x86_64_specific_features() { + // Test x86_64 configuration specifically + let x86_config = ArchConfig { + arch: "x86_64", + machine: "q35", + os_type: "hvm", + }; + + let features_xml = generate_xml(&x86_config, |cfg, w| cfg.write_features(w)); + + // Should have x86_64-specific vmport feature + assert!(features_xml.contains("vmport")); + assert!(features_xml.contains("state=\"off\"")); + + let timers_xml = generate_xml(&x86_config, |cfg, w| cfg.write_timers(w)); + + // Should have x86_64-specific timers + assert!(timers_xml.contains("pit")); + assert!(timers_xml.contains("hpet")); + assert!(timers_xml.contains("present=\"no\"")); + } + + #[test] + fn test_aarch64_specific_features() { + // Test aarch64 configuration specifically + let arm_config = ArchConfig { + arch: "aarch64", + machine: "virt", + os_type: "hvm", + }; + + let features_xml = generate_xml(&arm_config, |cfg, w| cfg.write_features(w)); + + // Should NOT have x86_64-specific vmport feature + assert!(!features_xml.contains("vmport")); + + let timers_xml = generate_xml(&arm_config, |cfg, w| cfg.write_timers(w)); + + // Should NOT have x86_64-specific timers + assert!(!timers_xml.contains("pit")); + assert!(!timers_xml.contains("hpet")); + // But should still have RTC + assert!(timers_xml.contains("rtc")); + } } diff --git a/crates/kit/src/domain_list.rs b/crates/kit/src/domain_list.rs index 67ef5ad57..2d1b80ebf 100644 --- a/crates/kit/src/domain_list.rs +++ b/crates/kit/src/domain_list.rs @@ -3,6 +3,7 @@ //! This module provides functionality to list libvirt domains created by bcvk libvirt, //! using libvirt as the source of truth instead of the VmRegistry cache. +use crate::xml_utils; use color_eyre::{eyre::Context, Result}; use serde::{Deserialize, Serialize}; use std::process::Command; @@ -130,8 +131,8 @@ impl DomainLister { Ok(String::from_utf8(output.stdout)?.trim().to_string()) } - /// Get domain XML metadata - pub fn get_domain_xml(&self, domain_name: &str) -> Result { + /// Get domain XML metadata as parsed DOM + pub fn get_domain_xml(&self, domain_name: &str) -> Result { let output = self .virsh_command() .args(&["dumpxml", domain_name]) @@ -147,11 +148,16 @@ impl DomainLister { )); } - Ok(String::from_utf8(output.stdout)?) + let xml = String::from_utf8(output.stdout)?; + xml_utils::parse_xml_dom(&xml) + .with_context(|| format!("Failed to parse XML for domain '{}'", domain_name)) } - /// Extract podman-bootc metadata from domain XML - fn extract_podman_bootc_metadata(&self, xml: &str) -> Option { + /// Extract podman-bootc metadata from parsed domain XML + fn extract_podman_bootc_metadata( + &self, + dom: &xml_utils::XmlNode, + ) -> Result> { // Look for bootc metadata in the XML // This could be in various forms: // 1. in metadata section @@ -159,46 +165,55 @@ impl DomainLister { // 3. Domain description containing bcvk signature // Try to extract source image from bootc metadata - let source_image = extract_xml_value(xml, "bootc:source-image") - .or_else(|| extract_xml_value(xml, "source-image")); + let source_image = dom + .find("bootc:source-image") + .or_else(|| dom.find("source-image")) + .map(|node| node.text_content().to_string()); // Extract other metadata - let created = - extract_xml_value(xml, "bootc:created").or_else(|| extract_xml_value(xml, "created")); + let created = dom + .find("bootc:created") + .or_else(|| dom.find("created")) + .map(|node| node.text_content().to_string()); // Extract memory and vcpu from domain XML - let memory_mb = extract_xml_value(xml, "memory").and_then(|mem_str| { + let memory_mb = dom.find("memory").and_then(|node| { // Memory might have unit attribute, but we'll try to parse the value - mem_str.parse::().ok() + node.text_content().parse::().ok() }); - let vcpus = - extract_xml_value(xml, "vcpu").and_then(|vcpu_str| vcpu_str.parse::().ok()); + let vcpus = dom + .find("vcpu") + .and_then(|node| node.text_content().parse::().ok()); // Extract disk path from first disk device - let disk_path = extract_disk_path(xml); + let disk_path = extract_disk_path(&dom); - Some(PodmanBootcDomainMetadata { + Ok(Some(PodmanBootcDomainMetadata { source_image, created, memory_mb, vcpus, disk_path, - }) + })) } /// Check if a domain was created by bcvk libvirt - fn is_podman_bootc_domain(&self, _domain_name: &str, xml: &str) -> bool { + fn is_podman_bootc_domain(&self, _domain_name: &str, dom: &xml_utils::XmlNode) -> bool { // Only use XML metadata - domains created by bcvk libvirt should have bootc metadata - xml.contains("bootc:source-image") || xml.contains("bootc:container") + dom.find("bootc:source-image").is_some() + || dom.find("source-image").is_some() + || dom.find("bootc:container").is_some() } - /// Get detailed information about a domain - pub fn get_domain_info(&self, domain_name: &str) -> Result { + /// Get detailed information about a domain with pre-parsed XML + pub fn get_domain_info_from_xml( + &self, + domain_name: &str, + dom: &xml_utils::XmlNode, + ) -> Result { let state = self.get_domain_state(domain_name)?; - let xml = self.get_domain_xml(domain_name)?; - - let metadata = self.extract_podman_bootc_metadata(&xml); + let metadata = self.extract_podman_bootc_metadata(dom)?; Ok(PodmanBootcDomain { name: domain_name.to_string(), @@ -211,6 +226,12 @@ impl DomainLister { }) } + /// Get detailed information about a domain + pub fn get_domain_info(&self, domain_name: &str) -> Result { + let dom = self.get_domain_xml(domain_name)?; + self.get_domain_info_from_xml(domain_name, &dom) + } + /// List all bootc domains pub fn list_bootc_domains(&self) -> Result> { let all_domains = self.list_all_domains()?; @@ -219,9 +240,10 @@ impl DomainLister { for domain_name in all_domains { // Get domain XML to check if it's a podman-bootc domain match self.get_domain_xml(&domain_name) { - Ok(xml) => { - if self.is_podman_bootc_domain(&domain_name, &xml) { - match self.get_domain_info(&domain_name) { + Ok(dom) => { + if self.is_podman_bootc_domain(&domain_name, &dom) { + // Use the already-parsed DOM instead of re-parsing + match self.get_domain_info_from_xml(&domain_name, &dom) { Ok(domain_info) => podman_bootc_domains.push(domain_info), Err(e) => { eprintln!( @@ -264,55 +286,42 @@ struct PodmanBootcDomainMetadata { disk_path: Option, } -/// Extract value from XML element (simple string parsing) -fn extract_xml_value(xml: &str, element: &str) -> Option { - let start_tag = format!("<{}>", element); - let end_tag = format!("", element); - - if let Some(start_pos) = xml.find(&start_tag) { - let start = start_pos + start_tag.len(); - if let Some(end_pos) = xml[start..].find(&end_tag) { - let value = &xml[start..start + end_pos]; - return Some(value.trim().to_string()); - } - } +/// Extract disk path from domain XML using DOM parser +fn extract_disk_path(dom: &xml_utils::XmlNode) -> Option { + // Look for first disk device with type="file" + // We need to find: + find_disk_with_file_type(dom) + .and_then(|disk_node| disk_node.find("source")) + .and_then(|source_node| source_node.attributes.get("file")) + .map(|path| path.clone()) +} - // Also try with attributes (e.g., 2048) - let start_tag_with_attrs = format!("<{} ", element); - if let Some(start_pos) = xml.find(&start_tag_with_attrs) { - if let Some(close_pos) = xml[start_pos..].find('>') { - let start = start_pos + close_pos + 1; - if let Some(end_pos) = xml[start..].find(&end_tag) { - let value = &xml[start..start + end_pos]; - return Some(value.trim().to_string()); +/// Recursively find a disk element with type="file" +fn find_disk_with_file_type(node: &xml_utils::XmlNode) -> Option<&xml_utils::XmlNode> { + if node.name == "disk" { + if let Some(disk_type) = node.attributes.get("type") { + if disk_type == "file" { + return Some(node); } } } - None -} - -/// Extract disk path from domain XML -fn extract_disk_path(xml: &str) -> Option { - // Look for first disk device with type="file" - if let Some(disk_start) = xml.find(" 2048 @@ -323,13 +332,25 @@ mod tests { "#; - assert_eq!(extract_xml_value(xml, "memory"), Some("2048".to_string())); - assert_eq!(extract_xml_value(xml, "vcpu"), Some("4".to_string())); + let dom = xml_utils::parse_xml_dom(xml).unwrap(); assert_eq!( - extract_xml_value(xml, "bootc:source-image"), + dom.find("memory").map(|n| n.text_content().to_string()), + Some("2048".to_string()) + ); + assert_eq!( + dom.find("vcpu").map(|n| n.text_content().to_string()), + Some("4".to_string()) + ); + assert_eq!( + dom.find("bootc:source-image") + .map(|n| n.text_content().to_string()), Some("quay.io/fedora/fedora-bootc:40".to_string()) ); - assert_eq!(extract_xml_value(xml, "nonexistent"), None); + assert_eq!( + dom.find("nonexistent") + .map(|n| n.text_content().to_string()), + None + ); } #[test] @@ -346,8 +367,9 @@ mod tests { "#; + let dom = xml_utils::parse_xml_dom(xml).unwrap(); assert_eq!( - extract_disk_path(xml), + extract_disk_path(&dom), Some("/var/lib/libvirt/images/test.raw".to_string()) ); } diff --git a/crates/kit/src/libvirt/create.rs b/crates/kit/src/libvirt/create.rs index 954c07d89..b70f135a0 100644 --- a/crates/kit/src/libvirt/create.rs +++ b/crates/kit/src/libvirt/create.rs @@ -12,6 +12,7 @@ use crate::libvirt::upload::LibvirtUploadOpts; use crate::run_ephemeral::default_vcpus; use crate::ssh::generate_ssh_keypair; use crate::sshcred::smbios_cred_for_root_ssh; +use crate::xml_utils; use base64::Engine; use camino::{Utf8Path, Utf8PathBuf}; use clap::Parser; @@ -297,9 +298,15 @@ impl LibvirtCreateOpts { let xml = String::from_utf8(output.stdout)?; debug!("Volume XML: {}", xml); - // Parse XML to extract bootc metadata - // For simplicity, using string parsing - could use proper XML parser - let source_image = extract_xml_value(&xml, "bootc:source-image"); + // Parse XML to extract bootc metadata using DOM parser + let dom = xml_utils::parse_xml_dom(&xml) + .map_err(|e| eyre!("Failed to parse volume XML: {}", e))?; + + let source_image = dom + .find("bootc:source-image") + .or_else(|| dom.find("source-image")) + .map(|node| node.text_content().to_string()); + Ok(BootcVolumeMetadata { source_image }) } @@ -668,21 +675,6 @@ impl LibvirtCreateOpts { } } -/// Extract value from XML element (simple string parsing) -fn extract_xml_value(xml: &str, element: &str) -> Option { - let start_tag = format!("<{}>", element); - let end_tag = format!("", element); - - if let Some(start_pos) = xml.find(&start_tag) { - let start = start_pos + start_tag.len(); - if let Some(end_pos) = xml[start..].find(&end_tag) { - let value = &xml[start..start + end_pos]; - return Some(value.trim().to_string()); - } - } - None -} - /// Execute the libvirt domain creation process pub fn run(opts: LibvirtCreateOpts) -> Result<()> { debug!( diff --git a/crates/kit/src/libvirt/domain.rs b/crates/kit/src/libvirt/domain.rs index d6c5c9151..f320226c0 100644 --- a/crates/kit/src/libvirt/domain.rs +++ b/crates/kit/src/libvirt/domain.rs @@ -6,6 +6,7 @@ use crate::arch::ArchConfig; use crate::common_opts::DEFAULT_MEMORY_USER_STR; use crate::run_ephemeral::default_vcpus; +use crate::xml_utils::XmlWriter; use color_eyre::{eyre::eyre, Result}; use std::collections::HashMap; use uuid::Uuid; @@ -116,99 +117,74 @@ impl DomainBuilder { // Detect architecture configuration let arch_config = ArchConfig::detect()?; - let mut xml = if self.qemu_args.is_empty() { - format!( - r#" - {} - {} - {} - {} - {} - - {} - "#, - name, - uuid, - memory, - memory, - vcpus, - arch_config.arch, - arch_config.machine, - arch_config.os_type - ) + let mut writer = XmlWriter::new(); + + // Root domain element + let domain_attrs = if self.qemu_args.is_empty() { + vec![("type", "kvm")] } else { - format!( - r#" - {} - {} - {} - {} - {} - - {} - "#, - name, - uuid, - memory, - memory, - vcpus, - arch_config.arch, - arch_config.machine, - arch_config.os_type - ) + vec![ + ("type", "kvm"), + ("xmlns:qemu", "http://libvirt.org/schemas/domain/qemu/1.0"), + ] }; + writer.start_element("domain", &domain_attrs)?; + + // Basic domain information + writer.write_text_element("name", &name)?; + writer.write_text_element("uuid", &uuid)?; + writer.write_text_element_with_attrs("memory", &memory.to_string(), &[("unit", "MiB")])?; + writer.write_text_element_with_attrs( + "currentMemory", + &memory.to_string(), + &[("unit", "MiB")], + )?; + writer.write_text_element("vcpu", &vcpus.to_string())?; + + // OS section + writer.start_element("os", &[])?; + writer.write_text_element_with_attrs( + "type", + &arch_config.os_type, + &[ + ("arch", &arch_config.arch), + ("machine", &arch_config.machine), + ], + )?; + writer.write_empty_element("boot", &[("dev", "hd")])?; // Add kernel arguments if specified (for direct boot) if let Some(ref kargs) = self.kernel_args { - xml.push_str(&format!("\n {}", kargs)); + writer.write_text_element("cmdline", kargs)?; } - xml.push_str("\n "); + writer.end_element("os")?; // Architecture-specific features - xml.push_str(arch_config.xml_features()); + arch_config.write_features(&mut writer)?; // Architecture-specific CPU configuration - xml.push_str(&format!( - r#" - "#, - arch_config.cpu_mode() - )); + writer.write_empty_element("cpu", &[("mode", arch_config.cpu_mode())])?; // Clock and lifecycle configuration - xml.push_str( - r#" - "#, - ); + writer.start_element("clock", &[("offset", "utc")])?; + arch_config.write_timers(&mut writer)?; + writer.end_element("clock")?; - // Architecture-specific timers - xml.push_str(arch_config.xml_timers()); - - xml.push_str( - r#" - - destroy - restart - destroy"#, - ); + writer.write_text_element("on_poweroff", "destroy")?; + writer.write_text_element("on_reboot", "restart")?; + writer.write_text_element("on_crash", "destroy")?; // Devices section - xml.push_str("\n "); - - // Let libvirt automatically detect the appropriate emulator - // based on the architecture and domain type + writer.start_element("devices", &[])?; // Disk if let Some(ref disk_path) = self.disk_path { - xml.push_str(&format!( - r#" - - - - - "#, - disk_path - )); + writer.start_element("disk", &[("type", "file"), ("device", "disk")])?; + writer.write_empty_element("driver", &[("name", "qemu"), ("type", "raw")])?; + writer.write_empty_element("source", &[("file", disk_path)])?; + writer.write_empty_element("target", &[("dev", "vda"), ("bus", "virtio")])?; + writer.end_element("disk")?; } // Network @@ -223,94 +199,86 @@ impl DomainBuilder { } "user" => { // User-mode networking (NAT) - no network name required - xml.push_str( - r#" - - - "#, - ); + writer.start_element("interface", &[("type", "user")])?; + writer.write_empty_element("model", &[("type", "virtio")])?; + writer.end_element("interface")?; } network if network.starts_with("bridge=") => { let bridge_name = &network[7..]; // Remove "bridge=" prefix - xml.push_str(&format!( - r#" - - - - "#, - bridge_name - )); + writer.start_element("interface", &[("type", "bridge")])?; + writer.write_empty_element("source", &[("bridge", bridge_name)])?; + writer.write_empty_element("model", &[("type", "virtio")])?; + writer.end_element("interface")?; } _ => { // Assume it's a network name - xml.push_str(&format!( - r#" - - - - "#, - network_config - )); + writer.start_element("interface", &[("type", "network")])?; + writer.write_empty_element("source", &[("network", network_config)])?; + writer.write_empty_element("model", &[("type", "virtio")])?; + writer.end_element("interface")?; } } // Serial console - xml.push_str( - r#" - - - - - - "#, - ); + writer.start_element("serial", &[("type", "pty")])?; + writer.write_empty_element("target", &[("port", "0")])?; + writer.end_element("serial")?; + + writer.start_element("console", &[("type", "pty")])?; + writer.write_empty_element("target", &[("type", "serial"), ("port", "0")])?; + writer.end_element("console")?; // VNC graphics if enabled if let Some(vnc_port) = self.vnc_port { - xml.push_str(&format!( - r#" - - "#, - vnc_port - )); + writer.write_empty_element( + "graphics", + &[ + ("type", "vnc"), + ("port", &vnc_port.to_string()), + ("listen", "127.0.0.1"), + ], + )?; + writer.start_element("video", &[])?; + writer.write_empty_element("model", &[("type", "vga")])?; + writer.end_element("video")?; } - xml.push_str("\n "); + writer.end_element("devices")?; // QEMU commandline section (if we have QEMU args) if !self.qemu_args.is_empty() { - xml.push_str("\n "); + writer.start_element("qemu:commandline", &[])?; for arg in &self.qemu_args { - xml.push_str(&format!("\n ", arg)); + writer.write_empty_element("qemu:arg", &[("value", arg)])?; } - xml.push_str("\n "); + writer.end_element("qemu:commandline")?; } // Metadata section if !self.metadata.is_empty() { - xml.push_str("\n "); - xml.push_str( - "\n ", - ); + writer.start_element("metadata", &[])?; + writer.start_element( + "bootc:container", + &[("xmlns:bootc", "https://github.com/containers/bootc")], + )?; for (key, value) in &self.metadata { - // Strip bootc: prefix if present for cleaner XML - let clean_key = key.strip_prefix("bootc:").unwrap_or(key); - xml.push_str(&format!( - "\n {}", - clean_key, value, clean_key - )); + // Ensure the key has the bootc: prefix + let element_name = if key.starts_with("bootc:") { + key.clone() + } else { + format!("bootc:{}", key) + }; + writer.write_text_element(&element_name, value)?; } - xml.push_str("\n "); - xml.push_str("\n "); + writer.end_element("bootc:container")?; + writer.end_element("metadata")?; } - xml.push_str("\n"); + writer.end_element("domain")?; - Ok(xml) + writer.into_string() } } @@ -411,7 +379,8 @@ mod tests { match host_arch { "x86_64" => { assert!(xml.contains("machine=\"q35\"")); - assert!(xml.contains("vmport state='off'")); // x86_64-specific feature + assert!(xml.contains("vmport")); // x86_64-specific feature + assert!(xml.contains("state=\"off\"")); // vmport should be disabled } "aarch64" => { assert!(xml.contains("machine=\"virt\"")); @@ -425,6 +394,6 @@ mod tests { // Should contain architecture-specific features and timers assert!(xml.contains("")); assert!(xml.contains("")); - assert!(xml.contains(" Option { - let start_tag = format!("<{}>", element); - let end_tag = format!("", element); - - if let Some(start_pos) = xml.find(&start_tag) { - let start = start_pos + start_tag.len(); - if let Some(end_pos) = xml[start..].find(&end_tag) { - let value = &xml[start..start + end_pos]; - return Some(value.trim().to_string()); - } - } - None -} /// Parse virsh size format (e.g., "5.00 GiB") to bytes fn parse_virsh_size(size_str: &str) -> Option { diff --git a/crates/kit/src/libvirt/run.rs b/crates/kit/src/libvirt/run.rs index 515bf4196..6d9d2d9d8 100644 --- a/crates/kit/src/libvirt/run.rs +++ b/crates/kit/src/libvirt/run.rs @@ -13,6 +13,7 @@ use std::hash::{Hash, Hasher}; use crate::common_opts::MemoryOpts; use crate::domain_list::DomainLister; use crate::utils::parse_memory_to_mb; +use crate::xml_utils; /// Options for creating and running a bootable container VM #[derive(Debug, Parser)] @@ -188,16 +189,13 @@ fn get_libvirt_storage_pool_path() -> Result { let xml = String::from_utf8(output.stdout).with_context(|| "Invalid UTF-8 in virsh output")?; - // Extract path from XML - // Looking for: /some/path - let start_tag = ""; - let end_tag = ""; + // 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(start_pos) = xml.find(start_tag) { - let start = start_pos + start_tag.len(); - if let Some(end_pos) = xml[start..].find(end_tag) { - let path_str = &xml[start..start + end_pos]; - return Ok(Utf8PathBuf::from(path_str.trim())); + 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)); } } diff --git a/crates/kit/src/libvirt/ssh.rs b/crates/kit/src/libvirt/ssh.rs index dae626a31..1ac15407a 100644 --- a/crates/kit/src/libvirt/ssh.rs +++ b/crates/kit/src/libvirt/ssh.rs @@ -4,6 +4,7 @@ //! with SSH key injection, automatically retrieving SSH credentials from domain XML //! metadata and establishing connection using embedded private keys. +use crate::xml_utils; use base64::Engine; use clap::Parser; use color_eyre::{eyre::eyre, Result}; @@ -98,22 +99,27 @@ impl LibvirtSshOpts { let xml = String::from_utf8(output.stdout)?; debug!("Domain XML for SSH extraction: {}", xml); + // Parse XML once for all metadata extraction + let dom = xml_utils::parse_xml_dom(&xml) + .map_err(|e| eyre!("Failed to parse domain XML: {}", e))?; + // Extract SSH metadata from bootc:container section // First try the new base64 encoded format - let private_key = if let Some(encoded_key) = - extract_xml_metadata(&xml, "ssh-private-key-base64") + let private_key = if let Some(encoded_key_node) = + dom.find_with_namespace("ssh-private-key-base64") { + let encoded_key = encoded_key_node.text_content(); debug!("Found base64 encoded SSH private key"); // Decode base64 encoded private key let decoded_bytes = base64::engine::general_purpose::STANDARD - .decode(&encoded_key) + .decode(encoded_key) .map_err(|e| eyre!("Failed to decode base64 SSH private key: {}", e))?; String::from_utf8(decoded_bytes) .map_err(|e| eyre!("SSH private key contains invalid UTF-8: {}", e))? - } else if let Some(legacy_key) = extract_xml_metadata(&xml, "ssh-private-key") { + } else if let Some(legacy_key_node) = dom.find_with_namespace("ssh-private-key") { debug!("Found legacy plain text SSH private key"); - legacy_key + legacy_key_node.text_content().to_string() } else { return Err(eyre!("No SSH private key found in domain '{}' metadata. Domain was not created with --generate-ssh-key or --ssh-key.", self.domain_name)); }; @@ -170,7 +176,7 @@ impl LibvirtSshOpts { )); } - let ssh_port_str = extract_xml_metadata(&xml, "ssh-port").ok_or_else(|| { + let ssh_port_str = dom.find_with_namespace("ssh-port").ok_or_else(|| { eyre!( "No SSH port found in domain '{}' metadata", self.domain_name @@ -178,12 +184,14 @@ impl LibvirtSshOpts { })?; let ssh_port = ssh_port_str + .text_content() .parse::() - .map_err(|e| eyre!("Invalid SSH port '{}': {}", ssh_port_str, e))?; + .map_err(|e| eyre!("Invalid SSH port '{}': {}", ssh_port_str.text_content(), e))?; - let is_generated = extract_xml_metadata(&xml, "ssh-generated") - .unwrap_or_else(|| "false".to_string()) - == "true"; + let is_generated = dom + .find_with_namespace("ssh-generated") + .map(|node| node.text_content() == "true") + .unwrap_or(false); Ok(DomainSshConfig { private_key_content: private_key, @@ -320,21 +328,6 @@ impl LibvirtSshOpts { } } -/// Extract metadata value from domain XML bootc:container section -fn extract_xml_metadata(xml: &str, key: &str) -> Option { - let start_tag = format!("", key); - let end_tag = format!("", key); - - if let Some(start_pos) = xml.find(&start_tag) { - let start = start_pos + start_tag.len(); - if let Some(end_pos) = xml[start..].find(&end_tag) { - let value = &xml[start..start + end_pos]; - return Some(value.trim().to_string()); - } - } - None -} - /// Execute the libvirt SSH command pub fn run(opts: LibvirtSshOpts) -> Result<()> { debug!("Connecting to libvirt domain: {}", opts.domain_name); @@ -369,7 +362,7 @@ mod tests { use super::*; #[test] - fn test_extract_xml_metadata() { + fn test_ssh_metadata_extraction() { let xml = r#" @@ -382,21 +375,30 @@ mod tests { "#; + let dom = xml_utils::parse_xml_dom(xml).unwrap(); + assert_eq!( - extract_xml_metadata(xml, "ssh-private-key"), + dom.find_with_namespace("ssh-private-key") + .map(|n| n.text_content().to_string()), Some("-----BEGIN OPENSSH PRIVATE KEY-----".to_string()) ); assert_eq!( - extract_xml_metadata(xml, "ssh-port"), + dom.find_with_namespace("ssh-port") + .map(|n| n.text_content().to_string()), Some("2222".to_string()) ); assert_eq!( - extract_xml_metadata(xml, "ssh-generated"), + dom.find_with_namespace("ssh-generated") + .map(|n| n.text_content().to_string()), Some("true".to_string()) ); - assert_eq!(extract_xml_metadata(xml, "nonexistent"), None); + assert_eq!( + dom.find_with_namespace("nonexistent") + .map(|n| n.text_content().to_string()), + None + ); } } diff --git a/crates/kit/src/libvirt_upload_disk.rs b/crates/kit/src/libvirt_upload_disk.rs index 715c0ceb8..b9eb8a6e0 100644 --- a/crates/kit/src/libvirt_upload_disk.rs +++ b/crates/kit/src/libvirt_upload_disk.rs @@ -6,6 +6,7 @@ use crate::common_opts::MemoryOpts; use crate::install_options::InstallOptions; use crate::to_disk::{run as to_disk, ToDiskOpts}; +use crate::xml_utils::{self, XmlWriter}; use crate::{images, utils}; use camino::Utf8Path; use clap::Parser; @@ -153,20 +154,23 @@ impl LibvirtUploadDiskOpts { debug!("Adding container image metadata to volume"); - // Create XML with metadata - let metadata_xml = format!( - r#" - - {} - {} - {} - 1.0.0 - -"#, - self.source_image, + // Create XML with metadata using XmlWriter + let mut writer = XmlWriter::new(); + writer.start_element("metadata", &[])?; + writer.start_element( + "bootc:container", + &[("xmlns:bootc", "https://github.com/containers/bootc")], + )?; + writer.write_text_element("bootc:source-image", &self.source_image)?; + writer.write_text_element( + "bootc:filesystem", self.install.filesystem.as_deref().unwrap_or("default"), - chrono::Utc::now().to_rfc3339() - ); + )?; + writer.write_text_element("bootc:created", &chrono::Utc::now().to_rfc3339())?; + writer.write_text_element("bootc:bcvk-version", "1.0.0")?; + writer.end_element("bootc:container")?; + writer.end_element("metadata")?; + let metadata_xml = writer.into_string()?; // Write metadata to temp file let temp_metadata = std::env::temp_dir().join("volume-metadata.xml"); @@ -193,31 +197,54 @@ impl LibvirtUploadDiskOpts { .output()?; if output.status.success() { - let mut xml = String::from_utf8(output.stdout)?; - - // Insert metadata before closing tag - let metadata = format!( - r#" - - {} - {} - {} - 1.0.0 - - -"#, - self.source_image, + let xml = String::from_utf8(output.stdout)?; + + // Parse the existing volume XML using DOM parser + let dom = xml_utils::parse_xml_dom(&xml)?; + + // Create new XML with metadata using XmlWriter + let mut writer = XmlWriter::new(); + + // Start volume element with attributes from original + let volume_attrs = dom + .attributes + .iter() + .map(|(k, v)| (k.as_str(), v.as_str())) + .collect::>(); + writer.start_element("volume", &volume_attrs)?; + + // Copy existing elements (name, capacity, allocation, target, etc.) + for child in &dom.children { + if child.name != "metadata" { + write_xml_node(&mut writer, child)?; + } + } + + // Add bootc metadata + writer.start_element("metadata", &[])?; + writer.start_element( + "bootc:container", + &[("xmlns:bootc", "https://github.com/containers/bootc")], + )?; + writer.write_text_element("bootc:source-image", &self.source_image)?; + writer.write_text_element( + "bootc:filesystem", self.install.filesystem.as_deref().unwrap_or("default"), - chrono::Utc::now().to_rfc3339() - ); + )?; + writer.write_text_element("bootc:created", &chrono::Utc::now().to_rfc3339())?; + writer.write_text_element("bootc:bcvk-version", "1.0.0")?; + writer.end_element("bootc:container")?; + writer.end_element("metadata")?; - xml = xml.replace("", &metadata); + // Close volume element + writer.end_element("volume")?; + let new_xml = writer.into_string()?; // Save modified XML let temp_xml = std::env::temp_dir().join("volume-with-metadata.xml"); - std::fs::write(&temp_xml, xml)?; + std::fs::write(&temp_xml, new_xml)?; - debug!("Added metadata to volume XML"); + debug!("Added metadata to volume XML using DOM parser"); } // Clean up temp file @@ -299,3 +326,32 @@ pub fn run(opts: LibvirtUploadDiskOpts) -> Result<()> { Ok(()) } + +/// Helper function to recursively write an XML node and its children using XmlWriter +fn write_xml_node(writer: &mut XmlWriter, node: &xml_utils::XmlNode) -> Result<()> { + let attrs = node + .attributes + .iter() + .map(|(k, v)| (k.as_str(), v.as_str())) + .collect::>(); + + if node.children.is_empty() && node.text.is_empty() { + // Empty element + writer.write_empty_element(&node.name, &attrs)?; + } else if node.children.is_empty() { + // Simple text element + writer.write_text_element_with_attrs(&node.name, &node.text, &attrs)?; + } else { + // Element with children + writer.start_element(&node.name, &attrs)?; + if !node.text.is_empty() { + writer.write_text(&node.text)?; + } + for child in &node.children { + write_xml_node(writer, child)?; + } + writer.end_element(&node.name)?; + } + + Ok(()) +} diff --git a/crates/kit/src/main.rs b/crates/kit/src/main.rs index 18bfb25ed..828238623 100644 --- a/crates/kit/src/main.rs +++ b/crates/kit/src/main.rs @@ -32,6 +32,7 @@ mod supervisor_status; pub(crate) mod systemd; mod to_disk; mod utils; +mod xml_utils; pub const CONTAINER_STATEDIR: &str = "/var/lib/bcvk"; diff --git a/crates/kit/src/xml_utils.rs b/crates/kit/src/xml_utils.rs new file mode 100644 index 000000000..c78d4738f --- /dev/null +++ b/crates/kit/src/xml_utils.rs @@ -0,0 +1,373 @@ +//! XML utilities using quick-xml for efficient XML generation and parsing +//! +//! This module provides helper functions for generating and parsing XML using the quick-xml crate, +//! replacing string-based XML manipulation with proper XML handling. + +use color_eyre::{eyre::eyre, Result}; +use quick_xml::events::{BytesEnd, BytesStart, BytesText, Event}; +use quick_xml::reader::Reader; +use quick_xml::writer::Writer; +use std::collections::HashMap; +use std::io::Cursor; + +/// A builder for creating XML documents with quick-xml +pub struct XmlWriter { + writer: Writer>>, +} + +impl XmlWriter { + /// Create a new XML writer + pub fn new() -> Self { + Self { + writer: Writer::new(Cursor::new(Vec::new())), + } + } + + /// Start an XML element with attributes + pub fn start_element(&mut self, name: &str, attributes: &[(&str, &str)]) -> Result<()> { + let mut elem = BytesStart::new(name); + for (key, value) in attributes { + elem.push_attribute((*key, *value)); + } + self.writer + .write_event(Event::Start(elem)) + .map_err(|e| eyre!("Failed to write start element: {}", e))?; + Ok(()) + } + + /// Write a simple element with text content + pub fn write_text_element(&mut self, name: &str, text: &str) -> Result<()> { + self.start_element(name, &[])?; + self.write_text(text)?; + self.end_element(name)?; + Ok(()) + } + + /// Write a simple element with text content and attributes + pub fn write_text_element_with_attrs( + &mut self, + name: &str, + text: &str, + attributes: &[(&str, &str)], + ) -> Result<()> { + self.start_element(name, attributes)?; + if !text.is_empty() { + self.write_text(text)?; + } + self.end_element(name)?; + Ok(()) + } + + /// Write a self-closing element with attributes + pub fn write_empty_element(&mut self, name: &str, attributes: &[(&str, &str)]) -> Result<()> { + let mut elem = BytesStart::new(name); + for (key, value) in attributes { + elem.push_attribute((*key, *value)); + } + self.writer + .write_event(Event::Empty(elem)) + .map_err(|e| eyre!("Failed to write empty element: {}", e))?; + Ok(()) + } + + /// Write text content + pub fn write_text(&mut self, text: &str) -> Result<()> { + if !text.is_empty() { + self.writer + .write_event(Event::Text(BytesText::new(text))) + .map_err(|e| eyre!("Failed to write text: {}", e))?; + } + Ok(()) + } + + /// End an XML element + pub fn end_element(&mut self, name: &str) -> Result<()> { + self.writer + .write_event(Event::End(BytesEnd::new(name))) + .map_err(|e| eyre!("Failed to write end element: {}", e))?; + Ok(()) + } + + /// Get the generated XML as a string + pub fn into_string(self) -> Result { + let bytes = self.writer.into_inner().into_inner(); + String::from_utf8(bytes).map_err(|e| eyre!("Failed to convert XML to string: {}", e)) + } +} + +impl Default for XmlWriter { + fn default() -> Self { + Self::new() + } +} + +/// Simple DOM node for XML parsing +#[derive(Debug, Clone)] +pub struct XmlNode { + pub name: String, + pub attributes: HashMap, + pub text: String, + pub children: Vec, +} + +impl XmlNode { + /// Find first element by name (recursive search) + pub fn find(&self, element_name: &str) -> Option<&XmlNode> { + if self.name == element_name { + return Some(self); + } + + for child in &self.children { + if let Some(found) = child.find(element_name) { + return Some(found); + } + } + + None + } + + /// Find first element by name with namespace fallback + pub fn find_with_namespace(&self, element_name: &str) -> Option<&XmlNode> { + // Try namespaced version first + if let Some(found) = self.find(&format!("bootc:{}", element_name)) { + return Some(found); + } + // Fallback to non-namespaced + self.find(element_name) + } + + /// Get text content of this node + pub fn text_content(&self) -> &str { + &self.text + } +} + +/// Parse XML string into a simple DOM structure +pub fn parse_xml_dom(xml: &str) -> Result { + let mut reader = Reader::from_str(xml); + reader.config_mut().trim_text(true); + let mut buf = Vec::new(); + + let mut stack: Vec = Vec::new(); + let mut root: Option = None; + + loop { + match reader.read_event_into(&mut buf) { + Ok(Event::Start(e)) => { + let name = String::from_utf8_lossy(e.name().as_ref()).into_owned(); + let mut attributes = HashMap::new(); + + for attr in e.attributes() { + if let Ok(attr) = attr { + let key = String::from_utf8_lossy(attr.key.as_ref()).into_owned(); + let value = String::from_utf8_lossy(&attr.value).into_owned(); + attributes.insert(key, value); + } + } + + let node = XmlNode { + name, + attributes, + text: String::new(), + children: Vec::new(), + }; + + stack.push(node); + } + Ok(Event::Empty(e)) => { + let name = String::from_utf8_lossy(e.name().as_ref()).into_owned(); + let mut attributes = HashMap::new(); + + for attr in e.attributes() { + if let Ok(attr) = attr { + let key = String::from_utf8_lossy(attr.key.as_ref()).into_owned(); + let value = String::from_utf8_lossy(&attr.value).into_owned(); + attributes.insert(key, value); + } + } + + let node = XmlNode { + name, + attributes, + text: String::new(), + children: Vec::new(), + }; + + // Add to parent or set as root + if let Some(parent) = stack.last_mut() { + parent.children.push(node); + } else if root.is_none() { + root = Some(node); + } + } + Ok(Event::End(_)) => { + if let Some(completed_node) = stack.pop() { + if let Some(parent) = stack.last_mut() { + parent.children.push(completed_node); + } else { + root = Some(completed_node); + } + } + } + Ok(Event::Text(e)) => { + if let Ok(text) = e.unescape() { + if let Some(current) = stack.last_mut() { + current.text.push_str(&text); + } + } + } + Ok(Event::Eof) => break, + Err(e) => return Err(eyre!("Failed to parse XML: {}", e)), + _ => {} + } + buf.clear(); + } + + root.ok_or_else(|| eyre!("No root element found in XML")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_xml_writer_basic() { + let mut writer = XmlWriter::new(); + writer.start_element("root", &[]).unwrap(); + writer.write_text_element("name", "test").unwrap(); + writer + .write_text_element_with_attrs("memory", "4096", &[("unit", "MiB")]) + .unwrap(); + writer + .write_empty_element("disk", &[("type", "file")]) + .unwrap(); + writer.end_element("root").unwrap(); + + let xml = writer.into_string().unwrap(); + assert!(xml.contains("")); + assert!(xml.contains("test")); + assert!(xml.contains("4096")); + assert!(xml.contains("")); + assert!(xml.contains("")); + } + + #[test] + fn test_find_with_namespace() { + let xml = r#" + + + + quay.io/fedora/fedora-bootc:42 + xfs + + + + "#; + + let dom = parse_xml_dom(xml).unwrap(); + + assert_eq!( + dom.find_with_namespace("source-image") + .map(|n| n.text_content().to_string()), + Some("quay.io/fedora/fedora-bootc:42".to_string()) + ); + assert_eq!( + dom.find_with_namespace("filesystem") + .map(|n| n.text_content().to_string()), + Some("xfs".to_string()) + ); + assert_eq!( + dom.find_with_namespace("nonexistent") + .map(|n| n.text_content().to_string()), + None + ); + } + + #[test] + fn test_xml_writer_complex() { + let mut writer = XmlWriter::new(); + writer.start_element("domain", &[("type", "kvm")]).unwrap(); + writer.write_text_element("name", "test-domain").unwrap(); + writer + .write_text_element_with_attrs("memory", "4096", &[("unit", "MiB")]) + .unwrap(); + + // Test nested elements + writer.start_element("devices", &[]).unwrap(); + writer + .write_empty_element("disk", &[("type", "file"), ("device", "disk")]) + .unwrap(); + writer + .start_element("interface", &[("type", "network")]) + .unwrap(); + writer + .write_empty_element("source", &[("network", "default")]) + .unwrap(); + writer.end_element("interface").unwrap(); + writer.end_element("devices").unwrap(); + + writer.end_element("domain").unwrap(); + + let xml = writer.into_string().unwrap(); + assert!(xml.contains("")); + assert!(xml.contains("")); + assert!(xml.contains("")); + assert!(xml.contains("")); + assert!(xml.contains("")); + assert!(xml.contains("")); + assert!(xml.contains("")); + assert!(xml.contains("")); + } + + #[test] + fn test_xml_writer_empty_text() { + let mut writer = XmlWriter::new(); + writer.start_element("root", &[]).unwrap(); + writer.write_text_element("empty", "").unwrap(); + writer + .write_text_element_with_attrs("empty-with-attrs", "", &[("type", "test")]) + .unwrap(); + writer.end_element("root").unwrap(); + + let xml = writer.into_string().unwrap(); + assert!(xml.contains("")); + assert!(xml.contains("")); + } + + #[test] + fn test_find_with_namespace_edge_cases() { + // Test with both namespaced and non-namespaced elements + let xml = r#" + + + + namespaced-image + non-namespaced-image + + + + "#; + + let dom = parse_xml_dom(xml).unwrap(); + + // Should find the namespaced version first + assert_eq!( + dom.find_with_namespace("source-image") + .map(|n| n.text_content().to_string()), + Some("namespaced-image".to_string()) + ); + } + + #[test] + fn test_xml_writer_nested_elements() { + let mut writer = XmlWriter::new(); + writer.start_element("root", &[]).unwrap(); + writer.write_text_element("custom", "raw content").unwrap(); + writer.end_element("root").unwrap(); + + let xml = writer.into_string().unwrap(); + assert!(xml.contains("")); + assert!(xml.contains("raw content")); + assert!(xml.contains("")); + } +}