agent/device: Types to represent update for a device in the OCI spec

Currently update_spec_device() takes parameters 'vm_path' and 'final_path'
to give it the information it needs to update a single device in the OCI
spec for the guest.  This bundles these parameters into a single structure
type describing the updates to a single device.  This doesn't accomplish
much immediately, but will allow a number of further cleanups.

At the same time we change the representation of vm_path from a Unicode
string to a std::path::Path, which is a bit more natural since we are
performing file operations on it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

src/agent/src/device.rs

@@ -11,7 +11,7 @@ use std::fmt;
 use std::fs;
 use std::os::unix::ffi::OsStrExt;
 use std::os::unix::fs::MetadataExt;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::sync::Arc;
 use tokio::sync::Mutex;
@@ -435,22 +435,74 @@ fn scan_scsi_bus(scsi_addr: &str) -> Result<()> {
     Ok(())
 }
 
+#[derive(Debug, Clone)]
+struct DevNumUpdate {
+    // the path to the equivalent device in the VM (used to determine
+    // the correct major/minor numbers)
+    vm_path: PathBuf,
+}
+
+impl DevNumUpdate {
+    fn from_vm_path<T: AsRef<Path>>(vm_path: T) -> Result<Self> {
+        Ok(DevNumUpdate {
+            vm_path: vm_path.as_ref().to_owned(),
+        })
+    }
+}
+
+// Represents the device-node and resource related updates to the OCI
+// spec needed for a particular device
+#[derive(Debug, Clone)]
+struct DevUpdate {
+    num: DevNumUpdate,
+    // an optional new path to update the device to in the "inner" container
+    // specification
+    final_path: Option<String>,
+}
+
+impl DevUpdate {
+    fn from_vm_path<T: AsRef<Path>>(vm_path: T, final_path: String) -> Result<Self> {
+        Ok(DevUpdate {
+            final_path: Some(final_path),
+            ..DevNumUpdate::from_vm_path(vm_path)?.into()
+        })
+    }
+}
+
+impl From<DevNumUpdate> for DevUpdate {
+    fn from(num: DevNumUpdate) -> Self {
+        DevUpdate {
+            num,
+            final_path: None,
+        }
+    }
+}
+
+// Represents the updates to the OCI spec needed for a particular device
+#[derive(Debug, Clone, Default)]
+struct SpecUpdate {
+    dev: Option<DevUpdate>,
+}
+
+impl<T: Into<DevUpdate>> From<T> for SpecUpdate {
+    fn from(dev: T) -> Self {
+        SpecUpdate {
+            dev: Some(dev.into()),
+        }
+    }
+}
+
 // update_spec_device updates the device list in the OCI spec to make
 // it include details appropriate for the VM, instead of the host.  It
 // is given:
 //     container_path: the path to the device in the original OCI spec
-//     vm_path: the path to the equivalent device in the VM (used to
+//     update: information on changes to make to the device
-//              determine the correct major/minor numbers)
-//     final_path: a new path to update the device to in the "inner"
-//                 container specification (usually the same as
-//                 container_path)
 #[instrument]
 fn update_spec_device(
     spec: &mut Spec,
     devidx: &DevIndex,
     container_path: &str,
-    vm_path: &str,
+    update: DevUpdate,
-    final_path: Option<String>,
 ) -> Result<()> {
     // If no container_path is provided, we won't be able to match and
     // update the device in the OCI spec device list. This is an error.
@@ -463,18 +515,24 @@ fn update_spec_device(
         .as_mut()
         .ok_or_else(|| anyhow!("Spec didn't container linux field"))?;
 
-    if !Path::new(vm_path).exists() {
+    if !update.num.vm_path.exists() {
-        return Err(anyhow!("vm_path:{} doesn't exist", vm_path));
+        return Err(anyhow!(
+            "vm_path:{} doesn't exist",
+            update.num.vm_path.display()
+        ));
     }
 
-    let meta = fs::metadata(vm_path)?;
+    let meta = fs::metadata(&update.num.vm_path)?;
     let dev_id = meta.rdev();
     let major_id = stat::major(dev_id);
     let minor_id = stat::minor(dev_id);
 
     info!(
         sl!(),
-        "update_spec_device(): vm_path={}, major: {}, minor: {}\n", vm_path, major_id, minor_id
+        "update_spec_device(): vm_path={}, major: {}, minor: {}\n",
+        update.num.vm_path.display(),
+        major_id,
+        minor_id
     );
 
     if let Some(idxdata) = devidx.0.get(container_path) {
@@ -484,7 +542,7 @@ fn update_spec_device(
 
         dev.major = major_id as i64;
         dev.minor = minor_id as i64;
-        if let Some(final_path) = final_path {
+        if let Some(final_path) = update.final_path {
             dev.path = final_path;
         }
 
@@ -535,7 +593,12 @@ async fn virtiommio_blk_device_handler(
         return Err(anyhow!("Invalid path for virtio mmio blk device"));
     }
 
-    update_spec_device(spec, devidx, &device.container_path, &device.vm_path, None)
+    update_spec_device(
+        spec,
+        devidx,
+        &device.container_path,
+        DevNumUpdate::from_vm_path(&device.vm_path)?.into(),
+    )
 }
 
 // device.Id should be a PCI path string
@@ -549,7 +612,12 @@ async fn virtio_blk_device_handler(
     let pcipath = pci::Path::from_str(&device.id)?;
     let vm_path = get_virtio_blk_pci_device_name(sandbox, &pcipath).await?;
 
-    update_spec_device(spec, devidx, &device.container_path, &vm_path, None)
+    update_spec_device(
+        spec,
+        devidx,
+        &device.container_path,
+        DevNumUpdate::from_vm_path(vm_path)?.into(),
+    )
 }
 
 // device.id should be a CCW path string
@@ -563,12 +631,12 @@ async fn virtio_blk_ccw_device_handler(
 ) -> Result<()> {
     let ccw_device = ccw::Device::from_str(&device.id)?;
     let vm_path = get_virtio_blk_ccw_device_name(sandbox, &ccw_device).await?;
+
     update_spec_device(
         spec,
         devidx,
         &device.container_path,
-        &vm_path,
+        DevNumUpdate::from_vm_path(vm_path)?.into(),
-        &device.container_path,
     )
 }
 
@@ -592,7 +660,13 @@ async fn virtio_scsi_device_handler(
     devidx: &DevIndex,
 ) -> Result<()> {
     let vm_path = get_scsi_device_name(sandbox, &device.id).await?;
-    update_spec_device(spec, devidx, &device.container_path, &vm_path, None)
+
+    update_spec_device(
+        spec,
+        devidx,
+        &device.container_path,
+        DevNumUpdate::from_vm_path(vm_path)?.into(),
+    )
 }
 
 #[instrument]
@@ -606,7 +680,12 @@ async fn virtio_nvdimm_device_handler(
         return Err(anyhow!("Invalid path for nvdimm device"));
     }
 
-    update_spec_device(spec, devidx, &device.container_path, &device.vm_path, None)
+    update_spec_device(
+        spec,
+        devidx,
+        &device.container_path,
+        DevNumUpdate::from_vm_path(&device.vm_path)?.into(),
+    )
 }
 
 fn split_vfio_option(opt: &str) -> Option<(&str, &str)> {
@@ -667,14 +746,13 @@ async fn vfio_device_handler(
     if vfio_in_guest {
         // If there are any devices at all, logic above ensures that group is not None
         let group = group.unwrap();
-        let vmpath = get_vfio_device_name(sandbox, group).await?;
+        let vm_path = get_vfio_device_name(sandbox, group).await?;
 
         update_spec_device(
             spec,
             devidx,
             &device.container_path,
-            &vmpath,
+            DevUpdate::from_vm_path(&vm_path, vm_path.clone())?,
-            Some(vmpath.to_string()),
         )?;
     }
 
@@ -825,20 +903,35 @@ mod tests {
         let container_path = "";
         let vm_path = "";
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_err());
 
         // linux is empty
         let container_path = "/dev/null";
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_err());
 
         spec.linux = Some(Linux::default());
 
         // linux.devices is empty
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_err());
 
         spec.linux.as_mut().unwrap().devices = vec![oci::LinuxDevice {
@@ -850,14 +943,24 @@ mod tests {
 
         // vm_path empty
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_err());
 
         let vm_path = "/dev/null";
 
         // guest and host path are not the same
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(
             res.is_err(),
             "container_path={:?} vm_path={:?} spec={:?}",
@@ -870,7 +973,12 @@ mod tests {
 
         // spec.linux.resources is empty
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_ok());
 
         // update both devices and cgroup lists
@@ -891,7 +999,12 @@ mod tests {
         });
 
         let devidx = DevIndex::new(&spec);
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_ok());
     }
 
@@ -971,7 +1084,8 @@ mod tests {
         assert_eq!(Some(host_major_b), specresources.devices[1].major);
         assert_eq!(Some(host_minor_b), specresources.devices[1].minor);
 
-        let res = update_spec_device(&mut spec, &devidx, container_path_a, vm_path_a, None);
+        let update_a = DevNumUpdate::from_vm_path(vm_path_a).unwrap().into();
+        let res = update_spec_device(&mut spec, &devidx, container_path_a, update_a);
         assert!(res.is_ok());
 
         let specdevices = &spec.linux.as_ref().unwrap().devices;
@@ -986,7 +1100,8 @@ mod tests {
         assert_eq!(Some(host_major_b), specresources.devices[1].major);
         assert_eq!(Some(host_minor_b), specresources.devices[1].minor);
 
-        let res = update_spec_device(&mut spec, &devidx, container_path_b, vm_path_b, None);
+        let update_b = DevNumUpdate::from_vm_path(vm_path_b).unwrap().into();
+        let res = update_spec_device(&mut spec, &devidx, container_path_b, update_b);
         assert!(res.is_ok());
 
         let specdevices = &spec.linux.as_ref().unwrap().devices;
@@ -1061,7 +1176,12 @@ mod tests {
         assert_eq!(Some(host_major), specresources.devices[1].major);
         assert_eq!(Some(host_minor), specresources.devices[1].minor);
 
-        let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, None);
+        let res = update_spec_device(
+            &mut spec,
+            &devidx,
+            container_path,
+            DevNumUpdate::from_vm_path(vm_path).unwrap().into(),
+        );
         assert!(res.is_ok());
 
         // Only the char device, not the block device should be updated
@@ -1104,8 +1224,7 @@ mod tests {
             &mut spec,
             &devidx,
             container_path,
-            vm_path,
+            DevUpdate::from_vm_path(vm_path, final_path.to_string()).unwrap(),
-            Some(final_path.to_string()),
         );
         assert!(res.is_ok());