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

@@ -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

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

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!(