libvirt run: Add port forwarding support

[ckyrouac]

Add --port/-p option to forward ports from the host to the VM guest. The option accepts host_port:guest_port format (e.g., --port 8080:80) and can be specified multiple times for forwarding multiple ports.

Implementation uses QEMU's user networking hostfwd parameter to forward TCP ports from the host to the VM. Port mappings are parsed and validated, with clear error messages for invalid formats.

The forwarded ports are displayed to the user when the VM is created, similar to how volume mounts are shown.

Assisted-by: Claude Code

---

I was trying out my nextcloud server with bootc this morning and needed this for testing. This is a one-shot claude prompt. Not sure the qemu args are optimal but this is working for my use case.

[gemini-code-assist]

Code Review

This pull request effectively adds port forwarding support to the libvirt run command. The implementation correctly builds the QEMU arguments for hostfwd and adds corresponding tests.

My review includes a few suggestions to improve the implementation:

• Refactoring the port mapping parsing to happen once at the beginning of the run command to improve efficiency and provide consistent, early error handling for invalid user input.
• Making the parse_port_mapping function more robust and idiomatic by using split_once and NonZeroU16.
• Correcting a compilation error in the new tests and enhancing them to cover more edge cases for port validation.

Overall, this is a great addition. Addressing these points will make the feature more robust and maintainable.

[gemini-code-assist]

This test, along with test_parse_port_mapping_invalid_host_port and test_parse_port_mapping_invalid_guest_port, will fail to compile due to a 'use of moved value' error. result.unwrap_err() consumes result, which cannot be used after assert!(result.is_err()) is called. You can simplify these tests to unwrap the error directly and assert on its contents.

    fn test_parse_port_mapping_invalid_format() {
        let err = parse_port_mapping("8080").unwrap_err();
        assert!(err
            .to_string()
            .contains("Expected format: host_port:guest_port"));
    }

[gemini-code-assist]

This test has the same 'use of moved value' compilation error as the one above. It should be simplified to unwrap the error and then assert on its contents.

    fn test_parse_port_mapping_invalid_host_port() {
        let err = parse_port_mapping("abc:80").unwrap_err();
        assert!(err.to_string().contains("Invalid host port"));
    }

[gemini-code-assist]

This test has the same 'use of moved value' compilation error as the ones above. It should be simplified to unwrap the error and then assert on its contents.

    fn test_parse_port_mapping_invalid_guest_port() {
        let err = parse_port_mapping("8080:xyz").unwrap_err();
        assert!(err
            .to_string()
            .contains("Invalid guest port"));
    }

[gemini-code-assist]

This block has two issues:

1. Inconsistent Error Handling: Invalid port mappings are silently ignored here, but will cause the program to fail later when building QEMU arguments. This provides a poor user experience. The program should fail fast with a clear error message.
2. Redundant Parsing: The port mapping strings are parsed here for display, and then parsed again inside create_libvirt_domain_from_disk. This is inefficient.

I recommend refactoring to parse the port mappings once at the beginning of the run function. You can collect the parsed mappings into a Vec<(u16, u16)>. This validates all inputs upfront and provides a single source of truth for the port mappings, which can then be used for both display and for domain creation. This would require updating the signature of create_libvirt_domain_from_disk to accept the parsed mappings.

[gemini-code-assist]

This function can be made more idiomatic and robust.

1. Using split_once is generally preferred over splitn and collect when you expect exactly two parts. It's more efficient and expresses the intent more clearly.
2. The error message states that ports must be between 1 and 65535, but parsing to u16 allows 0. To enforce this and make the code more robust, you can parse to std::num::NonZeroU16.

fn parse_port_mapping(port_str: &str) -> Result<(u16, u16)> {
    let (host_str, guest_str) = port_str.split_once(':').ok_or_else(|| {
        color_eyre::eyre::eyre!(
            "Invalid port format '{}'. Expected format: host_port:guest_port",
            port_str
        )
    })?;

    let host_port = host_str.trim().parse::<std::num::NonZeroU16>().map_err(|_| {
        color_eyre::eyre::eyre!(
            "Invalid host port '{}'. Must be a number between 1 and 65535",
            host_str
        )
    })?;

    let guest_port = guest_str.trim().parse::<std::num::NonZeroU16>().map_err(|_| {
        color_eyre::eyre::eyre!(
            "Invalid guest port '{}'. Must be a number between 1 and 65535",
            guest_str
        )
    })?;

    Ok((host_port.get(), guest_port.get()))
}

[gemini-code-assist]

This assertion is good, but it could be more specific. To ensure the correct validation is triggered, consider asserting on the content of the error message, similar to the other tests. This makes the test more robust against future refactoring.

Additionally, it would be beneficial to add test cases for port 0 (e.g., "0:80") and for the valid upper bound ("65535:80") to cover all edge cases of the port range.

        assert!(result.unwrap_err().to_string().contains("Invalid host port"));

[cgwalters]

Looks sane to me! just some minor nits

[cgwalters]

CI needs a cargo fmt to pass

[cgwalters]

Awesome! So let's get this in as is but there's another step we can take after this to add an integration test.