Address PR reviews

Signed-off-by: Marco Vedovati <mv@sba.lat>

src/arch/src/aarch64/fdt.rs

@@ -13,7 +13,7 @@ use std::ptr::null;
 use std::{io, result};
 
 use super::super::DeviceType;
-use super::super::InitrdInfo;
+use super::super::InitrdConfig;
 use super::get_fdt_addr;
 use super::gic::GICDevice;
 use super::layout::FDT_MAX_SIZE;
@@ -90,7 +90,7 @@ pub fn create_fdt<T: DeviceInfoForFDT + Clone + Debug>(
     cmdline: &CStr,
     device_info: Option<&HashMap<(DeviceType, String), T>>,
     gic_device: &Box<dyn GICDevice>,
-    initrd: &InitrdInfo,
+    initrd: &Option<InitrdConfig>,
 ) -> Result<(Vec<u8>)> {
     // Alocate stuff necessary for the holding the blob.
     let mut fdt = vec![0; FDT_MAX_SIZE];
@@ -343,17 +343,27 @@ fn create_memory_node(fdt: &mut Vec<u8>, guest_mem: &GuestMemory) -> Result<()>
     Ok(())
 }
 
-fn create_chosen_node(fdt: &mut Vec<u8>, cmdline: &CStr, initrd: &InitrdInfo) -> Result<()> {
+fn create_chosen_node(
+    fdt: &mut Vec<u8>,
+    cmdline: &CStr,
+    initrd: &Option<InitrdConfig>,
+) -> Result<()> {
     append_begin_node(fdt, "chosen")?;
     append_property_cstring(fdt, "bootargs", cmdline)?;
-    if initrd.size > 0 {
-        append_property_u64(fdt, "linux,initrd-start", initrd.address.raw_value() as u64)?;
+
+    if let Some(initrd_config) = initrd {
+        append_property_u64(
+            fdt,
+            "linux,initrd-start",
+            initrd_config.address.raw_value() as u64,
+        )?;
         append_property_u64(
             fdt,
             "linux,initrd-end",
-            initrd.address.raw_value() + initrd.size as u64,
+            initrd_config.address.raw_value() + initrd_config.size as u64,
         )?;
     }
+
     append_end_node(fdt)?;
 
     Ok(())
@@ -585,17 +595,13 @@ mod tests {
         let kvm = Kvm::new().unwrap();
         let vm = kvm.create_vm().unwrap();
         let gic = create_gic(&vm, 1).unwrap();
-        let initrd = super::InitrdInfo {
-            address: GuestAddress(0),
-            size: 0,
-        };
         assert!(create_fdt(
             &mem,
             vec![0],
             &CString::new("console=tty0").unwrap(),
             Some(&dev_info),
             &gic,
-            &initrd,
+            &None,
         )
         .is_ok())
     }
@@ -607,17 +613,13 @@ mod tests {
         let kvm = Kvm::new().unwrap();
         let vm = kvm.create_vm().unwrap();
         let gic = create_gic(&vm, 1).unwrap();
-        let initrd = super::InitrdInfo {
-            address: GuestAddress(0),
-            size: 0,
-        };
         let mut dtb = create_fdt(
             &mem,
             vec![0],
             &CString::new("console=tty0").unwrap(),
             None::<&std::collections::HashMap<(DeviceType, std::string::String), MMIODeviceInfo>>,
             &gic,
-            &initrd,
+            &None,
         )
         .unwrap();
 
@@ -656,7 +658,7 @@ mod tests {
         let kvm = Kvm::new().unwrap();
         let vm = kvm.create_vm().unwrap();
         let gic = create_gic(&vm, 1).unwrap();
-        let initrd = InitrdInfo {
+        let initrd = InitrdConfig {
             address: GuestAddress(0x10000000),
             size: 0x1000,
         };
@@ -667,7 +669,7 @@ mod tests {
             &CString::new("console=tty0").unwrap(),
             None::<&std::collections::HashMap<(DeviceType, std::string::String), MMIODeviceInfo>>,
             &gic,
-            &initrd,
+            &Some(initrd),
         )
         .unwrap();
 

src/arch/src/aarch64/mod.rs

@@ -50,13 +50,15 @@ pub fn arch_memory_regions(size: usize) -> Vec<(GuestAddress, usize)> {
 /// * `cmdline_cstring` - The kernel commandline.
 /// * `vcpu_mpidr` - Array of MPIDR register values per vcpu.
 /// * `device_info` - A hashmap containing the attached devices for building FDT device nodes.
+/// * `gic_device` - The GIC device.
+/// * `initrd` - Information about an optional initrd.
 pub fn configure_system<T: DeviceInfoForFDT + Clone + Debug>(
     guest_mem: &GuestMemory,
     cmdline_cstring: &CStr,
     vcpu_mpidr: Vec<u64>,
     device_info: Option<&HashMap<(DeviceType, String), T>>,
     gic_device: &Box<dyn GICDevice>,
-    initrd: &super::InitrdInfo,
+    initrd: &Option<super::InitrdConfig>,
 ) -> super::Result<()> {
     fdt::create_fdt(
         guest_mem,

src/arch/src/lib.rs

@@ -51,7 +51,7 @@ pub enum DeviceType {
 }
 
 /// Type for passing information about the initrd in the guest memory.
-pub struct InitrdInfo {
+pub struct InitrdConfig {
     /// Load address of initrd in guest memory
     pub address: memory_model::GuestAddress,
     /// Size of initrd in guest memory

src/arch/src/x86_64/mod.rs

@@ -18,7 +18,7 @@ use std::mem;
 
 use arch_gen::x86::bootparam::{boot_params, E820_RAM};
 use memory_model::{Address, ByteValued, GuestAddress, GuestMemory};
-use InitrdInfo;
+use InitrdConfig;
 
 // This is a workaround to the Rust enforcement specifying that any implementation of a foreign
 // trait (in this case `ByteValued`) where:
@@ -101,7 +101,7 @@ pub fn configure_system(
     guest_mem: &GuestMemory,
     cmdline_addr: GuestAddress,
     cmdline_size: usize,
-    initrd: &InitrdInfo,
+    initrd: &Option<InitrdConfig>,
     num_cpus: u8,
 ) -> super::Result<()> {
     const KERNEL_BOOT_FLAG_MAGIC: u16 = 0xaa55;
@@ -124,8 +124,10 @@ pub fn configure_system(
     params.0.hdr.cmd_line_ptr = cmdline_addr.raw_value() as u32;
     params.0.hdr.cmdline_size = cmdline_size as u32;
     params.0.hdr.kernel_alignment = KERNEL_MIN_ALIGNMENT_BYTES;
-    params.0.hdr.ramdisk_image = initrd.address.raw_value() as u32;
-    params.0.hdr.ramdisk_size = initrd.size as u32;
+    if let Some(initrd_config) = initrd {
+        params.0.hdr.ramdisk_image = initrd_config.address.raw_value() as u32;
+        params.0.hdr.ramdisk_size = initrd_config.size as u32;
+    }
 
     add_e820_entry(&mut params.0, 0, EBDA_START, E820_RAM)?;
 
@@ -216,11 +218,7 @@ mod tests {
     fn test_system_configuration() {
         let no_vcpus = 4;
         let gm = GuestMemory::new(&[(GuestAddress(0), 0x10000)]).unwrap();
-        let initrd = InitrdInfo {
-            address: GuestAddress(0),
-            size: 0,
-        };
-        let config_err = configure_system(&gm, GuestAddress(0), 0, &initrd, 1);
+        let config_err = configure_system(&gm, GuestAddress(0), 0, &None, 1);
         assert!(config_err.is_err());
         assert_eq!(
             config_err.unwrap_err(),
@@ -231,19 +229,19 @@ mod tests {
         let mem_size = 128 << 20;
         let arch_mem_regions = arch_memory_regions(mem_size);
         let gm = GuestMemory::new(&arch_mem_regions).unwrap();
-        configure_system(&gm, GuestAddress(0), 0, &initrd, no_vcpus).unwrap();
+        configure_system(&gm, GuestAddress(0), 0, &None, no_vcpus).unwrap();
 
         // Now assigning some memory that is equal to the start of the 32bit memory hole.
         let mem_size = 3328 << 20;
         let arch_mem_regions = arch_memory_regions(mem_size);
         let gm = GuestMemory::new(&arch_mem_regions).unwrap();
-        configure_system(&gm, GuestAddress(0), 0, &initrd, no_vcpus).unwrap();
+        configure_system(&gm, GuestAddress(0), 0, &None, no_vcpus).unwrap();
 
         // Now assigning some memory that falls after the 32bit memory hole.
         let mem_size = 3330 << 20;
         let arch_mem_regions = arch_memory_regions(mem_size);
         let gm = GuestMemory::new(&arch_mem_regions).unwrap();
-        configure_system(&gm, GuestAddress(0), 0, &initrd, no_vcpus).unwrap();
+        configure_system(&gm, GuestAddress(0), 0, &None, no_vcpus).unwrap();
     }
 
     #[test]

src/vmm/src/error.rs

@@ -72,12 +72,12 @@ impl std::fmt::Debug for Error {
 }
 
 /// Errors associated with loading initrd
-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub enum LoadInitrdError {
     /// Cannot load initrd due to an invalid memory configuration.
     LoadInitrd,
     /// Cannot load initrd due to an invalid image.
-    ReadInitrd,
+    ReadInitrd(io::Error),
 }
 
 /// It's convenient to automatically convert `LoadInitrdError`s
@@ -176,11 +176,10 @@ impl Display for LoadInitrdError {
         use self::LoadInitrdError::*;
         match *self {
             LoadInitrd => write!(f, "Failed to load the initrd image to guest memory"),
-            ReadInitrd => write!(f, "Failed to read the initrd image"),
+            ReadInitrd(ref e) => write!(f, "Failed to read the initrd image. {}", e),
         }
     }
 }
-
 impl Display for StartMicrovmError {
     fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
         use self::StartMicrovmError::*;

src/vmm/src/lib.rs

@@ -58,7 +58,7 @@ use timerfd::{ClockId, SetTimeFlags, TimerFd, TimerState};
 
 #[cfg(target_arch = "aarch64")]
 use arch::DeviceType;
-use arch::InitrdInfo;
+use arch::InitrdConfig;
 #[cfg(target_arch = "x86_64")]
 use device_manager::legacy::PortIODeviceManager;
 #[cfg(target_arch = "aarch64")]
@@ -1011,7 +1011,7 @@ impl Vmm {
     fn load_initrd<F>(
         vm_memory: &GuestMemory,
         image: &mut F,
-    ) -> std::result::Result<InitrdInfo, LoadInitrdError>
+    ) -> std::result::Result<InitrdConfig, LoadInitrdError>
     where
         F: Read + Seek,
     {
@@ -1020,11 +1020,17 @@ impl Vmm {
         let size: usize;
         // Get the image size
         match image.seek(SeekFrom::End(0)) {
-            Ok(0) | Err(..) => return Err(ReadInitrd),
+            Err(e) => return Err(ReadInitrd(e)),
+            Ok(0) => {
+                return Err(ReadInitrd(io::Error::new(
+                    io::ErrorKind::InvalidData,
+                    "Initrd image seek returned a size of zero",
+                )))
+            }
             Ok(s) => size = s as usize,
         };
         // Go back to the image start
-        image.seek(SeekFrom::Start(0)).map_err(|_| ReadInitrd)?;
+        image.seek(SeekFrom::Start(0)).map_err(ReadInitrd)?;
 
         // Get the target address
         let address = arch::initrd_load_addr(vm_memory, size).map_err(|_| LoadInitrd)?;
@@ -1034,7 +1040,7 @@ impl Vmm {
             .read_to_memory(GuestAddress(address), image, size)
             .map_err(|_| LoadInitrd)?;
 
-        Ok(InitrdInfo {
+        Ok(InitrdConfig {
             address: GuestAddress(address),
             size,
         })
@@ -1050,19 +1056,18 @@ impl Vmm {
 
         let kernel_config = self.kernel_config.as_ref().ok_or(MissingKernelConfig)?;
 
-        let initrd = match &kernel_config.initrd_file {
+        let initrd: Option<InitrdConfig> = match &kernel_config.initrd_file {
             Some(f) => {
                 let initrd_file = f.try_clone();
                 if initrd_file.is_err() {
-                    return Err(InitrdLoader(LoadInitrdError::ReadInitrd));
+                    return Err(InitrdLoader(LoadInitrdError::ReadInitrd(io::Error::from(
+                        io::ErrorKind::InvalidData,
+                    ))));
                 }
-
-                Vmm::load_initrd(vm_memory, &mut initrd_file.unwrap())?
+                let res = Vmm::load_initrd(vm_memory, &mut initrd_file.unwrap())?;
+                Some(res)
             }
-            None => InitrdInfo {
-                address: GuestAddress(0),
-                size: 0,
-            },
+            None => None,
         };
 
         #[cfg(target_arch = "x86_64")]
@@ -2931,7 +2936,10 @@ mod tests {
         let image = make_test_bin();
         let res = Vmm::load_initrd(&gm, &mut Cursor::new(&image));
         assert!(res.is_err());
-        assert_eq!(LoadInitrdError::LoadInitrd, res.err().unwrap());
+        assert_eq!(
+            LoadInitrdError::LoadInitrd.to_string(),
+            res.err().unwrap().to_string()
+        );
     }
 
     #[test]
@@ -2941,7 +2949,10 @@ mod tests {
 
         let res = Vmm::load_initrd(&gm, &mut Cursor::new(&image));
         assert!(res.is_err());
-        assert_eq!(LoadInitrdError::LoadInitrd, res.err().unwrap());
+        assert_eq!(
+            LoadInitrdError::LoadInitrd.to_string(),
+            res.err().unwrap().to_string()
+        );
     }
 
     #[test]
@@ -3035,14 +3046,7 @@ mod tests {
         #[cfg(target_arch = "aarch64")]
         assert!(vmm.vm.setup_irqchip(1).is_ok());
 
-        let err = vmm
-            .configure_system(&Vec::new())
-            .expect_err("Expected an initrd load error");
-
-        assert_eq!(
-            err.to_string(),
-            StartMicrovmError::InitrdLoader(LoadInitrdError::ReadInitrd).to_string()
-        );
+        assert!(vmm.configure_system(&Vec::new()).is_err());
     }
 
     #[test]