From a9de87a1ced5049b2bcea448e2e444e94fa47136 Mon Sep 17 00:00:00 2001 From: Erik Schilling Date: Fri, 7 Jul 2023 08:44:55 +0200 Subject: [PATCH] tree-wide: bubble up errors from daemon threads There was a mix of just unwrapping (panicking) and catching and logging errors. The unwrapping is not allowing for particulary pretty error handling, so let's bubble the errors up by not crashing the thread, but by just returning a Result<()> than is received when joining the threads. Not all .unwrap() uses were translated since a followup PR (#389) will rework that code anyway (and get rid of the .unwrap() in the process). Signed-off-by: Erik Schilling --- coverage_config_x86_64.json | 2 +- crates/gpio/src/backend.rs | 25 ++++++++++++++++++------- crates/i2c/src/main.rs | 14 +++++++++----- crates/rng/src/main.rs | 15 ++++++++++----- crates/vsock/src/main.rs | 35 +++++++++++++++++++++++++++-------- 5 files changed, 65 insertions(+), 26 deletions(-) diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index d528940..1773554 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 69.0, + "coverage_score": 68.2, "exclude_path": "", "crate_features": "" } diff --git a/crates/gpio/src/backend.rs b/crates/gpio/src/backend.rs index 0d93af8..28408eb 100644 --- a/crates/gpio/src/backend.rs +++ b/crates/gpio/src/backend.rs @@ -9,7 +9,7 @@ use log::{error, info, warn}; use std::num::ParseIntError; use std::process::exit; use std::sync::{Arc, RwLock}; -use std::thread::spawn; +use std::thread::{spawn, JoinHandle}; use clap::Parser; use thiserror::Error as ThisError; @@ -35,6 +35,14 @@ pub(crate) enum Error { ParseFailure(ParseIntError), #[error("Failed to join threads")] FailedJoiningThreads, + #[error("Could not open gpio device: {0}")] + CouldNotOpenDevice(crate::gpio::Error), + #[error("Could not create gpio controller: {0}")] + CouldNotCreateGpioController(crate::gpio::Error), + #[error("Could not create gpio backend: {0}")] + CouldNotCreateBackend(crate::vhu_gpio::Error), + #[error("Could not create daemon: {0}")] + CouldNotCreateDaemon(vhost_user_backend::Error), } #[derive(Parser, Debug)] @@ -134,7 +142,7 @@ fn start_backend(args: GpioArgs) -> Resul let socket = config.socket_path.to_owned() + &i.to_string(); let device_num = config.devices.inner[i]; - let handle = spawn(move || loop { + let handle: JoinHandle> = spawn(move || loop { // A separate thread is spawned for each socket and can connect to a separate guest. // These are run in an infinite loop to not require the daemon to be restarted once a // guest exits. @@ -143,9 +151,12 @@ fn start_backend(args: GpioArgs) -> Resul // threads, and so the code uses unwrap() instead. The panic on a thread won't cause // trouble to other threads/guests or the main() function and should be safe for the // daemon. - let device = D::open(device_num).unwrap(); - let controller = GpioController::::new(device).unwrap(); - let backend = Arc::new(RwLock::new(VhostUserGpioBackend::new(controller).unwrap())); + let device = D::open(device_num).map_err(Error::CouldNotOpenDevice)?; + let controller = + GpioController::::new(device).map_err(Error::CouldNotCreateGpioController)?; + let backend = Arc::new(RwLock::new( + VhostUserGpioBackend::new(controller).map_err(Error::CouldNotCreateBackend)?, + )); let listener = Listener::new(socket.clone(), true).unwrap(); let mut daemon = VhostUserDaemon::new( @@ -153,7 +164,7 @@ fn start_backend(args: GpioArgs) -> Resul backend.clone(), GuestMemoryAtomic::new(GuestMemoryMmap::new()), ) - .unwrap(); + .map_err(Error::CouldNotCreateDaemon)?; daemon.start(listener).unwrap(); @@ -179,7 +190,7 @@ fn start_backend(args: GpioArgs) -> Resul } for handle in handles { - handle.join().map_err(|_| Error::FailedJoiningThreads)?; + handle.join().map_err(|_| Error::FailedJoiningThreads)??; } Ok(()) diff --git a/crates/i2c/src/main.rs b/crates/i2c/src/main.rs index 0fa537c..4698063 100644 --- a/crates/i2c/src/main.rs +++ b/crates/i2c/src/main.rs @@ -12,7 +12,7 @@ use log::{error, info, warn}; use std::num::ParseIntError; use std::process::exit; use std::sync::{Arc, RwLock}; -use std::thread::spawn; +use std::thread::{spawn, JoinHandle}; use clap::Parser; use thiserror::Error as ThisError; @@ -42,6 +42,10 @@ pub(crate) enum Error { ParseFailure(ParseIntError), #[error("Failed to join threads")] FailedJoiningThreads, + #[error("Could not create backend: {0}")] + CouldNotCreateBackend(vhu_i2c::Error), + #[error("Could not create daemon: {0}")] + CouldNotCreateDaemon(vhost_user_backend::Error), } #[derive(Parser, Debug)] @@ -183,7 +187,7 @@ fn start_backend(args: I2cArgs) -> Result< let socket = config.socket_path.to_owned() + &i.to_string(); let i2c_map = i2c_map.clone(); - let handle = spawn(move || loop { + let handle: JoinHandle> = spawn(move || loop { // A separate thread is spawned for each socket and can connect to a separate guest. // These are run in an infinite loop to not require the daemon to be restarted once a // guest exits. @@ -193,7 +197,7 @@ fn start_backend(args: I2cArgs) -> Result< // trouble to other threads/guests or the main() function and should be safe for the // daemon. let backend = Arc::new(RwLock::new( - VhostUserI2cBackend::new(i2c_map.clone()).unwrap(), + VhostUserI2cBackend::new(i2c_map.clone()).map_err(Error::CouldNotCreateBackend)?, )); let listener = Listener::new(socket.clone(), true).unwrap(); @@ -202,7 +206,7 @@ fn start_backend(args: I2cArgs) -> Result< backend.clone(), GuestMemoryAtomic::new(GuestMemoryMmap::new()), ) - .unwrap(); + .map_err(Error::CouldNotCreateDaemon)?; daemon.start(listener).unwrap(); @@ -228,7 +232,7 @@ fn start_backend(args: I2cArgs) -> Result< } for handle in handles { - handle.join().map_err(|_| Error::FailedJoiningThreads)?; + handle.join().map_err(|_| Error::FailedJoiningThreads)??; } Ok(()) diff --git a/crates/rng/src/main.rs b/crates/rng/src/main.rs index f1c97fb..dc9d666 100644 --- a/crates/rng/src/main.rs +++ b/crates/rng/src/main.rs @@ -9,7 +9,7 @@ use log::{error, info, warn}; use std::fs::File; use std::process::exit; use std::sync::{Arc, Mutex, RwLock}; -use std::thread; +use std::thread::{self, JoinHandle}; use clap::Parser; use thiserror::Error as ThisError; @@ -36,6 +36,10 @@ pub(crate) enum Error { InvalidSocketCount(u32), #[error("Threads can't be joined")] FailedJoiningThreads, + #[error("Could not create backend: {0}")] + CouldNotCreateBackend(std::io::Error), + #[error("Could not create daemon: {0}")] + CouldNotCreateDaemon(vhost_user_backend::Error), } #[derive(Clone, Parser, Debug, PartialEq)] @@ -110,13 +114,14 @@ pub(crate) fn start_backend(config: VuRngConfig) -> Result<()> { let max_bytes = config.max_bytes; let random = Arc::clone(&random_file); - let handle = thread::spawn(move || loop { + let handle: JoinHandle> = thread::spawn(move || loop { // If creating the VuRngBackend isn't successull there isn't much else to do than // killing the thread, which .unwrap() does. When that happens an error code is // generated and displayed by the runtime mechanic. Killing a thread doesn't affect // the other threads spun-off by the daemon. let vu_rng_backend = Arc::new(RwLock::new( - VuRngBackend::new(random.clone(), period_ms, max_bytes).unwrap(), + VuRngBackend::new(random.clone(), period_ms, max_bytes) + .map_err(Error::CouldNotCreateBackend)?, )); let mut daemon = VhostUserDaemon::new( @@ -124,7 +129,7 @@ pub(crate) fn start_backend(config: VuRngConfig) -> Result<()> { Arc::clone(&vu_rng_backend), GuestMemoryAtomic::new(GuestMemoryMmap::new()), ) - .unwrap(); + .map_err(Error::CouldNotCreateDaemon)?; let listener = Listener::new(socket.clone(), true).unwrap(); daemon.start(listener).unwrap(); @@ -156,7 +161,7 @@ pub(crate) fn start_backend(config: VuRngConfig) -> Result<()> { } for handle in handles { - handle.join().map_err(|_| Error::FailedJoiningThreads)?; + handle.join().map_err(|_| Error::FailedJoiningThreads)??; } Ok(()) diff --git a/crates/vsock/src/main.rs b/crates/vsock/src/main.rs index 98d8751..c1156b6 100644 --- a/crates/vsock/src/main.rs +++ b/crates/vsock/src/main.rs @@ -11,13 +11,14 @@ mod vsock_conn; use std::{ collections::HashMap, convert::TryFrom, + process::exit, sync::{Arc, RwLock}, thread, }; use crate::vhu_vsock::{CidMap, VhostUserVsockBackend, VsockConfig}; use clap::{Args, Parser}; -use log::{info, warn}; +use log::{error, info, warn}; use serde::Deserialize; use thiserror::Error as ThisError; use vhost::{vhost_user, vhost_user::Listener}; @@ -47,6 +48,14 @@ enum VmArgsParseError { RequiredKeyNotFound(String), } +#[derive(Debug, ThisError)] +enum BackendError { + #[error("Could not create backend: {0}")] + CouldNotCreateBackend(vhu_vsock::Error), + #[error("Could not create daemon: {0}")] + CouldNotCreateDaemon(vhost_user_backend::Error), +} + #[derive(Args, Clone, Debug, Deserialize)] struct VsockParam { /// Context identifier of the guest which uniquely identifies the device for its lifetime. @@ -177,10 +186,15 @@ impl TryFrom for Vec { /// This is the public API through which an external program starts the /// vhost-user-vsock backend server. -pub(crate) fn start_backend_server(config: VsockConfig, cid_map: Arc>) { +pub(crate) fn start_backend_server( + config: VsockConfig, + cid_map: Arc>, +) -> Result<(), BackendError> { loop { - let backend = - Arc::new(VhostUserVsockBackend::new(config.clone(), cid_map.clone()).unwrap()); + let backend = Arc::new( + VhostUserVsockBackend::new(config.clone(), cid_map.clone()) + .map_err(BackendError::CouldNotCreateBackend)?, + ); cid_map .write() .unwrap() @@ -193,7 +207,7 @@ pub(crate) fn start_backend_server(config: VsockConfig, cid_map: Arc Result<(), BackendError> { let cid_map: Arc> = Arc::new(RwLock::new(HashMap::new())); let mut handles = Vec::new(); @@ -239,8 +253,10 @@ pub(crate) fn start_backend_servers(configs: &[VsockConfig]) { } for handle in handles { - handle.join().unwrap(); + handle.join().unwrap()?; } + + Ok(()) } fn main() { @@ -254,7 +270,10 @@ fn main() { } }; - start_backend_servers(&configs); + if let Err(e) = start_backend_servers(&configs) { + error!("{e}"); + exit(1); + } } #[cfg(test)]