arch: Replace Box<dyn Error> with error enum

Avoiding Box<dyn Error> makes it less likely that we display errors with
insufficient context by accident.

Many of the errors touched in this CL already had helpful message
written! But those corresponding enum variants were never being
instantiated, and that bug was masked by Box<dyn Error>. For example see
the Error::LoadCmdline and Error::LoadKernel.

    pub enum Error {
        LoadCmdline(kernel_loader::Error),
        ...
    }

Before this CL:

    // Bug: boxes the underlying error without adding LoadCmdline
    kernel_loader::load_cmdline(...)?;

After this CL:

    kernel_loader::load_cmdline(...).map_err(Error::LoadCmdline)?;

TEST=cargo check
TEST=cargo check --all-features
TEST=cargo check --target aarch64-unknown-linux-gnu

Change-Id: I7c0cff843c2211565226b9dfb4142ad6b7fa15ac
Reviewed-on: https://chromium-review.googlesource.com/1502112
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: David Tolnay <dtolnay@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>

2 files changed, 37 insertions, 47 deletions

diff --git a/arch/src/fdt.rs b/arch/src/fdt.rs
index 5b4961d..7761def 100644
--- a/arch/src/fdt.rs
+++ b/arch/src/fdt.rs
@@ -37,8 +37,6 @@ pub enum Error {
     FdtGuestMemoryWriteError,
 }
 
-impl std::error::Error for Error {}
-
 impl Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use self::Error::*;
@@ -59,27 +57,31 @@ impl Display for Error {
     }
 }
 
-pub fn begin_node(fdt: &mut Vec<u8>, name: &str) -> Result<(), Box<Error>> {
+pub type Result<T> = std::result::Result<T, Error>;
+
+impl std::error::Error for Error {}
+
+pub fn begin_node(fdt: &mut Vec<u8>, name: &str) -> Result<()> {
     let cstr_name = CString::new(name).unwrap();
 
     // Safe because we allocated fdt and converted name to a CString
     let fdt_ret = unsafe { fdt_begin_node(fdt.as_mut_ptr() as *mut c_void, cstr_name.as_ptr()) };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtBeginNodeError(fdt_ret)));
+        return Err(Error::FdtBeginNodeError(fdt_ret));
     }
     Ok(())
 }
 
-pub fn end_node(fdt: &mut Vec<u8>) -> Result<(), Box<Error>> {
+pub fn end_node(fdt: &mut Vec<u8>) -> Result<()> {
     // Safe because we allocated fdt
     let fdt_ret = unsafe { fdt_end_node(fdt.as_mut_ptr() as *mut c_void) };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtEndNodeError(fdt_ret)));
+        return Err(Error::FdtEndNodeError(fdt_ret));
     }
     Ok(())
 }
 
-pub fn property(fdt: &mut Vec<u8>, name: &str, val: &[u8]) -> Result<(), Box<Error>> {
+pub fn property(fdt: &mut Vec<u8>, name: &str, val: &[u8]) -> Result<()> {
     let cstr_name = CString::new(name).unwrap();
     let val_ptr = val.as_ptr() as *const c_void;
 
@@ -93,7 +95,7 @@ pub fn property(fdt: &mut Vec<u8>, name: &str, val: &[u8]) -> Result<(), Box<Err
         )
     };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtPropertyError(fdt_ret)));
+        return Err(Error::FdtPropertyError(fdt_ret));
     }
     Ok(())
 }
@@ -110,11 +112,11 @@ fn cpu_to_fdt64(input: u64) -> [u8; 8] {
     buf
 }
 
-pub fn property_u32(fdt: &mut Vec<u8>, name: &str, val: u32) -> Result<(), Box<Error>> {
+pub fn property_u32(fdt: &mut Vec<u8>, name: &str, val: u32) -> Result<()> {
     property(fdt, name, &cpu_to_fdt32(val))
 }
 
-pub fn property_u64(fdt: &mut Vec<u8>, name: &str, val: u64) -> Result<(), Box<Error>> {
+pub fn property_u64(fdt: &mut Vec<u8>, name: &str, val: u64) -> Result<()> {
     property(fdt, name, &cpu_to_fdt64(val))
 }
 
@@ -136,7 +138,7 @@ pub fn generate_prop64(cells: &[u64]) -> Vec<u8> {
     ret
 }
 
-pub fn property_null(fdt: &mut Vec<u8>, name: &str) -> Result<(), Box<Error>> {
+pub fn property_null(fdt: &mut Vec<u8>, name: &str) -> Result<()> {
     let cstr_name = CString::new(name).unwrap();
 
     // Safe because we allocated fdt, converted name to a CString
@@ -149,16 +151,12 @@ pub fn property_null(fdt: &mut Vec<u8>, name: &str) -> Result<(), Box<Error>> {
         )
     };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtPropertyError(fdt_ret)));
+        return Err(Error::FdtPropertyError(fdt_ret));
     }
     Ok(())
 }
 
-pub fn property_cstring(
-    fdt: &mut Vec<u8>,
-    name: &str,
-    cstr_value: &CStr,
-) -> Result<(), Box<Error>> {
+pub fn property_cstring(fdt: &mut Vec<u8>, name: &str, cstr_value: &CStr) -> Result<()> {
     let value_bytes = cstr_value.to_bytes_with_nul();
     let cstr_name = CString::new(name).unwrap();
 
@@ -172,40 +170,36 @@ pub fn property_cstring(
         )
     };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtPropertyError(fdt_ret)));
+        return Err(Error::FdtPropertyError(fdt_ret));
     }
     Ok(())
 }
 
-pub fn property_string(fdt: &mut Vec<u8>, name: &str, value: &str) -> Result<(), Box<Error>> {
+pub fn property_string(fdt: &mut Vec<u8>, name: &str, value: &str) -> Result<()> {
     let cstr_value = CString::new(value).unwrap();
     property_cstring(fdt, name, &cstr_value)
 }
 
-pub fn start_fdt(fdt: &mut Vec<u8>, fdt_max_size: usize) -> Result<(), Box<Error>> {
+pub fn start_fdt(fdt: &mut Vec<u8>, fdt_max_size: usize) -> Result<()> {
     // Safe since we allocated this array with fdt_max_size
     let mut fdt_ret = unsafe { fdt_create(fdt.as_mut_ptr() as *mut c_void, fdt_max_size as c_int) };
 
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtCreateError(fdt_ret)));
+        return Err(Error::FdtCreateError(fdt_ret));
     }
     // Safe since we allocated this array
     fdt_ret = unsafe { fdt_finish_reservemap(fdt.as_mut_ptr() as *mut c_void) };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtFinishReservemapError(fdt_ret)));
+        return Err(Error::FdtFinishReservemapError(fdt_ret));
     }
     Ok(())
 }
 
-pub fn finish_fdt(
-    fdt: &mut Vec<u8>,
-    fdt_final: &mut Vec<u8>,
-    fdt_max_size: usize,
-) -> Result<(), Box<Error>> {
+pub fn finish_fdt(fdt: &mut Vec<u8>, fdt_final: &mut Vec<u8>, fdt_max_size: usize) -> Result<()> {
     // Safe since we allocated fdt_final and previously passed in it's size
     let mut fdt_ret = unsafe { fdt_finish(fdt.as_mut_ptr() as *mut c_void) };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtFinishError(fdt_ret)));
+        return Err(Error::FdtFinishError(fdt_ret));
     }
 
     // Safe because we allocated both arrays with the correct size
@@ -217,13 +211,13 @@ pub fn finish_fdt(
         )
     };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtOpenIntoError(fdt_ret)));
+        return Err(Error::FdtOpenIntoError(fdt_ret));
     }
 
     // Safe since we allocated fdt_final
     fdt_ret = unsafe { fdt_pack(fdt_final.as_mut_ptr() as *mut c_void) };
     if fdt_ret != 0 {
-        return Err(Box::new(Error::FdtPackError(fdt_ret)));
+        return Err(Error::FdtPackError(fdt_ret));
     }
     Ok(())
 }
diff --git a/arch/src/lib.rs b/arch/src/lib.rs
index d725baa..aa6c8a3 100644
--- a/arch/src/lib.rs
+++ b/arch/src/lib.rs
@@ -15,11 +15,11 @@ extern crate sync;
 extern crate sys_util;
 
 use std::collections::BTreeMap;
+use std::error::Error as StdError;
 use std::fmt::{self, Display};
 use std::fs::File;
-use std::io::{Read, Seek, SeekFrom};
+use std::io::{self, Read, Seek, SeekFrom};
 use std::os::unix::io::AsRawFd;
-use std::result;
 use std::sync::Arc;
 
 use devices::virtio::VirtioDevice;
@@ -33,8 +33,6 @@ use resources::SystemAllocator;
 use sync::Mutex;
 use sys_util::{syslog, EventFd, GuestAddress, GuestMemory, GuestMemoryError};
 
-pub type Result<T> = result::Result<T, Box<std::error::Error>>;
-
 /// Holds the pieces needed to build a VM. Passed to `build_vm` in the `LinuxArch` trait below to
 /// create a `RunnableLinuxVm`.
 pub struct VmComponents {
@@ -70,6 +68,8 @@ pub struct VirtioDeviceStub {
 /// Trait which is implemented for each Linux Architecture in order to
 /// set up the memory, cpus, and system devices and to boot the kernel.
 pub trait LinuxArch {
+    type Error: StdError;
+
     /// Takes `VmComponents` and generates a `RunnableLinuxVm`.
     ///
     /// # Arguments
@@ -77,16 +77,14 @@ pub trait LinuxArch {
     /// * `components` - Parts to use to build the VM.
     /// * `split_irqchip` - whether to use a split IRQ chip (i.e. userspace PIT/PIC/IOAPIC)
     /// * `create_devices` - Function to generate a list of devices.
-    fn build_vm<F>(
+    fn build_vm<F, E>(
         components: VmComponents,
         split_irqchip: bool,
         create_devices: F,
-    ) -> Result<RunnableLinuxVm>
+    ) -> Result<RunnableLinuxVm, Self::Error>
     where
-        F: FnOnce(
-            &GuestMemory,
-            &EventFd,
-        ) -> Result<Vec<(Box<PciDevice + 'static>, Option<Minijail>)>>;
+        F: FnOnce(&GuestMemory, &EventFd) -> Result<Vec<(Box<PciDevice>, Option<Minijail>)>, E>,
+        E: StdError + 'static;
 }
 
 /// Errors for device manager.
@@ -141,14 +139,12 @@ impl Display for DeviceRegistrationError {
 
 /// Creates a root PCI device for use by this Vm.
 pub fn generate_pci_root(
-    devices: Vec<(Box<PciDevice + 'static>, Option<Minijail>)>,
+    devices: Vec<(Box<PciDevice>, Option<Minijail>)>,
     mmio_bus: &mut Bus,
     resources: &mut SystemAllocator,
     vm: &mut Vm,
-) -> std::result::Result<
-    (PciRoot, Vec<(u32, PciInterruptPin)>, BTreeMap<u32, String>),
-    DeviceRegistrationError,
-> {
+) -> Result<(PciRoot, Vec<(u32, PciInterruptPin)>, BTreeMap<u32, String>), DeviceRegistrationError>
+{
     let mut root = PciRoot::new();
     let mut pci_irqs = Vec::new();
     let mut pid_labels = BTreeMap::new();
@@ -214,7 +210,7 @@ pub fn generate_pci_root(
 /// Errors for image loading.
 #[derive(Debug)]
 pub enum LoadImageError {
-    Seek(std::io::Error),
+    Seek(io::Error),
     ImageSizeTooLarge(u64),
     ReadToMemory(GuestMemoryError),
 }
@@ -246,7 +242,7 @@ pub fn load_image<F>(
     image: &mut F,
     guest_addr: GuestAddress,
     max_size: u64,
-) -> std::result::Result<usize, LoadImageError>
+) -> Result<usize, LoadImageError>
 where
     F: Read + Seek,
 {