main: convert battery options to use serde_keyvalue

This requires the introduction of a BatteryConfig structure to parse,
which is probably a good idea anyway as it reserves some space to add
more battery-related options.

BUG=b:218223240
TEST=cargo test parse_battery

Change-Id: I82db7fe7ce2daeca004e734aaafa542583998561
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3784661
Tested-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>

aarch64/src/lib.rs

@@ -271,7 +271,7 @@ impl arch::LinuxArch for AArch64 {
         system_allocator: &mut SystemAllocator,
         serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>,
         serial_jail: Option<Minijail>,
-        (bat_type, bat_jail): (&Option<BatteryType>, Option<Minijail>),
+        (bat_type, bat_jail): (Option<BatteryType>, Option<Minijail>),
         mut vm: V,
         ramoops_region: Option<arch::pstore::RamoopsRegion>,
         devs: Vec<(Box<dyn BusDeviceObj>, Option<Minijail>)>,

arch/src/lib.rs

@@ -250,7 +250,7 @@ pub trait LinuxArch {
         system_allocator: &mut SystemAllocator,
         serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>,
         serial_jail: Option<Minijail>,
-        battery: (&Option<BatteryType>, Option<Minijail>),
+        battery: (Option<BatteryType>, Option<Minijail>),
         vm: V,
         ramoops_region: Option<pstore::RamoopsRegion>,
         devices: Vec<(Box<dyn BusDeviceObj>, Option<Minijail>)>,

src/crosvm/cmdline.rs

@@ -44,7 +44,6 @@ use devices::SerialParameters;
 use devices::StubPciParameters;
 use hypervisor::ProtectionType;
 use resources::AddressRange;
-use vm_control::BatteryType;
 
 #[cfg(feature = "gpu")]
 use super::sys::config::parse_gpu_options;
@@ -55,7 +54,6 @@ use super::sys::GpuRenderServerParameters;
 use crate::crosvm::config::numbered_disk_option;
 #[cfg(feature = "audio")]
 use crate::crosvm::config::parse_ac97_options;
-use crate::crosvm::config::parse_battery_options;
 use crate::crosvm::config::parse_bus_id_addr;
 use crate::crosvm::config::parse_cpu_affinity;
 use crate::crosvm::config::parse_cpu_capacity;
@@ -76,6 +74,7 @@ use crate::crosvm::config::parse_serial_options;
 use crate::crosvm::config::parse_stub_pci_parameters;
 #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
 use crate::crosvm::config::parse_userspace_msr_options;
+use crate::crosvm::config::BatteryConfig;
 #[cfg(feature = "plugin")]
 use crate::crosvm::config::BindMount;
 #[cfg(feature = "direct")]
@@ -471,13 +470,13 @@ pub struct RunCommand {
     #[argh(option, arg_name = "PATH")]
     /// path for balloon controller socket.
     pub balloon_control: Option<PathBuf>,
-    #[argh(option, from_str_fn(parse_battery_options))]
+    #[argh(option)]
     /// comma separated key=value pairs for setting up battery
     /// device
     /// Possible key values:
     ///     type=goldfish - type of battery emulation, defaults to
     ///     goldfish
-    pub battery: Option<BatteryType>,
+    pub battery: Option<BatteryConfig>,
     #[argh(option)]
     /// path to BIOS/firmware ROM
     pub bios: Option<PathBuf>,
@@ -1761,7 +1760,7 @@ impl TryFrom<RunCommand> for super::config::Config {
             cfg.pvm_fw = Some(p);
         }
 
-        cfg.battery_type = cmd.battery;
+        cfg.battery_config = cmd.battery;
 
         #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
         {

src/crosvm/config.rs

@@ -43,6 +43,7 @@ use hypervisor::ProtectionType;
 use resources::AddressRange;
 use serde::Deserialize;
 use serde::Serialize;
+use serde_keyvalue::FromKeyValues;
 use uuid::Uuid;
 use vm_control::BatteryType;
 #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
@@ -893,30 +894,11 @@ pub fn invalid_value_err<T: AsRef<str>, S: ToString>(value: T, expected: S) -> S
     format!("invalid value {}: {}", value.as_ref(), expected.to_string())
 }
 
-pub fn parse_battery_options(s: &str) -> Result<BatteryType, String> {
-    let mut battery_type: BatteryType = Default::default();
-
-    let opts = s
-        .split(',')
-        .map(|frag| frag.split('='))
-        .map(|mut kv| (kv.next().unwrap_or(""), kv.next().unwrap_or("")));
-
-    for (k, v) in opts {
-        match k {
-            "type" => match v.parse::<BatteryType>() {
-                Ok(type_) => battery_type = type_,
-                Err(e) => {
-                    return Err(invalid_value_err(v, e));
-                }
-            },
-            "" => {}
-            _ => {
-                return Err(format!("battery parameter {}", k));
-            }
-        }
-    }
-
-    Ok(battery_type)
+#[derive(Debug, Serialize, Deserialize, FromKeyValues)]
+#[serde(deny_unknown_fields, rename_all = "kebab-case")]
+pub struct BatteryConfig {
+    #[serde(rename = "type", default)]
+    pub type_: BatteryType,
 }
 
 pub fn parse_cpu_capacity(s: &str) -> Result<BTreeMap<usize, u32>, String> {
@@ -1215,7 +1197,7 @@ pub struct Config {
     pub balloon: bool,
     pub balloon_bias: i64,
     pub balloon_control: Option<PathBuf>,
-    pub battery_type: Option<BatteryType>,
+    pub battery_config: Option<BatteryConfig>,
     #[cfg(windows)]
     pub block_control_tube: Vec<Tube>,
     #[cfg(windows)]
@@ -1406,7 +1388,7 @@ impl Default for Config {
             balloon: true,
             balloon_bias: 0,
             balloon_control: None,
-            battery_type: None,
+            battery_config: None,
             #[cfg(windows)]
             block_control_tube: Vec::new(),
             #[cfg(windows)]
@@ -2038,23 +2020,25 @@ mod tests {
     }
 
     #[test]
-    fn parse_battery_vaild() {
-        parse_battery_options("type=goldfish").expect("parse should have succeded");
+    fn parse_battery_valid() {
+        let bat_config: BatteryConfig = from_key_values("type=goldfish").unwrap();
+        assert_eq!(bat_config.type_, BatteryType::Goldfish);
     }
 
     #[test]
-    fn parse_battery_vaild_no_type() {
-        parse_battery_options("").expect("parse should have succeded");
+    fn parse_battery_valid_no_type() {
+        let bat_config: BatteryConfig = from_key_values("").unwrap();
+        assert_eq!(bat_config.type_, BatteryType::Goldfish);
     }
 
     #[test]
-    fn parse_battery_invaild_parameter() {
-        parse_battery_options("tyep=goldfish").expect_err("parse should have failed");
+    fn parse_battery_invalid_parameter() {
+        from_key_values::<BatteryConfig>("tyep=goldfish").expect_err("parse should have failed");
     }
 
     #[test]
-    fn parse_battery_invaild_type_value() {
-        parse_battery_options("type=xxx").expect_err("parse should have failed");
+    fn parse_battery_invalid_type_value() {
+        from_key_values::<BatteryConfig>("type=xxx").expect_err("parse should have failed");
     }
 
     #[test]

src/crosvm/sys/unix.rs

@@ -1399,7 +1399,7 @@ where
         control_tubes.push(TaggedControlTube::VmIrq(ioapic_host_tube));
     }
 
-    let battery = if cfg.battery_type.is_some() {
+    let battery = if cfg.battery_config.is_some() {
         #[cfg_attr(not(feature = "power-monitor-powerd"), allow(clippy::manual_map))]
         let jail = match simple_jail(&cfg.jail_config, "battery")? {
             #[cfg_attr(not(feature = "power-monitor-powerd"), allow(unused_mut))]
@@ -1425,9 +1425,9 @@ where
             }
             None => None,
         };
-        (&cfg.battery_type, jail)
+        (cfg.battery_config.as_ref().map(|c| c.type_), jail)
     } else {
-        (&cfg.battery_type, None)
+        (cfg.battery_config.as_ref().map(|c| c.type_), None)
     };
 
     let map_request: Arc<Mutex<Option<ExternalMapping>>> = Arc::new(Mutex::new(None));

vm_control/src/lib.rs

@@ -591,6 +591,7 @@ impl Display for BatControlResult {
 }
 
 #[derive(Serialize, Deserialize, Copy, Clone, Debug, PartialEq)]
+#[serde(rename_all = "kebab-case")]
 pub enum BatteryType {
     Goldfish,
 }

x86_64/src/lib.rs

@@ -549,7 +549,7 @@ impl arch::LinuxArch for X8664arch {
         system_allocator: &mut SystemAllocator,
         serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>,
         serial_jail: Option<Minijail>,
-        battery: (&Option<BatteryType>, Option<Minijail>),
+        battery: (Option<BatteryType>, Option<Minijail>),
         mut vm: V,
         ramoops_region: Option<arch::pstore::RamoopsRegion>,
         devs: Vec<(Box<dyn BusDeviceObj>, Option<Minijail>)>,
@@ -1575,7 +1575,7 @@ impl X8664arch {
         #[cfg(feature = "direct")] direct_gpe: &[u32],
         irq_chip: &mut dyn IrqChip,
         sci_irq: u32,
-        battery: (&Option<BatteryType>, Option<Minijail>),
+        battery: (Option<BatteryType>, Option<Minijail>),
         #[cfg_attr(windows, allow(unused_variables))] mmio_bus: &devices::Bus,
         max_bus: u8,
         resume_notify_devices: &mut Vec<Arc<Mutex<dyn BusResumeDevice>>>,

x86_64/src/test_integration.rs

@@ -200,7 +200,7 @@ where
         &[], // direct_gpe
         &mut irq_chip,
         X86_64_SCI_IRQ,
-        (&None, None),
+        (None, None),
         &mmio_bus,
         max_bus,
         &mut resume_notify_devices,