Re: [PATCH] host/start-vmm: replace jq calls with miniserde

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]