From da20cf1d78a6487d86d6355023bce549298fe0e4 Mon Sep 17 00:00:00 2001 From: Alexandre Courbot Date: Thu, 3 Feb 2022 19:39:30 +0900 Subject: [PATCH] devices: serial: parse options using serde_keyvalue crate Use our new serde_keyvalue crate to annotate the SerialParameters structure and allow us to create it from a key-values string. Add tests to help ensure parsing doesn't break in the future. The existing arguments can be parsed identically by this new code, so replace the old serial options. BUG=b:218223240 TEST=cargo test -p devices serial_device::tests::params_from_key_values TEST=cargo test parse_serial Change-Id: I4898a45399b69b87a44f80d3a214daf081b06173 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3439670 Reviewed-by: Anton Romanov Reviewed-by: Daniel Verkamp Reviewed-by: Keiichi Watanabe Tested-by: kokoro Commit-Queue: Alexandre Courbot --- devices/Cargo.toml | 1 + devices/src/serial_device.rs | 169 ++++++++++++++++++++++++++++------- src/argument.rs | 3 + src/main.rs | 96 +++----------------- 4 files changed, 157 insertions(+), 112 deletions(-) diff --git a/devices/Cargo.toml b/devices/Cargo.toml index 3338a6a23a..7c8c456d74 100644 --- a/devices/Cargo.toml +++ b/devices/Cargo.toml @@ -55,6 +55,7 @@ remain = "*" resources = { path = "../resources" } serde = { version = "1", features = [ "derive" ] } serde_json = "1" +serde_keyvalue = { path = "../serde_keyvalue" } smallvec = "1.6.1" sync = { path = "../common/sync" } system_api = { version = "*", optional = true } diff --git a/devices/src/serial_device.rs b/devices/src/serial_device.rs index 795e05509f..8e5c6419ab 100644 --- a/devices/src/serial_device.rs +++ b/devices/src/serial_device.rs @@ -8,7 +8,6 @@ use std::fs::{File, OpenOptions}; use std::io::{self, stdin, stdout, ErrorKind}; use std::os::unix::net::UnixDatagram; use std::path::{Path, PathBuf}; -use std::str::FromStr; use std::thread; use std::time::Duration; @@ -18,6 +17,7 @@ use base::{ }; use hypervisor::ProtectionType; use remain::sorted; +use serde::Deserialize; use thiserror::Error as ThisError; #[sorted] @@ -54,15 +54,23 @@ pub trait SerialDevice { } /// Enum for possible type of serial devices -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Deserialize)] +#[serde(rename_all = "kebab-case")] pub enum SerialType { File, Stdout, Sink, Syslog, + #[serde(rename = "unix")] UnixSocket, } +impl Default for SerialType { + fn default() -> Self { + Self::Sink + } +} + impl Display for SerialType { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let s = match &self { @@ -77,27 +85,20 @@ impl Display for SerialType { } } -impl FromStr for SerialType { - type Err = Error; - fn from_str(s: &str) -> std::result::Result { - match s { - "file" | "File" => Ok(SerialType::File), - "stdout" | "Stdout" => Ok(SerialType::Stdout), - "sink" | "Sink" => Ok(SerialType::Sink), - "syslog" | "Syslog" => Ok(SerialType::Syslog), - "unix" | "UnixSocket" => Ok(SerialType::UnixSocket), - _ => Err(Error::InvalidSerialType(s.to_string())), - } - } -} - /// Serial device hardware types -#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize)] +#[serde(rename_all = "kebab-case")] pub enum SerialHardware { Serial, // Standard PC-style (8250/16550 compatible) UART VirtioConsole, // virtio-console device } +impl Default for SerialHardware { + fn default() -> Self { + Self::Serial + } +} + impl Display for SerialHardware { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let s = match &self { @@ -109,17 +110,6 @@ impl Display for SerialHardware { } } -impl FromStr for SerialHardware { - type Err = Error; - fn from_str(s: &str) -> std::result::Result { - match s { - "serial" => Ok(SerialHardware::Serial), - "virtio-console" => Ok(SerialHardware::VirtioConsole), - _ => Err(Error::InvalidSerialHardware(s.to_string())), - } - } -} - struct WriteSocket { sock: UnixDatagram, buf: String, @@ -187,13 +177,19 @@ impl io::Write for WriteSocket { } } -/// Holds the parameters for a serial device -#[derive(Clone, Debug)] +fn serial_parameters_default_num() -> u8 { + 1 +} + +#[derive(Clone, Debug, Default, PartialEq, Deserialize)] +#[serde(deny_unknown_fields, default)] pub struct SerialParameters { + #[serde(rename = "type")] pub type_: SerialType, pub hardware: SerialHardware, pub path: Option, pub input: Option, + #[serde(default = "serial_parameters_default_num")] pub num: u8, pub console: bool, pub earlycon: bool, @@ -348,3 +344,116 @@ impl SerialParameters { Ok(T::new(protected_vm, evt, input, output, keep_rds.to_vec())) } } + +#[cfg(test)] +mod tests { + use super::*; + use serde_keyvalue::*; + + fn from_serial_arg(options: &str) -> Result { + from_key_values(options) + } + + #[test] + fn params_from_key_values() { + // Defaults + let params = from_serial_arg("").unwrap(); + assert_eq!( + params, + SerialParameters { + type_: SerialType::Sink, + hardware: SerialHardware::Serial, + path: None, + input: None, + num: 1, + console: false, + earlycon: false, + stdin: false, + } + ); + + // type parameter + let params = from_serial_arg("type=file").unwrap(); + assert_eq!(params.type_, SerialType::File); + let params = from_serial_arg("type=stdout").unwrap(); + assert_eq!(params.type_, SerialType::Stdout); + let params = from_serial_arg("type=sink").unwrap(); + assert_eq!(params.type_, SerialType::Sink); + let params = from_serial_arg("type=syslog").unwrap(); + assert_eq!(params.type_, SerialType::Syslog); + let params = from_serial_arg("type=unix").unwrap(); + assert_eq!(params.type_, SerialType::UnixSocket); + let params = from_serial_arg("type=foobar"); + assert!(params.is_err()); + + // hardware parameter + let params = from_serial_arg("hardware=serial").unwrap(); + assert_eq!(params.hardware, SerialHardware::Serial); + let params = from_serial_arg("hardware=virtio-console").unwrap(); + assert_eq!(params.hardware, SerialHardware::VirtioConsole); + let params = from_serial_arg("hardware=foobar"); + assert!(params.is_err()); + + // path parameter + let params = from_serial_arg("path=/test/path").unwrap(); + assert_eq!(params.path, Some("/test/path".into())); + let params = from_serial_arg("path"); + assert!(params.is_err()); + + // input parameter + let params = from_serial_arg("input=/path/to/input").unwrap(); + assert_eq!(params.input, Some("/path/to/input".into())); + let params = from_serial_arg("input"); + assert!(params.is_err()); + + // console parameter + let params = from_serial_arg("console").unwrap(); + assert!(params.console); + let params = from_serial_arg("console=true").unwrap(); + assert!(params.console); + let params = from_serial_arg("console=false").unwrap(); + assert!(!params.console); + let params = from_serial_arg("console=foobar"); + assert!(params.is_err()); + + // earlycon parameter + let params = from_serial_arg("earlycon").unwrap(); + assert!(params.earlycon); + let params = from_serial_arg("earlycon=true").unwrap(); + assert!(params.earlycon); + let params = from_serial_arg("earlycon=false").unwrap(); + assert!(!params.earlycon); + let params = from_serial_arg("earlycon=foobar"); + assert!(params.is_err()); + + // stdin parameter + let params = from_serial_arg("stdin").unwrap(); + assert!(params.stdin); + let params = from_serial_arg("stdin=true").unwrap(); + assert!(params.stdin); + let params = from_serial_arg("stdin=false").unwrap(); + assert!(!params.stdin); + let params = from_serial_arg("stdin=foobar"); + assert!(params.is_err()); + + // all together + let params = from_serial_arg("type=stdout,path=/some/path,hardware=virtio-console,num=5,earlycon,console,stdin,input=/some/input").unwrap(); + assert_eq!( + params, + SerialParameters { + type_: SerialType::Stdout, + hardware: SerialHardware::VirtioConsole, + path: Some("/some/path".into()), + input: Some("/some/input".into()), + num: 5, + console: true, + earlycon: true, + stdin: true, + } + ); + + // invalid field + let params = from_serial_arg("type=stdout,foo=bar"); + assert!(params.is_err()); + } +} diff --git a/src/argument.rs b/src/argument.rs index 42211fdd55..e659391018 100644 --- a/src/argument.rs +++ b/src/argument.rs @@ -54,6 +54,9 @@ use thiserror::Error; #[sorted] #[derive(Error, Debug)] pub enum Error { + /// Free error for use with the `serde_keyvalue` crate parser. + #[error("failed to parse key-value arguments: {0}")] + ConfigParserError(String), /// The argument was required. #[error("expected argument: {0}")] ExpectedArgument(String), diff --git a/src/main.rs b/src/main.rs index d28a54be1c..7f3366fd3b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,7 +32,7 @@ use crosvm::{ SharedDir, TouchDeviceOption, VfioCommand, VhostUserFsOption, VhostUserOption, VhostUserWlOption, VhostVsockDeviceParameter, VvuOption, DISK_ID_LEN, }; -use devices::serial_device::{SerialHardware, SerialParameters, SerialType}; +use devices::serial_device::{SerialHardware, SerialParameters}; #[cfg(feature = "audio_cras")] use devices::virtio::snd::cras_backend::Error as CrasSndError; #[cfg(feature = "audio_cras")] @@ -61,6 +61,7 @@ use disk::{ create_composite_disk, create_disk_file, create_zero_filler, ImagePartitionType, PartitionInfo, }; use hypervisor::ProtectionType; +use serde_keyvalue::from_key_values; use uuid::Uuid; use vm_control::{ client::{ @@ -690,88 +691,19 @@ fn parse_ac97_options(s: &str) -> argument::Result { } fn parse_serial_options(s: &str) -> argument::Result { - let mut serial_setting = SerialParameters { - type_: SerialType::Sink, - hardware: SerialHardware::Serial, - path: None, - input: None, - num: 1, - console: false, - earlycon: false, - stdin: false, - }; + let serial_setting: SerialParameters = + from_key_values(s).map_err(|e| argument::Error::ConfigParserError(e.to_string()))?; - let opts = s - .split(',') - .map(|frag| frag.splitn(2, '=')) - .map(|mut kv| (kv.next().unwrap_or(""), kv.next().unwrap_or(""))); - - for (k, v) in opts { - match k { - "hardware" => { - serial_setting.hardware = v - .parse::() - .map_err(|e| argument::Error::UnknownArgument(format!("{}", e)))? - } - "type" => { - serial_setting.type_ = v - .parse::() - .map_err(|e| argument::Error::UnknownArgument(format!("{}", e)))? - } - "num" => { - let num = v.parse::().map_err(|e| { - argument::Error::Syntax(format!("serial device number is not parsable: {}", e)) - })?; - if num < 1 { - return Err(argument::Error::InvalidValue { - value: num.to_string(), - expected: String::from("Serial port num must be at least 1"), - }); - } - serial_setting.num = num; - } - "console" => { - serial_setting.console = v.parse::().map_err(|e| { - argument::Error::Syntax(format!( - "serial device console is not parseable: {}", - e - )) - })? - } - "earlycon" => { - serial_setting.earlycon = v.parse::().map_err(|e| { - argument::Error::Syntax(format!( - "serial device earlycon is not parseable: {}", - e, - )) - })? - } - "stdin" => { - serial_setting.stdin = v.parse::().map_err(|e| { - argument::Error::Syntax(format!("serial device stdin is not parseable: {}", e)) - })?; - if serial_setting.stdin && serial_setting.input.is_some() { - return Err(argument::Error::TooManyArguments( - "Cannot specify both stdin and input options".to_string(), - )); - } - } - "path" => serial_setting.path = Some(PathBuf::from(v)), - "input" => { - if serial_setting.stdin { - return Err(argument::Error::TooManyArguments( - "Cannot specify both stdin and input options".to_string(), - )); - } - serial_setting.input = Some(PathBuf::from(v)); - } - _ => { - return Err(argument::Error::UnknownArgument(format!( - "serial parameter {}", - k - ))); - } - } + if serial_setting.stdin && serial_setting.input.is_some() { + return Err(argument::Error::TooManyArguments( + "Cannot specify both stdin and input options".to_string(), + )); + } + if serial_setting.num < 1 { + return Err(argument::Error::InvalidValue { + value: serial_setting.num.to_string(), + expected: String::from("Serial port num must be at least 1"), + }); } if serial_setting.hardware == SerialHardware::Serial && serial_setting.num > 4 {