nzr-api, et al: implement a serializable ApiError

This replaces all the API functions that returned Result<T, String>.

Additionally, ToApiResult<T> and Simplify<T> make converting errors to
ApiError easier than with String.

client/src/main.rs

@@ -1,5 +1,6 @@
 use clap::{CommandFactory, FromArgMatches, Parser, Subcommand};
 use nzr_api::config;
+use nzr_api::error::Simplify;
 use nzr_api::hickory_proto::rr::Name;
 use nzr_api::model;
 use nzr_api::net::cidr::CidrV4;
@@ -332,9 +333,9 @@ async fn handle_command() -> Result<(), Box<dyn std::error::Error>> {
                 let mut net = client
                     .get_subnets(nzr_api::default_ctx())
                     .await
-                    .map_err(|e| e.to_string())
+                    .simplify()
                     .and_then(|res| {
-                        res?.iter()
+                        res.iter()
                             .find_map(|ent| {
                                 if ent.name == args.name {
                                     Some(ent.clone())
@@ -342,7 +343,7 @@ async fn handle_command() -> Result<(), Box<dyn std::error::Error>> {
                                     None
                                 }
                             })
-                            .ok_or_else(|| format!("Couldn't find network {}", &args.name))
+                            .ok_or_else(|| format!("Couldn't find network {}", &args.name).into())
                     })?;
 
                 // merge in the new args
@@ -365,15 +366,11 @@ async fn handle_command() -> Result<(), Box<dyn std::error::Error>> {
                 }
 
                 // run the update
-                client
+                let net = client
                     .modify_subnet(nzr_api::default_ctx(), net)
                     .await
-                    .map_err(|err| format!("RPC error: {}", err))
+                    .simplify()?;
-                    .and_then(|res| {
+                println!("Subnet {} updated.", net.name);
-                        res.map(|e| {
-                            println!("Subnet {} updated.", e.name);
-                        })
-                    })?;
             }
             NetCmd::Dump { name } => {
                 let subnets = (client.get_subnets(nzr_api::default_ctx()).await?)?;

nzr-api/src/error.rs

@@ -0,0 +1,208 @@
+use std::fmt;
+
+use serde::{Deserialize, Serialize};
+use tarpc::client::RpcError;
+
+#[derive(Clone, Copy, Debug, Serialize, Deserialize)]
+pub enum ErrorType {
+    /// Entity was not found.
+    NotFound,
+    /// Error occurred with a database call.
+    Database,
+    /// Error occurred in a libvirt call.
+    VirtError,
+    /// Error occurred while parsing input.
+    Parse,
+    /// An unknown API error occurred.
+    Other,
+}
+
+impl ErrorType {
+    fn as_str(&self) -> &'static str {
+        match self {
+            ErrorType::NotFound => "Entity not found",
+            ErrorType::Database => "Database error",
+            ErrorType::VirtError => "libvirt error",
+            ErrorType::Parse => "Unable to parse input",
+            ErrorType::Other => "Unknown API error",
+        }
+    }
+}
+
+impl fmt::Display for ErrorType {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.as_str().fmt(f)
+    }
+}
+
+#[derive(Debug, Serialize, Deserialize)]
+pub struct ApiError {
+    error_type: ErrorType,
+    message: Option<String>,
+    inner: Option<InnerError>,
+}
+
+impl fmt::Display for ApiError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.message
+            .as_deref()
+            .unwrap_or_else(|| self.error_type.as_str())
+            .fmt(f)?;
+        if let Some(inner) = &self.inner {
+            write!(f, ": {inner}")?;
+        }
+        Ok(())
+    }
+}
+
+impl std::error::Error for ApiError {}
+
+impl ApiError {
+    pub fn new<E>(error_type: ErrorType, message: impl Into<String>, err: E) -> Self
+    where
+        E: std::error::Error,
+    {
+        Self {
+            error_type,
+            message: Some(message.into()),
+            inner: Some(err.into()),
+        }
+    }
+
+    pub fn err_type(&self) -> ErrorType {
+        self.error_type
+    }
+}
+
+impl From<ErrorType> for ApiError {
+    fn from(value: ErrorType) -> Self {
+        Self {
+            error_type: value,
+            message: None,
+            inner: None,
+        }
+    }
+}
+
+impl<T> From<T> for ApiError
+where
+    T: AsRef<str>,
+{
+    fn from(value: T) -> Self {
+        let value = value.as_ref();
+        Self {
+            error_type: ErrorType::Other,
+            message: Some(value.to_owned()),
+            inner: None,
+        }
+    }
+}
+
+#[derive(Serialize, Deserialize)]
+struct InnerError {
+    error_debug: String,
+    error_message: String,
+}
+
+impl fmt::Debug for InnerError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.error_debug.fmt(f)
+    }
+}
+
+impl fmt::Display for InnerError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.error_message.fmt(f)
+    }
+}
+
+impl<E> From<E> for InnerError
+where
+    E: std::error::Error,
+{
+    fn from(value: E) -> Self {
+        Self {
+            error_debug: format!("{value:?}"),
+            error_message: format!("{value}"),
+        }
+    }
+}
+
+pub trait ToApiResult<T> {
+    /// Converts the result's error type to [`ApiError`] with [`ErrorType::Other`].
+    ///
+    /// [`ApiError`]: nzr-api::error::ApiError
+    /// [`ErrorType::Other`]: nzr-api::error::ErrorType::Other
+    fn to_api(self) -> Result<T, ApiError>;
+    /// Converts the result's error type to [`ApiError`] with
+    /// [`ErrorType::Other`], and the given context.
+    ///
+    /// [`ApiError`]: nzr-api::error::ApiError
+    /// [`ErrorType::Other`]: nzr-api::error::ErrorType::Other
+    fn to_api_with(self, context: impl AsRef<str>) -> Result<T, ApiError>;
+    /// Converts the result's error type to [`ApiError`] with the given
+    /// [`ErrorType`] and context.
+    ///
+    /// [`ApiError`]: nzr-api::error::ApiError
+    /// [`ErrorType`]: nzr-api::error::ErrorType
+    fn to_api_with_type(self, err_type: ErrorType, context: impl AsRef<str>)
+        -> Result<T, ApiError>;
+    /// Converts the result's error type to [`ApiError`] with the given
+    /// [`ErrorType`].
+    ///
+    /// [`ApiError`]: nzr-api::error::ApiError
+    /// [`ErrorType`]: nzr-api::error::ErrorType
+    fn to_api_type(self, err_type: ErrorType) -> Result<T, ApiError>;
+}
+
+impl<T, E> ToApiResult<T> for Result<T, E>
+where
+    E: std::error::Error + 'static,
+{
+    fn to_api(self) -> Result<T, ApiError> {
+        self.map_err(|e| ApiError {
+            error_type: ErrorType::Other,
+            message: None,
+            inner: Some(e.into()),
+        })
+    }
+
+    fn to_api_with(self, context: impl AsRef<str>) -> Result<T, ApiError> {
+        self.map_err(|e| ApiError {
+            error_type: ErrorType::Other,
+            message: Some(context.as_ref().to_owned()),
+            inner: Some(e.into()),
+        })
+    }
+
+    fn to_api_type(self, err_type: ErrorType) -> Result<T, ApiError> {
+        self.map_err(|e| ApiError {
+            error_type: err_type,
+            message: None,
+            inner: Some(e.into()),
+        })
+    }
+
+    fn to_api_with_type(
+        self,
+        err_type: ErrorType,
+        context: impl AsRef<str>,
+    ) -> Result<T, ApiError> {
+        self.map_err(|e| ApiError {
+            error_type: err_type,
+            message: Some(context.as_ref().to_owned()),
+            inner: Some(e.into()),
+        })
+    }
+}
+
+pub trait Simplify<T> {
+    fn simplify(self) -> Result<T, ApiError>;
+}
+
+impl<T> Simplify<T> for Result<Result<T, ApiError>, RpcError> {
+    /// Flattens a Result of `RpcError` and `ApiError` to just `ApiError`.
+    fn simplify(self) -> Result<T, ApiError> {
+        self.to_api_with("RPC Error").and_then(|r| r)
+    }
+}

nzr-api/src/lib.rs

@@ -1,9 +1,11 @@
 use std::net::Ipv4Addr;
 
+use error::ApiError;
 use model::{CreateStatus, Instance, SshPubkey, Subnet};
 
 pub mod args;
 pub mod config;
+pub mod error;
 pub mod event;
 #[cfg(feature = "mock")]
 pub mod mock;
@@ -24,40 +26,40 @@ pub enum InstanceQuery {
 #[tarpc::service]
 pub trait Nazrin {
     /// Creates a new instance.
-    async fn new_instance(build_args: args::NewInstance) -> Result<uuid::Uuid, String>;
+    async fn new_instance(build_args: args::NewInstance) -> Result<uuid::Uuid, ApiError>;
     /// Poll for the current status of an instance being created.
     async fn poll_new_instance(task_id: uuid::Uuid) -> Option<CreateStatus>;
     /// Deletes an existing instance.
     ///
     /// This should involve deleting all related disks and clearing
     /// the lease information from the subnet data, if any.
-    async fn delete_instance(name: String) -> Result<(), String>;
+    async fn delete_instance(name: String) -> Result<(), ApiError>;
     /// Gets a single instance by the given InstanceQuery.
-    async fn find_instance(query: InstanceQuery) -> Result<Option<Instance>, String>;
+    async fn find_instance(query: InstanceQuery) -> Result<Option<Instance>, ApiError>;
     /// Gets a list of existing instances.
-    async fn get_instances(with_status: bool) -> Result<Vec<Instance>, String>;
+    async fn get_instances(with_status: bool) -> Result<Vec<Instance>, ApiError>;
     /// Cleans up unusable entries in the database.
-    async fn garbage_collect() -> Result<(), String>;
+    async fn garbage_collect() -> Result<(), ApiError>;
     /// Creates a new subnet.
     ///
     /// Unlike instances, subnets shouldn't perform any changes to the
     /// interfaces they reference. This should be used primarily for
     /// ease-of-use and bookkeeping (e.g., assigning dynamic leases).
-    async fn new_subnet(build_args: Subnet) -> Result<Subnet, String>;
+    async fn new_subnet(build_args: Subnet) -> Result<Subnet, ApiError>;
     /// Modifies an existing subnet.
-    async fn modify_subnet(edit_args: Subnet) -> Result<Subnet, String>;
+    async fn modify_subnet(edit_args: Subnet) -> Result<Subnet, ApiError>;
     /// Gets a list of existing subnets.
-    async fn get_subnets() -> Result<Vec<Subnet>, String>;
+    async fn get_subnets() -> Result<Vec<Subnet>, ApiError>;
     /// Deletes an existing subnet.
-    async fn delete_subnet(interface: String) -> Result<(), String>;
+    async fn delete_subnet(interface: String) -> Result<(), ApiError>;
     /// Gets the cloud-init user-data for the given instance.
-    async fn get_instance_userdata(id: i32) -> Result<Vec<u8>, String>;
+    async fn get_instance_userdata(id: i32) -> Result<Vec<u8>, ApiError>;
     /// Gets all SSH keys stored in the database.
-    async fn get_ssh_pubkeys() -> Result<Vec<SshPubkey>, String>;
+    async fn get_ssh_pubkeys() -> Result<Vec<SshPubkey>, ApiError>;
     /// Adds a new SSH public key to the database.
-    async fn add_ssh_pubkey(pub_key: String) -> Result<SshPubkey, String>;
+    async fn add_ssh_pubkey(pub_key: String) -> Result<SshPubkey, ApiError>;
     /// Deletes an SSH public key from the database.
-    async fn delete_ssh_pubkey(id: i32) -> Result<(), String>;
+    async fn delete_ssh_pubkey(id: i32) -> Result<(), ApiError>;
 }
 
 /// Create a new NazrinClient.

nzr-api/src/mock/client.rs

@@ -1,20 +1,20 @@
 use std::net::Ipv4Addr;
 
-use crate::{args, model, net::cidr::CidrV4};
+use crate::{args, error::ApiError, model, net::cidr::CidrV4};
 
 pub trait NzrClientExt {
     #[allow(async_fn_in_trait)]
     async fn new_mock_instance(
         &mut self,
         name: impl AsRef<str>,
-    ) -> Result<Result<model::Instance, String>, crate::RpcError>;
+    ) -> Result<Result<model::Instance, ApiError>, crate::RpcError>;
 }
 
 impl NzrClientExt for crate::NazrinClient {
     async fn new_mock_instance(
         &mut self,
         name: impl AsRef<str>,
-    ) -> Result<Result<model::Instance, String>, crate::RpcError> {
+    ) -> Result<Result<model::Instance, ApiError>, crate::RpcError> {
         let name = name.as_ref().to_owned();
 
         let subnet = self

nzr-api/src/mock/mod.rs

@@ -10,6 +10,7 @@ use futures::{future, StreamExt};
 use tokio::{sync::RwLock, task::JoinHandle};
 
 use crate::{
+    error::ApiError,
     model,
     net::{cidr::CidrV4, mac::MacAddr},
     InstanceQuery, Nazrin, NazrinClient,
@@ -62,14 +63,14 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         build_args: crate::args::NewInstance,
-    ) -> Result<uuid::Uuid, String> {
+    ) -> Result<uuid::Uuid, ApiError> {
         let mut db = self.db.write().await;
         let Some(net_pos) = db
             .subnets
             .iter()
             .position(|s| s.as_ref().filter(|s| s.name == build_args.subnet).is_some())
         else {
-            return Err("Subnet doesn't exist".to_owned());
+            return Err("Subnet doesn't exist".into());
         };
         let subnet = db.subnets[net_pos].as_ref().unwrap().clone();
         let cur_lease = *(db
@@ -134,7 +135,11 @@ impl Nazrin for MockServer {
         }
     }
 
-    async fn delete_instance(self, _: tarpc::context::Context, name: String) -> Result<(), String> {
+    async fn delete_instance(
+        self,
+        _: tarpc::context::Context,
+        name: String,
+    ) -> Result<(), ApiError> {
         let mut db = self.db.write().await;
         let Some(inst) = db
             .instances
@@ -142,7 +147,7 @@ impl Nazrin for MockServer {
             .find(|i| i.as_ref().filter(|i| i.name == name).is_some())
             .take()
         else {
-            return Err("Instance doesn't exist".to_owned());
+            return Err("Instance doesn't exist".into());
         };
         inst.take();
         Ok(())
@@ -152,7 +157,7 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         query: crate::InstanceQuery,
-    ) -> Result<Option<crate::model::Instance>, String> {
+    ) -> Result<Option<crate::model::Instance>, ApiError> {
         let db = self.db.read().await;
 
         let res = {
@@ -177,7 +182,7 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         id: i32,
-    ) -> Result<Vec<u8>, String> {
+    ) -> Result<Vec<u8>, ApiError> {
         let db = self.db.read().await;
         let Some(inst) = db
             .instances
@@ -185,7 +190,7 @@ impl Nazrin for MockServer {
             .find(|i| i.as_ref().map(|i| i.id == id).is_some())
             .and_then(|o| o.as_ref())
         else {
-            return Err("No such instance".to_owned());
+            return Err("No such instance".into());
         };
         Ok(db.ci_userdatas.get(&inst.name).cloned().unwrap_or_default())
     }
@@ -194,7 +199,7 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         _with_status: bool,
-    ) -> Result<Vec<crate::model::Instance>, String> {
+    ) -> Result<Vec<crate::model::Instance>, ApiError> {
         let db = self.db.read().await;
         Ok(db
             .instances
@@ -207,7 +212,7 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         build_args: crate::model::Subnet,
-    ) -> Result<crate::model::Subnet, String> {
+    ) -> Result<crate::model::Subnet, ApiError> {
         let mut db = self.db.write().await;
         let subnet = build_args.clone();
         db.subnets.push(Some(build_args));
@@ -218,14 +223,14 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         _edit_args: crate::model::Subnet,
-    ) -> Result<crate::model::Subnet, String> {
+    ) -> Result<crate::model::Subnet, ApiError> {
         todo!()
     }
 
     async fn get_subnets(
         self,
         _: tarpc::context::Context,
-    ) -> Result<Vec<crate::model::Subnet>, String> {
+    ) -> Result<Vec<crate::model::Subnet>, ApiError> {
         let db = self.db.read().await;
         Ok(db.subnets.iter().filter_map(|net| net.clone()).collect())
     }
@@ -234,7 +239,7 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         interface: String,
-    ) -> Result<(), String> {
+    ) -> Result<(), ApiError> {
         let mut db = self.db.write().await;
         db.instances
             .iter()
@@ -249,20 +254,20 @@ impl Nazrin for MockServer {
             .iter_mut()
             .find(|net| net.as_ref().filter(|n| n.name == interface).is_some())
         else {
-            return Err("Subnet doesn't exist".to_owned());
+            return Err("Subnet doesn't exist".into());
         };
         subnet.take();
         Ok(())
     }
 
-    async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), String> {
+    async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), ApiError> {
         todo!()
     }
 
     async fn get_ssh_pubkeys(
         self,
         _: tarpc::context::Context,
-    ) -> Result<Vec<model::SshPubkey>, String> {
+    ) -> Result<Vec<model::SshPubkey>, ApiError> {
         let db = self.db.read().await;
 
         Ok(db
@@ -276,7 +281,7 @@ impl Nazrin for MockServer {
         self,
         _: tarpc::context::Context,
         pub_key: String,
-    ) -> Result<model::SshPubkey, String> {
+    ) -> Result<model::SshPubkey, ApiError> {
         let mut key_model = model::SshPubkey::from_str(&pub_key).map_err(|e| e.to_string())?;
         let mut db = self.db.write().await;
         key_model.id = Some(db.ssh_keys.len() as i32);
@@ -284,7 +289,7 @@ impl Nazrin for MockServer {
         Ok(key_model)
     }
 
-    async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), String> {
+    async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), ApiError> {
         let mut db = self.db.write().await;
         if let Some(key) = db.ssh_keys.get_mut(id as usize) {
             key.take();

nzr-api/src/model.rs

@@ -5,7 +5,10 @@ use serde::{Deserialize, Serialize};
 use std::{fmt, net::Ipv4Addr};
 use thiserror::Error;
 
-use crate::net::{cidr::CidrV4, mac::MacAddr};
+use crate::{
+    error::ApiError,
+    net::{cidr::CidrV4, mac::MacAddr},
+};
 
 #[derive(Copy, Clone, Debug, Serialize, Deserialize)]
 #[repr(u32)]
@@ -67,7 +70,7 @@ impl fmt::Display for DomainState {
 pub struct CreateStatus {
     pub status_text: String,
     pub completion: f32,
-    pub result: Option<Result<Instance, String>>,
+    pub result: Option<Result<Instance, ApiError>>,
 }
 
 /// Struct representing a VM instance.

nzrd/src/cmd/mod.rs

@@ -1,23 +1 @@
-pub mod net;
 pub mod vm;
-
-use std::fmt;
-
-#[derive(Debug)]
-pub struct CommandError(String);
-
-impl fmt::Display for CommandError {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", self.0)
-    }
-}
-
-impl std::error::Error for CommandError {}
-
-macro_rules! cmd_error {
-    ($($arg:tt)*) => {
-        Box::new(CommandError(format!($($arg)*)))
-    };
-}
-
-pub(crate) use cmd_error;

nzrd/src/cmd/net.rs

@@ -1,41 +0,0 @@
-use super::*;
-use crate::ctx::Context;
-use crate::model::tx::Transaction;
-use crate::model::Subnet;
-use nzr_api::model;
-
-pub async fn add_subnet(
-    ctx: &Context,
-    args: model::Subnet,
-) -> Result<Subnet, Box<dyn std::error::Error>> {
-    let subnet = {
-        let s = Subnet::insert(ctx, args.name, args.data)
-            .await
-            .map_err(|er| cmd_error!("Couldn't generate subnet: {}", er))?;
-        Transaction::begin(ctx, s)
-    };
-
-    Ok(subnet.take())
-}
-
-pub async fn delete_subnet(
-    ctx: &Context,
-    name: impl AsRef<str>,
-) -> Result<(), Box<dyn std::error::Error>> {
-    match Subnet::get_by_name(ctx, name.as_ref())
-        .await
-        .map_err(|er| cmd_error!("Couldn't find subnet: {}", er))?
-    {
-        Some(subnet) => {
-            // TODO: notify clients
-
-            subnet
-                .delete(ctx)
-                .await
-                .map_err(|er| cmd_error!("Couldn't fully delete subnet entry: {}", er))
-        }
-        None => Err(cmd_error!("Subnet not found")),
-    }?;
-
-    Ok(())
-}

nzrd/src/cmd/vm.rs

@@ -1,3 +1,4 @@
+use nzr_api::error::{ApiError, ErrorType, ToApiResult};
 use nzr_api::net::cidr::CidrV4;
 use nzr_virt::error::DomainError;
 use nzr_virt::xml::build::DomainBuilder;
@@ -5,9 +6,9 @@ use nzr_virt::xml::{self, InfoMap, SerialType, Sysinfo};
 use nzr_virt::{datasize, dom, vol};
 use tokio::sync::RwLock;
 
-use super::*;
 use crate::ctrl::vm::Progress;
 use crate::ctx::Context;
+use crate::model::tx::Transaction;
 use crate::model::{Instance, Subnet};
 use nzr_api::net::mac::MacAddr;
 use nzr_api::{args, model, nzr_event};
@@ -29,23 +30,21 @@ pub async fn new_instance(
     ctx: Context,
     prog_task: Arc<RwLock<Progress>>,
     args: &args::NewInstance,
-) -> Result<(Instance, dom::Domain), Box<dyn std::error::Error>> {
+) -> Result<(Instance, dom::Domain), ApiError> {
     progress!(prog_task, 0.0, "Starting...");
     // find the subnet corresponding to the interface
     let subnet = Subnet::get_by_name(&ctx, &args.subnet)
         .await
-        .map_err(|er| cmd_error!("Unable to get interface: {}", er))?
+        .to_api_with("Unable to get interface")?
-        .ok_or(cmd_error!(
+        .ok_or::<ApiError>(format!("Subnet {} wasn't found in database", &args.subnet).into())?;
-            "Subnet {} wasn't found in database",
-            &args.subnet
-        ))?;
 
     // bail if a domain already exists
     if let Ok(dom) = ctx.virt.conn.get_instance(&args.name).await {
-        Err(cmd_error!(
+        Err(format!(
             "Domain with name already exists (uuid {})",
             dom.xml().await.uuid,
-        ))
+        )
+        .into())
     } else {
         // make sure the base image exists
         let mut base_image = ctx
@@ -54,7 +53,7 @@ pub async fn new_instance(
             .baseimg
             .volume(&args.base_image)
             .await
-            .map_err(|er| cmd_error!("Couldn't find base image: {}", er))?;
+            .to_api_with("Couldn't find base image")?;
         progress!(prog_task, 10.0, "Generating metadata...");
 
         // generate a new lease with a new MAC addr
@@ -62,19 +61,23 @@ pub async fn new_instance(
             let bytes = [VIRT_MAC_OUI, rand::random::<[u8; 3]>().as_ref()].concat();
             MacAddr::from_bytes(bytes)
         }
-        .map_err(|er| cmd_error!("Unable to create a new MAC address: {}", er))?;
+        .to_api_with("Unable to create a new MAC address")?;
 
         // Get highest host addr + 1 for our new addr
         let addr = {
             let addr_num = Instance::all_in_subnet(&ctx, &subnet)
-                .await?
+                .await
+                .to_api_with("Couldn't get instances in subnet")?
                 .into_iter()
                 .max_by(|a, b| a.host_num.cmp(&b.host_num))
                 .map_or(subnet.start_host, |i| i.host_num + 1);
             if addr_num > subnet.end_host || addr_num < subnet.start_host {
-                Err(cmd_error!("Got invalid lease address for instance"))?;
+                return Err("Got invalid lease address for instance".into());
             }
-            let addr = subnet.network.make_ip(addr_num as u32)?;
+            let addr = subnet
+                .network
+                .make_ip(addr_num as u32)
+                .to_api_with("Unable to generate instance IP")?;
             CidrV4::new(addr, subnet.network.cidr())
         };
 
@@ -85,7 +88,12 @@ pub async fn new_instance(
         };
 
         // generate cloud-init data
-        let db_inst = Instance::insert(&ctx, &args.name, &subnet, lease.clone(), None).await?;
+        let db_inst = {
+            let inst = Instance::insert(&ctx, &args.name, &subnet, lease.clone(), None)
+                .await
+                .to_api_type(ErrorType::Database)?;
+            Transaction::begin(&ctx, inst)
+        };
 
         progress!(prog_task, 30.0, "Creating instance images...");
         // create primary volume from base image
@@ -96,7 +104,7 @@ pub async fn new_instance(
                 datasize!((args.disk_sizes.0) GiB),
             )
             .await
-            .map_err(|er| cmd_error!("Failed to clone base image: {}", er))?;
+            .to_api_with("Failed to clone base image")?;
 
         // and, if it exists: the second volume
         let sec_vol = match args.disk_sizes.1 {
@@ -104,7 +112,11 @@ pub async fn new_instance(
                 let voldata =
                     // TODO: Fix VolType
                     xml::Volume::new(&args.name, xml::VolType::Qcow2, datasize!(sec_size GiB));
-                Some(vol::Volume::create(&ctx.virt.pools.secondary, voldata, 0).await?)
+                Some(
+                    vol::Volume::create(&ctx.virt.pools.secondary, voldata, 0)
+                        .await
+                        .to_api_with("Couldn't create secondary volume")?,
+                )
             }
             None => None,
         };
@@ -168,15 +180,20 @@ pub async fn new_instance(
             .build()
         };
 
-        let mut virt_dom = ctx.virt.conn.define_instance(dom_xml).await?;
+        let mut virt_dom = ctx
+            .virt
+            .conn
+            .define_instance(dom_xml)
+            .await
+            .to_api_with("Couldn't define libvirt instance")?;
 
         // not a fatal error, we can set autostart afterward
-        if let Err(er) = virt_dom.autostart(true).await {
+        if let Err(err) = virt_dom.autostart(true).await {
-            warn!("Couldn't set autostart for domain: {}", er);
+            warn!("Couldn't set autostart for domain: {err}");
         }
 
-        if let Err(er) = virt_dom.start().await {
+        if let Err(err) = virt_dom.start().await {
-            warn!("Domain defined, but couldn't be started! Error: {}", er);
+            warn!("Domain defined, but couldn't be started! Error: {err}");
         }
 
         // set all volumes to persistent to avoid deletion
@@ -188,16 +205,19 @@ pub async fn new_instance(
 
         progress!(prog_task, 80.0, "Domain created!");
         debug!("Domain {} created!", virt_dom.xml().await.name.as_str());
-        Ok((db_inst, virt_dom))
+        Ok((db_inst.take(), virt_dom))
     }
 }
 
 pub async fn delete_instance(
     ctx: Context,
     name: String,
-) -> Result<Option<model::Instance>, Box<dyn std::error::Error>> {
+) -> Result<Option<model::Instance>, ApiError> {
-    let Some(inst_db) = Instance::get_by_name(&ctx, &name).await? else {
+    let Some(inst_db) = Instance::get_by_name(&ctx, &name)
-        return Err(cmd_error!("Instance {name} not found"));
+        .await
+        .to_api_with_type(ErrorType::Database, "Couldn't find instance")?
+    else {
+        return Err(ErrorType::NotFound.into());
     };
     let api_model = match inst_db.api_model(&ctx).await {
         Ok(model) => Some(model),
@@ -209,16 +229,25 @@ pub async fn delete_instance(
     // First, destroy the instance
     match ctx.virt.conn.get_instance(name.clone()).await {
         Ok(mut inst) => {
-            inst.stop().await?;
+            inst.stop().await.to_api_with("Couldn't stop instance")?;
-            inst.undefine(true).await?;
+            inst.undefine(true)
+                .await
+                .to_api_with("Couldn't undefine instance")?;
         }
         Err(DomainError::DomainNotFound) => {
             warn!("Deleting instance that exists in DB but not libvirt");
         }
-        Err(err) => Err(err)?,
+        Err(err) => Err(ApiError::new(
+            nzr_api::error::ErrorType::VirtError,
+            "Couldn't get instance from libvirt",
+            err,
+        ))?,
     }
     // Then, delete the DB entity
-    inst_db.delete(&ctx).await?;
+    inst_db
+        .delete(&ctx)
+        .await
+        .to_api_with("Couldn't delete from database")?;
 
     Ok(api_model)
 }

nzrd/src/model/mod.rs

@@ -20,12 +20,14 @@ use tx::Transactable;
 
 #[derive(Debug, Error)]
 pub enum ModelError {
-    #[error("Database error occurred: {0}")]
+    #[error("{0}")]
     Db(#[from] diesel::result::Error),
-    #[error("Unable to get database handle: {0}")]
+    #[error("Database pool error ({0})")]
     Pool(#[from] diesel::r2d2::PoolError),
     #[error("{0}")]
     Cidr(#[from] cidr::Error),
+    #[error("Instance belongs to a subnet that has since disappeared")]
+    NoSubnet,
 }
 
 diesel::table! {
@@ -247,10 +249,7 @@ impl Instance {
 
     /// Creates an [nzr_api::model::Instance] from the information available in
     /// the database.
-    pub async fn api_model(
+    pub async fn api_model(&self, ctx: &Context) -> Result<nzr_api::model::Instance, ModelError> {
-        &self,
-        ctx: &Context,
-    ) -> Result<nzr_api::model::Instance, Box<dyn std::error::Error>> {
         let netid = self.subnet_id;
         let Some(subnet) = ctx
             .spawn_db(move |mut db| Subnet::table().find(netid).load::<Subnet>(&mut db))
@@ -258,7 +257,7 @@ impl Instance {
             .into_iter()
             .next()
         else {
-            todo!("something went horribly wrong");
+            return Err(ModelError::NoSubnet);
         };
 
         Ok(nzr_api::model::Instance {

nzrd/src/rpc.rs

@@ -1,4 +1,5 @@
 use futures::{future, StreamExt};
+use nzr_api::error::{ApiError, ErrorType, ToApiResult};
 use nzr_api::{args, model, nzr_event, InstanceQuery, Nazrin};
 use std::str::FromStr;
 use std::sync::Arc;
@@ -36,7 +37,7 @@ impl Nazrin for NzrServer {
         self,
         _: tarpc::context::Context,
         build_args: args::NewInstance,
-    ) -> Result<uuid::Uuid, String> {
+    ) -> Result<uuid::Uuid, ApiError> {
         let progress = Arc::new(RwLock::new(crate::ctrl::vm::Progress {
             status_text: "Starting...".to_owned(),
             percentage: 0.0,
@@ -44,13 +45,11 @@ impl Nazrin for NzrServer {
         let prog_task = progress.clone();
         let build_task = tokio::spawn(async move {
             let (inst, dom) =
-                cmd::vm::new_instance(self.ctx.clone(), prog_task.clone(), &build_args)
+                cmd::vm::new_instance(self.ctx.clone(), prog_task.clone(), &build_args).await?;
-                    .await
-                    .map_err(|e| format!("Instance creation failed: {}", e))?;
             let mut api_model = inst
                 .api_model(&self.ctx)
                 .await
-                .map_err(|e| format!("Couldn't generate API response: {e}"))?;
+                .to_api_with("Couldn't generate API response")?;
             match dom.state().await {
                 Ok(state) => {
                     api_model.state = state.into();
@@ -97,7 +96,7 @@ impl Nazrin for NzrServer {
             Some(
                 task.inner
                     .await
-                    .map_err(|err| format!("Task failed with panic: {}", err))
+                    .to_api_with("Task failed with panic")
                     .and_then(|res| res),
             )
         } else {
@@ -111,10 +110,12 @@ impl Nazrin for NzrServer {
         })
     }
 
-    async fn delete_instance(self, _: tarpc::context::Context, name: String) -> Result<(), String> {
+    async fn delete_instance(
-        let api_model = cmd::vm::delete_instance(self.ctx.clone(), name)
+        self,
-            .await
+        _: tarpc::context::Context,
-            .map_err(|e| format!("Couldn't delete instance: {}", e))?;
+        name: String,
+    ) -> Result<(), ApiError> {
+        let api_model = cmd::vm::delete_instance(self.ctx.clone(), name).await?;
 
         if let Some(api_model) = api_model {
             nzr_event!(self.ctx.events, Deleted, api_model);
@@ -126,18 +127,16 @@ impl Nazrin for NzrServer {
         self,
         _: tarpc::context::Context,
         query: nzr_api::InstanceQuery,
-    ) -> Result<Option<model::Instance>, String> {
+    ) -> Result<Option<model::Instance>, ApiError> {
         let res = match query {
             InstanceQuery::Name(name) => Instance::get_by_name(&self.ctx, name).await,
             InstanceQuery::MacAddr(addr) => Instance::get_by_mac(&self.ctx, addr).await,
             InstanceQuery::Ipv4Addr(addr) => Instance::get_by_ip4(&self.ctx, addr).await,
         }
-        .map_err(|e| e.to_string())?;
+        .to_api()?;
 
         if let Some(inst) = res {
-            inst.api_model(&self.ctx)
+            inst.api_model(&self.ctx).await.to_api().map(Some)
-                .await
-                .map_or_else(|e| Err(e.to_string()), |m| Ok(Some(m)))
         } else {
             Ok(None)
         }
@@ -147,10 +146,10 @@ impl Nazrin for NzrServer {
         self,
         _: tarpc::context::Context,
         with_status: bool,
-    ) -> Result<Vec<model::Instance>, String> {
+    ) -> Result<Vec<model::Instance>, ApiError> {
         let db_models = Instance::all(&self.ctx)
             .await
-            .map_err(|e| format!("Unable to get all instances: {e}"))?;
+            .to_api_type(ErrorType::Database)?;
         let mut models = Vec::new();
         for inst in db_models {
             let mut api_model = match inst.api_model(&self.ctx).await {
@@ -188,12 +187,12 @@ impl Nazrin for NzrServer {
         self,
         _: tarpc::context::Context,
         build_args: model::Subnet,
-    ) -> Result<model::Subnet, String> {
+    ) -> Result<model::Subnet, ApiError> {
-        let subnet = cmd::net::add_subnet(&self.ctx, build_args)
+        let subnet = Subnet::insert(&self.ctx, build_args.name, build_args.data)
             .await
-            .map_err(|e| e.to_string())?
+            .to_api_type(ErrorType::Database)?
             .api_model()
-            .map_err(|e| e.to_string())?;
+            .to_api_with("Unable to generate API model")?;
 
         // inform event listeners
         nzr_event!(self.ctx.events, Created, subnet);
@@ -204,22 +203,23 @@ impl Nazrin for NzrServer {
         self,
         _: tarpc::context::Context,
         edit_args: model::Subnet,
-    ) -> Result<model::Subnet, String> {
+    ) -> Result<model::Subnet, ApiError> {
         if let Some(subnet) = Subnet::get_by_name(&self.ctx, &edit_args.name)
             .await
             .map_err(|e| e.to_string())?
         {
             Err("Modifying subnets not yet supported".into())
         } else {
-            Err(format!("Subnet {} not found", &edit_args.name))
+            Err(ErrorType::NotFound.into())
         }
     }
 
-    async fn get_subnets(self, _: tarpc::context::Context) -> Result<Vec<model::Subnet>, String> {
+    async fn get_subnets(self, _: tarpc::context::Context) -> Result<Vec<model::Subnet>, ApiError> {
-        Subnet::all(&self.ctx).await.map_or_else(
+        Subnet::all(&self.ctx)
-            |e| Err(e.to_string()),
+            .await
-            |v| {
+            .to_api_with("Couldn't get list of subnets")
-                Ok(v.into_iter()
+            .map(|v| {
+                v.into_iter()
                     .filter_map(|s| match s.api_model() {
                         Ok(model) => Some(model),
                         Err(err) => {
@@ -227,23 +227,43 @@ impl Nazrin for NzrServer {
                             None
                         }
                     })
-                    .collect())
+                    .collect()
-            },
+            })
-        )
     }
 
     async fn delete_subnet(
         self,
         _: tarpc::context::Context,
         subnet_name: String,
-    ) -> Result<(), String> {
+    ) -> Result<(), ApiError> {
-        cmd::net::delete_subnet(&self.ctx, &subnet_name)
+        if let Some(subnet) = Subnet::get_by_name(&self.ctx, subnet_name)
             .await
-            .map_err(|e| e.to_string())?;
+            .to_api_type(ErrorType::Database)?
-        Ok(())
+        {
+            let api_model = match subnet.api_model() {
+                Ok(model) => Some(model),
+                Err(err) => {
+                    tracing::error!("Unable to generate model for clients: {err}");
+                    None
+                }
+            };
+
+            subnet
+                .delete(&self.ctx)
+                .await
+                .to_api_type(ErrorType::Database)?;
+
+            if let Some(api_model) = api_model {
+                nzr_event!(&self.ctx.events, Deleted, api_model);
+            }
+
+            Ok(())
+        } else {
+            Err(ErrorType::NotFound.into())
+        }
     }
 
-    async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), String> {
+    async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), ApiError> {
         cmd::vm::prune_instances(&self.ctx)
             .await
             .map_err(|e| e.to_string())?;
@@ -254,50 +274,52 @@ impl Nazrin for NzrServer {
         self,
         _: tarpc::context::Context,
         id: i32,
-    ) -> Result<Vec<u8>, String> {
+    ) -> Result<Vec<u8>, ApiError> {
-        let Some(db_model) = Instance::get(&self.ctx, id)
+        if let Some(db_model) = Instance::get(&self.ctx, id)
             .await
-            .map_err(|e| e.to_string())?
+            .to_api_type(ErrorType::Database)?
-        else {
+        {
-            return Err("Instance doesn't exist".to_owned());
+            Ok(db_model.ci_userdata.unwrap_or_default())
-        };
+        } else {
-
+            Err(ErrorType::NotFound.into())
-        Ok(db_model.ci_userdata.unwrap_or_default())
+        }
     }
 
     async fn get_ssh_pubkeys(
         self,
         _: tarpc::context::Context,
-    ) -> Result<Vec<model::SshPubkey>, String> {
+    ) -> Result<Vec<model::SshPubkey>, ApiError> {
-        SshPubkey::all(&self.ctx).await.map_or_else(
+        SshPubkey::all(&self.ctx)
-            |e| Err(e.to_string()),
+            .await
-            |k| Ok(k.iter().map(|k| k.api_model()).collect()),
+            .to_api_type(ErrorType::Database)
-        )
+            .map(|k| k.iter().map(|k| k.api_model()).collect())
     }
 
     async fn add_ssh_pubkey(
         self,
         _: tarpc::context::Context,
         pub_key: String,
-    ) -> Result<model::SshPubkey, String> {
+    ) -> Result<model::SshPubkey, ApiError> {
-        let pubkey = model::SshPubkey::from_str(&pub_key).map_err(|e| e.to_string())?;
+        let pubkey = model::SshPubkey::from_str(&pub_key).to_api_type(ErrorType::Parse)?;
 
         SshPubkey::insert(&self.ctx, pubkey.algorithm, pubkey.key_data, pubkey.comment)
             .await
-            .map_err(|e| e.to_string())
+            .to_api_type(ErrorType::Database)
             .map(|k| k.api_model())
     }
 
-    async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), String> {
+    async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), ApiError> {
-        let Some(key) = SshPubkey::get(&self.ctx, id)
+        if let Some(key) = SshPubkey::get(&self.ctx, id)
             .await
-            .map_err(|e| e.to_string())?
+            .to_api_type(ErrorType::Database)?
-        else {
+        {
-            return Err("SSH key with ID doesn't exist".into());
+            key.delete(&self.ctx)
-        };
+                .await
-
+                .to_api_type(ErrorType::Database)?;
-        key.delete(&self.ctx).await.map_err(|e| e.to_string())?;
+            Ok(())
-        Ok(())
+        } else {
+            Err(ErrorType::NotFound.into())
+        }
     }
 }
 
@@ -335,7 +357,6 @@ pub async fn serve(ctx: Context) -> Result<(), Box<dyn std::error::Error>> {
         debug!("Listening for new connection...");
         let (conn, _addr) = listener.accept().await?;
         let ctx = ctx.clone();
-        // hack?
         tokio::spawn(async move {
             let framed = codec_builder.new_framed(conn);
             let transport = tarpc::serde_transport::new(framed, Bincode::default());
@@ -351,6 +372,6 @@ pub async fn serve(ctx: Context) -> Result<(), Box<dyn std::error::Error>> {
 }
 
 struct InstCreateStatus {
-    inner: JoinHandle<Result<model::Instance, String>>,
+    inner: JoinHandle<Result<model::Instance, ApiError>>,
     progress: Arc<RwLock<crate::ctrl::vm::Progress>>,
 }

omyacid/src/ctx.rs

@@ -57,7 +57,7 @@ impl Context {
     }
 
     pub async fn get_sshkeys(&self) -> Result<Vec<SshPubkey>> {
-        // TODO: do we cache SSH keys? I don't like the idea of it
+        // We don't cache SSH keys, so always get from the API server
         let ssh_keys = self
             .api_client
             .get_ssh_pubkeys(nzr_api::default_ctx())

omyacid/src/main.rs

@@ -41,7 +41,6 @@ async fn get_meta_data(
             Ok(Some(inst)) => {
                 let meta = Metadata {
                     inst_name: &inst.name,
-                    // XXX: this is very silly imo
                     ssh_pubkeys: ssh_pubkeys.iter().collect(),
                 };