Yureka <yuka@yuka.dev> writes:

--- host/start-vmm/ch.rs | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-)

Thanks a lot for the patch! Could you please add a Signed-off-by and copyright header? (You already have one in release/combined/default.nix — just needs to be like that but with the current year.)

diff --git a/host/start-vmm/ch.rs b/host/start-vmm/ch.rs index cf164c6..abc6a00 100644 --- a/host/start-vmm/ch.rs +++ b/host/start-vmm/ch.rs @@ -9,7 +9,7 @@ use std::os::raw::{c_char, c_int}; use std::os::unix::prelude::*; use std::process::{Command, Stdio};

-use miniserde::{json, Serialize}; +use miniserde::{json, Deserialize, Serialize};

use crate::net::MacAddress;

@@ -126,34 +126,26 @@ pub fn create_vm(vm_name: &str, mut config: VmConfig) -> Result<(), String> { }

pub fn add_net(vm_name: &str, net: &NetConfig) -> Result<OsString, NonZeroI32> { - let mut ch_remote = command(vm_name, "add-net") + let ch_output = command(vm_name, "add-net") .arg(format!("fd={},mac={}", net.fd, net.mac)) - .stdout(Stdio::piped()) .spawn() - .or(Err(EPERM))?; + .or(Err(EPERM))? + .wait_with_output() + .or(Err(EPROTO))?;

- let jq_out = match Command::new("jq") - .args(["-j", ".id"]) - .stdin(ch_remote.stdout.take().unwrap()) - .stderr(Stdio::inherit()) - .output() - { - Ok(o) => o, - Err(_) => { - // Try not to leave a zombie. - let _ = ch_remote.kill(); - let _ = ch_remote.wait(); - return Err(EPERM); - } - }; + if !ch_output.status.success() { + return Err(EPROTO); + }

- if let Ok(ch_remote_status) = ch_remote.wait() { - if ch_remote_status.success() && jq_out.status.success() { - return Ok(OsString::from_vec(jq_out.stdout)); - } + #[derive(Deserialize)] + struct AddNetOutput { + id: String, }

- Err(EPROTO) + let output_str = std::str::from_utf8(&ch_output.stdout).map_err(|_| EPROTO)?;

Can you use std::str at the top of the file? That's the normal convention in Spectrum.

+ let output_parsed: AddNetOutput = json::from_str(output_str).map_err(|_| EPROTO)?; + + Ok(OsString::from_vec(output_parsed.id.into()))

Should we just change this function to return String? I think the only reason to use OsString was that there was no reason to go through a UTF-8 check before, but now miniserde is doing that anyway. This would be a bigger diff, so if you'd rather I did that myself as follow-up that's fine too.