From e04d18a016219c472428cf102c3b3ecadbe5bb1c Mon Sep 17 00:00:00 2001
From: Alexandre Courbot <acourbot@chromium.org>
Date: Tue, 25 Oct 2022 15:52:41 +0900
Subject: [PATCH] crosvm: use serde_keyvalue to build Pstore

We had a manual parsing function that is strictly equivalent to how
serde_keyvalue would deserialize, so do the latter instead.

Also add some tests to make sure we don't regress in the future.

BUG=b:218223240
TEST=./tools/health-check

Change-Id: I5b6317774368fa4256a1944e7aec54e8fe8f210a
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3979494
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
---
 Cargo.lock            |  1 +
 arch/Cargo.toml       |  1 +
 arch/src/lib.rs       | 32 +++++++++++++++++++++++++++++++-
 src/crosvm/cmdline.rs |  3 +--
 src/crosvm/config.rs  | 32 --------------------------------
 5 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 117ce5bfa8..738eed3dee 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -94,6 +94,7 @@ dependencies = [
  "remain",
  "resources",
  "serde",
+ "serde_keyvalue",
  "sync",
  "thiserror",
  "vm_control",
diff --git a/arch/Cargo.toml b/arch/Cargo.toml
index 4700ae3f0f..d41522279e 100644
--- a/arch/Cargo.toml
+++ b/arch/Cargo.toml
@@ -24,6 +24,7 @@ libc = "*"
 resources = { path = "../resources" }
 remain = "*"
 serde = { version = "*", features = [ "derive"] }
+serde_keyvalue = { path = "../serde_keyvalue", features = ["argh_derive"] }
 sync = { path = "../common/sync" }
 thiserror = "1.0.20"
 vm_control = { path = "../vm_control" }
diff --git a/arch/src/lib.rs b/arch/src/lib.rs
index 070cd3e85e..45bf3ec62e 100644
--- a/arch/src/lib.rs
+++ b/arch/src/lib.rs
@@ -96,6 +96,7 @@ use resources::SystemAllocator;
 use resources::SystemAllocatorConfig;
 use serde::Deserialize;
 use serde::Serialize;
+use serde_keyvalue::FromKeyValues;
 pub use serial::add_serial_devices;
 pub use serial::get_serial_cmdline;
 pub use serial::set_default_serial_parameters;
@@ -115,7 +116,8 @@ pub enum VmImage {
     Bios(File),
 }
 
-#[derive(Clone, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize, Serialize, FromKeyValues, PartialEq)]
+#[serde(deny_unknown_fields, rename_all = "kebab-case")]
 pub struct Pstore {
     pub path: PathBuf,
     pub size: u32,
@@ -1120,3 +1122,31 @@ pub enum MsrExitHandlerError {
     #[error("Fail to create MSR handler")]
     HandlerCreateFailed,
 }
+
+#[cfg(test)]
+mod tests {
+    use serde_keyvalue::from_key_values;
+
+    use super::*;
+
+    #[test]
+    fn parse_pstore() {
+        let res: Pstore = from_key_values("path=/some/path,size=16384").unwrap();
+        assert_eq!(
+            res,
+            Pstore {
+                path: "/some/path".into(),
+                size: 16384,
+            }
+        );
+
+        let res = from_key_values::<Pstore>("path=/some/path");
+        assert!(res.is_err());
+
+        let res = from_key_values::<Pstore>("size=16384");
+        assert!(res.is_err());
+
+        let res = from_key_values::<Pstore>("");
+        assert!(res.is_err());
+    }
+}
diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs
index 6deab64e24..c3719a38f5 100644
--- a/src/crosvm/cmdline.rs
+++ b/src/crosvm/cmdline.rs
@@ -74,7 +74,6 @@ use crate::crosvm::config::parse_pcie_root_port_params;
 use crate::crosvm::config::parse_pflash_parameters;
 #[cfg(feature = "plugin")]
 use crate::crosvm::config::parse_plugin_mount_option;
-use crate::crosvm::config::parse_pstore;
 use crate::crosvm::config::parse_serial_options;
 use crate::crosvm::config::parse_stub_pci_parameters;
 #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
@@ -1225,7 +1224,7 @@ pub struct RunCommand {
     /// (EXPERIMENTAL) prevent host access to guest memory, but don't use protected VM firmware
     protected_vm_without_firmware: bool,
 
-    #[argh(option, arg_name = "path=PATH,size=SIZE", from_str_fn(parse_pstore))]
+    #[argh(option, arg_name = "path=PATH,size=SIZE")]
     /// path to pstore buffer backend file followed by size
     ///     [--pstore <path=PATH,size=SIZE>]
     pub pstore: Option<Pstore>,
diff --git a/src/crosvm/config.rs b/src/crosvm/config.rs
index c81fe127e8..934cdd2647 100644
--- a/src/crosvm/config.rs
+++ b/src/crosvm/config.rs
@@ -576,38 +576,6 @@ pub fn parse_mmio_address_range(s: &str) -> Result<Vec<AddressRange>, String> {
         .collect()
 }
 
-pub fn parse_pstore(value: &str) -> Result<Pstore, String> {
-    let components: Vec<&str> = value.split(',').collect();
-    if components.len() != 2 {
-        return Err(invalid_value_err(
-            value,
-            "pstore must have exactly 2 components: path=<path>,size=<size>",
-        ));
-    }
-    Ok(Pstore {
-        path: {
-            if components[0].len() <= 5 || !components[0].starts_with("path=") {
-                return Err(invalid_value_err(
-                    components[0],
-                    "pstore path must follow with `path=`",
-                ));
-            };
-            PathBuf::from(&components[0][5..])
-        },
-        size: {
-            if components[1].len() <= 5 || !components[1].starts_with("size=") {
-                return Err(invalid_value_err(
-                    components[1],
-                    "pstore size must follow with `size=`",
-                ));
-            };
-            components[1][5..]
-                .parse()
-                .map_err(|_| invalid_value_err(value, "pstore size must be an integer"))?
-        },
-    })
-}
-
 #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
 pub fn parse_userspace_msr_options(value: &str) -> Result<(u32, MsrConfig), String> {
     let mut rw_type: Option<MsrRWType> = None;