From d6ce6ad9a3044776849ca2bc04d5e2bb498207b8 Mon Sep 17 00:00:00 2001
From: Daniel Verkamp <dverkamp@chromium.org>
Date: Fri, 18 Aug 2023 15:23:19 -0700
Subject: [PATCH] devices: fw_cfg: clarify --fw-cfg parameter errors

Report the error from fs::read() if it fails, in addition to the path.

Also add an error for when both string and path are missing, and remove
the validation for this case in config.rs. This simplifies the cmdline
parsing code because it can now use the default deserialization method
instead of a custom from_str_fn.

BUG=b:283990685
TEST=tools/dev_container tools/presubmit
TEST=crosvm run --fw-cfg with string= and path= options

Change-Id: I37104ae00c9acda55f10fdd382ea727d5b59b6cd
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4795353
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Sebastian Hereu <sebastianhereu@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
---
 devices/src/fw_cfg.rs | 19 +++++++++++--------
 src/crosvm/cmdline.rs |  7 +------
 src/crosvm/config.rs  | 25 -------------------------
 3 files changed, 12 insertions(+), 39 deletions(-)

diff --git a/devices/src/fw_cfg.rs b/devices/src/fw_cfg.rs
index 96cc866691..e99e35e042 100644
--- a/devices/src/fw_cfg.rs
+++ b/devices/src/fw_cfg.rs
@@ -59,8 +59,11 @@ pub enum Error {
     #[error("Filename must be less than 55 characters long")]
     FileNameTooLong,
 
-    #[error("Unable to open file {0} for fw_cfg")]
-    FileOpen(PathBuf),
+    #[error("Unable to open file {0} for fw_cfg: {1}")]
+    FileOpen(PathBuf, std::io::Error),
+
+    #[error("fw_cfg parameters must have exactly one of string or path")]
+    StringOrPathRequired,
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -152,12 +155,12 @@ impl FwCfgDevice {
         };
 
         for param in fw_cfg_parameters {
-            let data: Vec<u8> = if let Some(string) = &param.string {
-                string.as_bytes().to_vec()
-            } else if let Ok(bytes) = fs::read(param.clone().path.unwrap()) {
-                bytes
-            } else {
-                return Err(Error::FileOpen(param.path.unwrap()));
+            let data = match (&param.string, &param.path) {
+                (Some(string), None) => string.as_bytes().to_vec(),
+                (None, Some(path)) => {
+                    fs::read(path).map_err(|e| Error::FileOpen(path.clone(), e))?
+                }
+                _ => return Err(Error::StringOrPathRequired),
             };
 
             // The file added from the command line will be a generic item. QEMU does not
diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs
index fce1b0e412..3ba7e41a8e 100644
--- a/src/crosvm/cmdline.rs
+++ b/src/crosvm/cmdline.rs
@@ -75,7 +75,6 @@ use crate::crosvm::config::parse_bus_id_addr;
 use crate::crosvm::config::parse_cpu_affinity;
 use crate::crosvm::config::parse_cpu_capacity;
 use crate::crosvm::config::parse_dynamic_power_coefficient;
-use crate::crosvm::config::parse_fw_cfg_options;
 #[cfg(target_arch = "x86_64")]
 use crate::crosvm::config::parse_memory_region;
 use crate::crosvm::config::parse_mmio_address_range;
@@ -1228,11 +1227,7 @@ pub struct RunCommand {
     /// doesn't require one.
     pub force_calibrated_tsc_leaf: Option<bool>,
 
-    #[argh(
-        option,
-        arg_name = "[name=NAME,path=PATH,string=STRING]",
-        from_str_fn(parse_fw_cfg_options)
-    )]
+    #[argh(option, arg_name = "name=NAME,(path=PATH|string=STRING)")]
     #[serde(skip)] // TODO(b/255223604)
     #[merge(strategy = append)]
     /// comma separated key=value pairs to specify data to pass to
diff --git a/src/crosvm/config.rs b/src/crosvm/config.rs
index c5fad4134a..872ba0ccf6 100644
--- a/src/crosvm/config.rs
+++ b/src/crosvm/config.rs
@@ -426,22 +426,6 @@ pub fn parse_mmio_address_range(s: &str) -> Result<Vec<AddressRange>, String> {
         .collect()
 }
 
-pub fn validate_fw_cfg_parameters(params: &FwCfgParameters) -> Result<(), String> {
-    if !(params.string.is_some() ^ params.path.is_some()) {
-        return Err("Provide exactly one of string or path args to --fw-cfg".to_string());
-    }
-
-    Ok(())
-}
-
-pub fn parse_fw_cfg_options(s: &str) -> Result<FwCfgParameters, String> {
-    let params: FwCfgParameters = from_key_values(s)?;
-
-    validate_fw_cfg_parameters(&params)?;
-
-    Ok(params)
-}
-
 pub fn validate_serial_parameters(params: &SerialParameters) -> Result<(), String> {
     if params.stdin && params.input.is_some() {
         return Err("Cannot specify both stdin and input options".to_string());
@@ -1840,15 +1824,6 @@ mod tests {
         assert_eq!(cfg.fw_cfg_parameters[0].path, None);
     }
 
-    #[test]
-    fn parse_fw_cfg_invalid_both_string_and_path() {
-        assert!(crate::crosvm::cmdline::RunCommand::from_args(
-            &[],
-            &["--fw-cfg", "name=bar,string=foo,path=path/to/file",]
-        )
-        .is_err());
-    }
-
     #[test]
     fn parse_fw_cfg_invalid_no_name() {
         assert!(