Merge branch 'errors' into 'main'

subplot fix and error message improvements

Closes #14

See merge request larswirzenius/vmadm!29

10 files changed, 181 insertions, 62 deletions

diff --git a/src/cloudinit.rs b/src/cloudinit.rs
index 169b13f..e6d2587 100644
--- a/src/cloudinit.rs
+++ b/src/cloudinit.rs
@@ -13,7 +13,7 @@ use shell_words::quote;
 use std::default::Default;
 use std::fs::write;
 // use std::os::unix::fs::PermissionsExt;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 use tempfile::tempdir;
 
@@ -119,8 +119,8 @@ pub enum CloudInitError {
     KeyError(#[from] KeyError),
 
     /// Something went wrong doing I/O.
-    #[error(transparent)]
-    IoError(#[from] std::io::Error),
+    #[error("could not write to {0}: {1}")]
+    WriteError(PathBuf, #[source] std::io::Error),
 
     /// Something went wrong parsing a string as UTF8.
     #[error(transparent)]
@@ -129,6 +129,14 @@ pub enum CloudInitError {
     /// Something went wrong parsing or serializing to YAML.
     #[error(transparent)]
     SerdeError(#[from] serde_yaml::Error),
+
+    /// Could not create a temporary directory.
+    #[error("couldn't create a temporary directory: {0}")]
+    TempDirError(#[source] std::io::Error),
+
+    /// Could not execute command.
+    #[error("couldn't execute {0}: {1}")]
+    CommandError(String, #[source] std::io::Error),
 }
 
 #[derive(Clone, Debug, Serialize)]
@@ -304,11 +312,13 @@ impl CloudInitConfig {
     pub fn create_dir(&self, path: &Path) -> Result<(), CloudInitError> {
         let metadata = path.join("meta-data");
         debug!("writing metadata to {}", metadata.display());
-        write(&metadata, self.metadata()?)?;
+        write(&metadata, self.metadata()?)
+            .map_err(|err| CloudInitError::WriteError(metadata, err))?;
 
         let userdata = path.join("user-data");
         debug!("writing userdata to {}", userdata.display());
-        write(&userdata, self.userdata()?)?;
+        write(&userdata, self.userdata()?)
+            .map_err(|err| CloudInitError::WriteError(userdata, err))?;
 
         // let scriptsdir = path.join("scripts/per-once");
         // let script = scriptsdir.join("vmadm-configure-sshd");
@@ -328,10 +338,13 @@ impl CloudInitConfig {
     /// The image will be attached to the VM when it starts.
     /// cloud-init finds it via the volume ID (file system label).
     pub fn create_iso(&self, filename: &Path) -> Result<(), CloudInitError> {
-        let dir = tempdir()?;
+        let dir = match tempdir() {
+            Ok(path) => path,
+            Err(err) => return Err(CloudInitError::TempDirError(err)),
+        };
         self.create_dir(dir.path())?;
 
-        let r = Command::new("genisoimage")
+        let r = match Command::new("genisoimage")
             .arg("-quiet")
             .arg("-volid")
             .arg("cidata")
@@ -340,7 +353,11 @@ impl CloudInitConfig {
             .arg("-output")
             .arg(filename)
             .arg(dir.path())
-            .output()?;
+            .output()
+        {
+            Ok(r) => r,
+            Err(err) => return Err(CloudInitError::CommandError("genisoimage".to_string(), err)),
+        };
         if !r.status.success() {
             let stderr = String::from_utf8(r.stderr)?;
             return Err(CloudInitError::IsoFailed(stderr));
diff --git a/src/config.rs b/src/config.rs
index 88c186b..1866971 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -41,9 +41,9 @@ pub struct Configuration {
 /// Errors from this module.
 #[derive(Debug, thiserror::Error)]
 pub enum ConfigurationError {
-    /// I/O error.
-    #[error(transparent)]
-    IoError(#[from] std::io::Error),
+    /// Error reading configuration file.
+    #[error("couldn't read configuration file {0}")]
+    ReadError(PathBuf, #[source] std::io::Error),
 
     /// YAML parsing error.
     #[error(transparent)]
@@ -55,7 +55,8 @@ impl Configuration {
     pub fn from_file(filename: &Path) -> Result<Self, ConfigurationError> {
         if filename.exists() {
             debug!("reading configuration file {}", filename.display());
-            let config = fs::read(filename)?;
+            let config = fs::read(filename)
+                .map_err(|err| ConfigurationError::ReadError(filename.to_path_buf(), err))?;
             let config: Configuration = serde_yaml::from_slice(&config)?;
             debug!("config: {:#?}", config);
             Ok(config)
diff --git a/src/errors.rs b/src/errors.rs
deleted file mode 100644
index ba7875a..0000000
--- a/src/errors.rs
+++ /dev/null
@@ -1,4 +0,0 @@
-use thiserror::Error;
-
-#[derive(Debug, Error)]
-pub enum VmadminError {}
diff --git a/src/image.rs b/src/image.rs
index 62f80f7..7b7c2ce 100644
--- a/src/image.rs
+++ b/src/image.rs
@@ -12,9 +12,13 @@ pub enum ImageError {
     #[error("qemu-img resize failed: {0}")]
     ResizeError(String),
 
-    /// I/O error.
-    #[error(transparent)]
-    IoError(#[from] std::io::Error),
+    /// Base image copy error.
+    #[error("could not copy base image {0} to {1}")]
+    BaseImageCopy(PathBuf, PathBuf, #[source] std::io::Error),
+
+    /// Could not execute command.
+    #[error("couldn't execute {0}: {1}")]
+    CommandError(String, #[source] std::io::Error),
 
     /// Error parsing a string as UTF8.
     #[error(transparent)]
@@ -30,7 +34,9 @@ pub struct VirtualMachineImage {
 impl VirtualMachineImage {
     /// Create new image from a base image.
     pub fn new_from_base(base_image: &Path, image: &Path) -> Result<Self, ImageError> {
-        copy(base_image, image)?;
+        copy(base_image, image).map_err(|err| {
+            ImageError::BaseImageCopy(base_image.to_path_buf(), image.to_path_buf(), err)
+        })?;
         Ok(Self {
             filename: image.to_path_buf(),
         })
@@ -47,7 +53,8 @@ impl VirtualMachineImage {
             .arg("resize")
             .arg(self.filename())
             .arg(format!("{}", new_size))
-            .output()?;
+            .output()
+            .map_err(|err| ImageError::CommandError("qemu-img resize".to_string(), err))?;
         if !r.status.success() {
             let stderr = String::from_utf8(r.stderr)?;
             return Err(ImageError::ResizeError(stderr));
diff --git a/src/install.rs b/src/install.rs
index dce0fea..e74e6ba 100644
--- a/src/install.rs
+++ b/src/install.rs
@@ -17,9 +17,9 @@ pub enum VirtInstallError {
     #[error("virt-install failed: {0}")]
     VirtInstallFailed(String),
 
-    /// I/O error.
-    #[error(transparent)]
-    IoError(#[from] std::io::Error),
+    /// Failed to run virt-install.
+    #[error("couldn't run virt-install")]
+    Run(#[source] std::io::Error),
 
     /// Error parsing a string as UTF8.
     #[error(transparent)]
@@ -115,7 +115,8 @@ pub fn virt_install(args: &VirtInstallArgs, iso: &Path) -> Result<PathBuf, VirtI
         .arg("--graphics=spice")
         .arg("--noautoconsole")
         .arg("--quiet")
-        .output()?;
+        .output()
+        .map_err(|err| VirtInstallError::Run(err))?;
     if !r.status.success() {
         let stderr = String::from_utf8(r.stderr)?;
         return Err(VirtInstallError::VirtInstallFailed(stderr));
diff --git a/src/libvirt.rs b/src/libvirt.rs
index 2162f3c..e65ea1e 100644
--- a/src/libvirt.rs
+++ b/src/libvirt.rs
@@ -13,9 +13,45 @@ use virt::domain::{
 /// Errors from this module.
 #[derive(Debug, thiserror::Error)]
 pub enum VirtError {
-    /// Error creating virtual machine.
-    #[error(transparent)]
-    VirtError(#[from] virt::error::Error),
+    /// Error connecting to libvirtd.
+    #[error("couldn't connect to the libvirt daemon")]
+    Connect(#[source] virt::error::Error),
+
+    /// Error listing domains.
+    #[error("couldn't list all domains")]
+    Domains(#[source] virt::error::Error),
+
+    /// Error listing domains.
+    #[error("couldn't get name of domain")]
+    GetName(#[source] virt::error::Error),
+
+    /// Error checking if domain is active.
+    #[error("couldn't check is domain {0} is active")]
+    IsActive(String, #[source] virt::error::Error),
+
+    /// Error getting domain's XML description.
+    #[error("couldn't get domain's XML description: {0}")]
+    GetXml(String, #[source] virt::error::Error),
+
+    /// Error detaching cloud-init ISO from domain
+    #[error("couldn't detach cloud-init ISO file from domain {0}")]
+    DetachIso(String, #[source] virt::error::Error),
+
+    /// Error detaching drive from domain
+    #[error("couldn't create domain {0}")]
+    Create(String, #[source] virt::error::Error),
+
+    /// Error shutting down domain
+    #[error("couldn't shut down domain {0}")]
+    Shutdown(String, #[source] virt::error::Error),
+
+    /// Error undefining domain
+    #[error("couldn't undefine domain {0}")]
+    Undefine(String, #[source] virt::error::Error),
+
+    /// Error undefining domain
+    #[error("couldn't set domain {0} to be autostarted")]
+    Autostart(String, #[source] virt::error::Error),
 
     /// Error doing I/O.
     #[error(transparent)]
@@ -30,18 +66,21 @@ pub struct Libvirt {
 impl Libvirt {
     pub fn connect(url: &str) -> Result<Self, VirtError> {
         debug!("connecting to libvirtd {}", url);
-        let conn = Connect::open(url)?;
+        let conn = Connect::open(url).map_err(|err| VirtError::Connect(err))?;
         Ok(Self { conn })
     }
 
     fn get_domains(&self) -> Result<Vec<Domain>, VirtError> {
         debug!("listing all domains");
-        Ok(self.conn.list_all_domains(0)?)
+        Ok(self
+            .conn
+            .list_all_domains(0)
+            .map_err(|err| VirtError::Domains(err))?)
     }
 
     fn get_domain(&self, name: &str) -> Result<Option<Domain>, VirtError> {
         for domain in self.get_domains()? {
-            if domain.get_name()? == name {
+            if get_name(&domain)? == name {
                 return Ok(Some(domain));
             }
         }
@@ -51,14 +90,14 @@ impl Libvirt {
     pub fn names(&self) -> Result<Vec<String>, VirtError> {
         let mut ret = vec![];
         for domain in self.get_domains()? {
-            ret.push(domain.get_name()?);
+            ret.push(get_name(&domain)?);
         }
         Ok(ret)
     }
 
     pub fn is_active(&self, name: &str) -> Result<bool, VirtError> {
         if let Some(domain) = self.get_domain(name)? {
-            Ok(domain.is_active()?)
+            Ok(is_active(&domain, name)?)
         } else {
             Ok(false)
         }
@@ -76,12 +115,14 @@ impl Libvirt {
     pub fn detach_cloud_init_iso(&self, name: &str) -> Result<(), VirtError> {
         if let Some(domain) = self.get_domain(name)? {
             debug!("detaching cloud-init ISO from {}", name);
-            let xml = domain.get_xml_desc(0)?;
+            let xml = get_xml(&domain, name)?;
             let disk = find_iso_xml(&xml);
             let flags =
                 VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_AFFECT_CURRENT | VIR_DOMAIN_AFFECT_LIVE;
             if disk.len() > 0 {
-                domain.detach_device_flags(&disk, flags)?;
+                domain
+                    .detach_device_flags(&disk, flags)
+                    .map_err(|err| VirtError::DetachIso(name.to_string(), err))?;
             }
         }
         Ok(())
@@ -89,9 +130,11 @@ impl Libvirt {
 
     pub fn trigger_start(&self, name: &str) -> Result<(), VirtError> {
         if let Some(domain) = self.get_domain(name)? {
-            if !domain.is_active()? {
+            if !is_active(&domain, name)? {
                 debug!("starting {}", name);
-                domain.create()?;
+                domain
+                    .create()
+                    .map_err(|err| VirtError::Create(name.to_string(), err))?;
             }
         }
         Ok(())
@@ -108,8 +151,10 @@ impl Libvirt {
     pub fn trigger_shutdown(&self, name: &str) -> Result<(), VirtError> {
         if let Some(domain) = self.get_domain(name)? {
             debug!("asking {} to shut down", name);
-            if domain.is_active()? {
-                domain.shutdown()?;
+            if is_active(&domain, name)? {
+                domain
+                    .shutdown()
+                    .map_err(|err| VirtError::Shutdown(name.to_string(), err))?;
             }
         }
         Ok(())
@@ -129,7 +174,9 @@ impl Libvirt {
             self.shutdown(name)?;
 
             debug!("undefine {}", name);
-            domain.undefine()?;
+            domain
+                .undefine()
+                .map_err(|err| VirtError::Undefine(name.to_string(), err))?;
 
             debug!("removing image file {}", image.display());
             std::fs::remove_file(image)?;
@@ -140,12 +187,33 @@ impl Libvirt {
 
     pub fn set_autostart(&self, name: &str, autostart: bool) -> Result<(), VirtError> {
         if let Some(domain) = self.get_domain(name)? {
-            domain.set_autostart(autostart)?;
+            domain
+                .set_autostart(autostart)
+                .map_err(|err| VirtError::Autostart(name.to_string(), err))?;
         }
         Ok(())
     }
 }
 
+fn get_name(domain: &Domain) -> Result<String, VirtError> {
+    let name = domain.get_name().map_err(|err| VirtError::GetName(err))?;
+    Ok(name)
+}
+
+fn is_active(domain: &Domain, name: &str) -> Result<bool, VirtError> {
+    let is = domain
+        .is_active()
+        .map_err(|err| VirtError::IsActive(name.to_string(), err))?;
+    Ok(is)
+}
+
+fn get_xml(domain: &Domain, name: &str) -> Result<String, VirtError> {
+    let is = domain
+        .get_xml_desc(0)
+        .map_err(|err| VirtError::GetXml(name.to_string(), err))?;
+    Ok(is)
+}
+
 fn wait_until_inactive(domain: &Domain, name: &str) {
     debug!("waiting for domain {} to become inactive", name);
     let briefly = Duration::from_millis(1000);
diff --git a/src/spec.rs b/src/spec.rs
index b9828e9..a1d1c2f 100644
--- a/src/spec.rs
+++ b/src/spec.rs
@@ -216,14 +216,14 @@ pub enum SpecificationError {
     #[error("No SSH authorized keys specified for {0} and no default configured")]
     NoAuthorizedKeys(String),
 
+    /// Error reading specification file.
+    #[error("Couldn't read specification file {0}")]
+    Read(PathBuf, #[source] std::io::Error),
+
     /// Error reading SSH public key.
     #[error("Failed to read SSH public key file {0}")]
     SshKeyRead(PathBuf, #[source] std::io::Error),
 
-    /// I/O error.
-    #[error(transparent)]
-    IoError(#[from] std::io::Error),
-
     /// Error parsing string as UTF8.
     #[error(transparent)]
     FromUtf8Error(#[from] std::string::FromUtf8Error),
@@ -247,7 +247,8 @@ impl Specification {
         filename: &Path,
     ) -> Result<Vec<Specification>, SpecificationError> {
         debug!("reading specification from {}", filename.display());
-        let spec = fs::read(filename)?;
+        let spec = fs::read(filename)
+            .map_err(|err| SpecificationError::Read(filename.to_path_buf(), err))?;
         let input: HashMap<String, OneVmInputSpecification> = serde_yaml::from_slice(&spec)?;
         debug!("specification as read from file: {:#?}", input);
 
diff --git a/src/sshkeys.rs b/src/sshkeys.rs
index 1425cb3..349cabe 100644
--- a/src/sshkeys.rs
+++ b/src/sshkeys.rs
@@ -3,7 +3,7 @@
 use std::fs::{read, File, Permissions};
 use std::io::Write;
 use std::os::unix::fs::PermissionsExt;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 use tempfile::tempdir;
 
@@ -18,9 +18,29 @@ pub enum KeyError {
     #[error("ssh-keygen failed to certify a key: {0}")]
     CertError(String),
 
-    /// I/O error.
-    #[error(transparent)]
-    IoError(#[from] std::io::Error),
+    /// Error creating a temporary directory.
+    #[error("Couldn't create temporary directory")]
+    TempDir(#[source] std::io::Error),
+
+    /// Error running ssh-keygen.
+    #[error("Couldn't run ssh-keygen")]
+    Run(#[source] std::io::Error),
+
+    /// Error reading a file.
+    #[error("Couldn't read file {0}")]
+    Read(PathBuf, #[source] std::io::Error),
+
+    /// Error writing a file.
+    #[error("Couldn't write file {0}")]
+    Write(PathBuf, #[source] std::io::Error),
+
+    /// Error creating a file.
+    #[error("Couldn't create file {0}")]
+    Create(PathBuf, #[source] std::io::Error),
+
+    /// Error setting file permissions.
+    #[error("Couldn't set permissions for file {0}")]
+    SetPerm(PathBuf, #[source] std::io::Error),
 
     /// Error parsing a string as UTF8.
     #[error(transparent)]
@@ -90,7 +110,7 @@ impl KeyPair {
 
     /// Generate a new key pair of the desired kind.
     pub fn generate(kind: KeyKind) -> Result<Self, KeyError> {
-        let dirname = tempdir()?;
+        let dirname = tempdir().map_err(|err| KeyError::TempDir(err))?;
         let private_key = dirname.path().join("key");
         let output = Command::new("ssh-keygen")
             .arg("-f")
@@ -101,7 +121,8 @@ impl KeyPair {
             .arg(format!("{}", kind.bits()))
             .arg("-N")
             .arg("")
-            .output()?;
+            .output()
+            .map_err(|err| KeyError::Run(err))?;
 
         if !output.status.success() {
             let stderr = String::from_utf8_lossy(&output.stderr).into_owned();
@@ -128,7 +149,7 @@ impl KeyPair {
 }
 
 fn read_string(filename: &Path) -> Result<String, KeyError> {
-    let bytes = read(filename)?;
+    let bytes = read(filename).map_err(|err| KeyError::Read(filename.to_path_buf(), err))?;
     Ok(String::from_utf8(bytes)?)
 }
 
@@ -157,7 +178,7 @@ impl CaKey {
     ///
     /// Return as a string.
     pub fn certify_host(&self, host_key: &KeyPair, hostname: &str) -> Result<String, KeyError> {
-        let dirname = tempdir()?;
+        let dirname = tempdir().map_err(|err| KeyError::TempDir(err))?;
         let ca_key = dirname.path().join("ca");
         let host_key_pub = dirname.path().join("host.pub");
         let cert = dirname.path().join("host-cert.pub");
@@ -174,7 +195,8 @@ impl CaKey {
             .arg("-I")
             .arg(format!("host key for {}", hostname))
             .arg(&host_key_pub)
-            .output()?;
+            .output()
+            .map_err(|err| KeyError::Run(err))?;
 
         if !output.status.success() {
             let stderr = String::from_utf8_lossy(&output.stderr).into_owned();
@@ -186,10 +208,13 @@ impl CaKey {
 }
 
 fn write_string(filename: &Path, s: &str) -> Result<(), KeyError> {
-    let mut file = File::create(filename)?;
+    let mut file =
+        File::create(filename).map_err(|err| KeyError::Create(filename.to_path_buf(), err))?;
     let ro_user = Permissions::from_mode(0o600);
-    file.set_permissions(ro_user)?;
-    file.write_all(s.as_bytes())?;
+    file.set_permissions(ro_user)
+        .map_err(|err| KeyError::SetPerm(filename.to_path_buf(), err))?;
+    file.write_all(s.as_bytes())
+        .map_err(|err| KeyError::Write(filename.to_path_buf(), err))?;
     Ok(())
 }
 
diff --git a/subplot/vmadm.yaml b/subplot/vmadm.yaml
index 76881e1..fb778dd 100644
--- a/subplot/vmadm.yaml
+++ b/subplot/vmadm.yaml
@@ -11,6 +11,9 @@
   function: create_vm
   cleanup: delete_vm
 
+- when: "I invoke vmadm delete --config {config} {filename}"
+  function: delete_vm
+
 - when: "I invoke vmadm shutdown --config {config} {filename}"
   function: shutdown_vm
 
diff --git a/vmadm.md b/vmadm.md
index bfd09b4..24ffe5c 100644
--- a/vmadm.md
+++ b/vmadm.md
@@ -158,7 +158,7 @@ given file .ssh/known_hosts from known_hosts
 
 Then we create the VM, and verify that we can log in.
 
-~~~
+~~~scenario
 when I invoke vmadm new --config config.yaml smoke.yaml
 when I invoke ssh -F .ssh/config debian@smoke hostname
 then stdout contains "smoke"
@@ -171,7 +171,7 @@ then stdout contains "1997"
 Then we shut it down, twice. The second time is to verify shutting
 down works even if the VM is already shut down.
 
-~~~
+~~~scenario
 when I invoke vmadm shutdown --config config.yaml smoke.yaml
 when I invoke vmadm shutdown --config config.yaml smoke.yaml
 ~~~
@@ -179,7 +179,7 @@ when I invoke vmadm shutdown --config config.yaml smoke.yaml
 Then we start it back up again and verify we can log in. Then we start
 it again, while it's already running, to verify that that works.
 
-~~~
+~~~scenario
 when I invoke vmadm start --config config.yaml smoke.yaml
 when I invoke ssh -F .ssh/config debian@smoke hostname
 when I invoke vmadm start --config config.yaml smoke.yaml
@@ -187,7 +187,7 @@ when I invoke vmadm start --config config.yaml smoke.yaml
 
 Finally, we delete it twice.
 
-~~~
+~~~scenario
 when I invoke vmadm delete --config config.yaml smoke.yaml
 when I invoke vmadm delete --config config.yaml smoke.yaml
 ~~~