libvirt: Disable firmware debug log (isa-debugcon) by default

[cgwalters]

Previously I added isa-debugcon when debugging a secure boot issue, but it slows down boot significantly if it's found. Make it an opt in thing.

Assisted-by: OpenCode (Claude Opus 4)

[gemini-code-assist]

Code Review

This pull request disables the firmware debug log by default to improve boot performance, making it an opt-in feature via a new --firmware-log command-line flag. The changes are logical and align with the goal. However, I've identified two areas for improvement: the complete removal of related tests, which should be updated instead, and a slightly convoluted implementation of the conditional logic for the new flag. My review includes suggestions to address both of these points.

I am having trouble creating individual review comments. Click here to see my feedback.

crates/kit/src/libvirt/domain.rs (780-831)

Removing tests is generally discouraged as it reduces test coverage and increases the risk of future regressions. Instead of deleting these tests, they should be updated to reflect the new default behavior (firmware log disabled) and to test the new opt-in functionality. I've provided a suggestion to restore and update the tests accordingly.

    #[test]
    fn test_firmware_log_default_disabled() {
        // By default, firmware log should be disabled
        let xml = DomainBuilder::new()
            .with_name("test-firmware-log-default-disabled")
            .build_xml()
            .unwrap();

        // On x86_64, should NOT have isa-debugcon serial device
        if std::env::consts::ARCH == "x86_64" {
            assert!(!xml.contains("isa-debugcon"));
        }
    }

    #[test]
    fn test_firmware_log_console() {
        // Test enabling firmware log to console
        let xml = DomainBuilder::new()
            .with_name("test-firmware-log-console")
            .with_firmware_log(FirmwareLogOutput::Console)
            .build_xml()
            .unwrap();

        // On x86_64, should have isa-debugcon serial device
        if std::env::consts::ARCH == "x86_64" {
            assert!(xml.contains("serial type=\"pty\""));
            assert!(xml.contains("target type=\"isa-debug\""));
            assert!(xml.contains("model name=\"isa-debugcon\""));
            assert!(xml.contains("address type=\"isa\" iobase=\"0x402\""));
        }
    }

    #[test]
    fn test_firmware_log_file() {
        // Test firmware log to file
        let xml = DomainBuilder::new()
            .with_name("test-firmware-log-file")
            .with_firmware_log(FirmwareLogOutput::File("/tmp/ovmf-debug.log".to_string()))
            .build_xml()
            .unwrap();

        // On x86_64, should have isa-debugcon with file output
        if std::env::consts::ARCH == "x86_64" {
            assert!(xml.contains("serial type=\"file\""));
            assert!(xml.contains("source path=\"/tmp/ovmf-debug.log\""));
            assert!(xml.contains("target type=\"isa-debug\""));
            assert!(xml.contains("model name=\"isa-debugcon\""));
            assert!(xml.contains("address type=\"isa\" iobase=\"0x402\""));
        }
    }

crates/kit/src/libvirt/run.rs (1098-1110)

The logic for conditionally adding the firmware log is a bit convoluted due to the variable renaming (builder then domain_builder). You can make this more straightforward by keeping the domain_builder variable throughout, which improves readability.

    let mut domain_builder = DomainBuilder::new()
        .with_name(domain_name)
        .with_memory(memory.into())
        .with_vcpus(cpus)
        .with_disk(disk_path.as_str())
        .with_transient_disk(opts.transient)
        .with_network("none") // Use QEMU args for SSH networking instead
        .with_firmware(opts.firmware)
        .with_tpm(!opts.disable_tpm);
    if opts.firmware_log {
        domain_builder = domain_builder.with_firmware_log(crate::libvirt::domain::FirmwareLogOutput::Console);
    }
    let mut domain_builder = domain_builder