From 43b36429559275af7093c97dfbd948b612e4da28 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Tue, 1 Oct 2019 16:41:50 +0200 Subject: [PATCH] vmm: Clean Error handling up We used to have errors definitions spread across vmm, vm, api, and http. We now have a cleaner separation: All API routines only return an ApiResult. All VM operations, including the VMM wrappers, return a VmResult. This makes it easier to carry errors up to the HTTP caller. Signed-off-by: Samuel Ortiz --- vmm/src/api/http_endpoint.rs | 27 +++++++------ vmm/src/api/mod.rs | 76 ++++++++++++++++++------------------ vmm/src/lib.rs | 66 ++++++++----------------------- vmm/src/vm.rs | 6 +++ 4 files changed, 73 insertions(+), 102 deletions(-) diff --git a/vmm/src/api/http_endpoint.rs b/vmm/src/api/http_endpoint.rs index 4e5de1007..e5cef19d6 100644 --- a/vmm/src/api/http_endpoint.rs +++ b/vmm/src/api/http_endpoint.rs @@ -4,9 +4,10 @@ // use crate::api::http::EndpointHandler; -use crate::api::VmConfig; -use crate::api::{vm_boot, vm_create, vm_info, vm_reboot, vm_shutdown, ApiRequest, VmAction}; -use crate::{Error, Result}; +use crate::api::{ + vm_boot, vm_create, vm_info, vm_reboot, vm_shutdown, ApiError, ApiRequest, ApiResult, VmAction, + VmConfig, +}; use micro_http::{Body, Method, Request, Response, StatusCode, Version}; use serde_json::Error as SerdeError; use std::sync::mpsc::Sender; @@ -20,22 +21,22 @@ pub enum HttpError { SerdeJsonDeserialize(SerdeError), /// Could not create a VM - VmCreate(Error), + VmCreate(ApiError), /// Could not boot a VM - VmBoot(Error), + VmBoot(ApiError), /// Could not get the VM information - VmInfo(Error), + VmInfo(ApiError), /// Could not shut a VM down - VmShutdown(Error), + VmShutdown(ApiError), /// Could not reboot a VM - VmReboot(Error), + VmReboot(ApiError), /// Could not act on a VM - VmAction(Error), + VmAction(ApiError), } fn error_response(error: HttpError, status: StatusCode) -> Response { @@ -90,7 +91,7 @@ pub struct VmActionHandler { action_fn: VmActionFn, } -type VmActionFn = Box) -> Result<()> + Send + Sync>; +type VmActionFn = Box) -> ApiResult<()> + Send + Sync>; impl VmActionHandler { pub fn new(action: VmAction) -> Self { @@ -114,9 +115,9 @@ impl EndpointHandler for VmActionHandler { match req.method() { Method::Put => { match (self.action_fn)(api_notifier, api_sender).map_err(|e| match e { - Error::ApiVmBoot(_) => HttpError::VmBoot(e), - Error::ApiVmShutdown(_) => HttpError::VmShutdown(e), - Error::ApiVmReboot(_) => HttpError::VmReboot(e), + ApiError::VmBoot(_) => HttpError::VmBoot(e), + ApiError::VmShutdown(_) => HttpError::VmShutdown(e), + ApiError::VmReboot(_) => HttpError::VmReboot(e), _ => HttpError::VmAction(e), }) { Ok(_) => Response::new(Version::Http11, StatusCode::OK), diff --git a/vmm/src/api/mod.rs b/vmm/src/api/mod.rs index 3d9d1962d..e64feefc5 100644 --- a/vmm/src/api/mod.rs +++ b/vmm/src/api/mod.rs @@ -38,16 +38,25 @@ pub mod http_endpoint; use crate::config::VmConfig; use crate::vm::{Error as VmError, VmState}; -use crate::{Error, Result}; -use std::sync::mpsc::{channel, Sender}; +use std::io; +use std::sync::mpsc::{channel, RecvError, SendError, Sender}; use std::sync::Arc; use vmm_sys_util::eventfd::EventFd; /// API errors are sent back from the VMM API server through the ApiResponse. #[derive(Debug)] pub enum ApiError { - /// The VM could not be created. - VmCreate(VmError), + /// Cannot write to EventFd. + EventFdWrite(io::Error), + + /// API request send error + RequestSend(SendError), + + /// Wrong reponse payload type + ResponsePayloadType, + + /// API response receive error + ResponseRecv(RecvError), /// The VM could not boot. VmBoot(VmError), @@ -55,8 +64,11 @@ pub enum ApiError { /// The VM is already created. VmAlreadyCreated, + /// The VM could not be created. + VmCreate(VmError), + /// The VM info is not available. - VmInfo, + VmInfo(VmError), /// The VM config is missing. VmMissingConfig, @@ -71,8 +83,9 @@ pub enum ApiError { VmShutdown(VmError), /// The VM could not reboot. - VmReboot, + VmReboot(VmError), } +pub type ApiResult = std::result::Result; #[derive(Clone, Deserialize, Serialize)] pub struct VmInfo { @@ -122,19 +135,16 @@ pub fn vm_create( api_evt: EventFd, api_sender: Sender, config: Arc, -) -> Result<()> { +) -> ApiResult<()> { let (response_sender, response_receiver) = channel(); // Send the VM creation request. api_sender .send(ApiRequest::VmCreate(config, response_sender)) - .map_err(Error::ApiRequestSend)?; - api_evt.write(1).map_err(Error::EventFdWrite)?; + .map_err(ApiError::RequestSend)?; + api_evt.write(1).map_err(ApiError::EventFdWrite)?; - response_receiver - .recv() - .map_err(Error::ApiResponseRecv)? - .map_err(Error::ApiVmCreate)?; + response_receiver.recv().map_err(ApiError::ResponseRecv)??; Ok(()) } @@ -153,7 +163,7 @@ pub enum VmAction { Reboot, } -fn vm_action(api_evt: EventFd, api_sender: Sender, action: VmAction) -> Result<()> { +fn vm_action(api_evt: EventFd, api_sender: Sender, action: VmAction) -> ApiResult<()> { let (response_sender, response_receiver) = channel(); let request = match action { @@ -163,63 +173,51 @@ fn vm_action(api_evt: EventFd, api_sender: Sender, action: VmAction) }; // Send the VM request. - api_sender.send(request).map_err(Error::ApiRequestSend)?; - api_evt.write(1).map_err(Error::EventFdWrite)?; + api_sender.send(request).map_err(ApiError::RequestSend)?; + api_evt.write(1).map_err(ApiError::EventFdWrite)?; match action { VmAction::Boot => { - response_receiver - .recv() - .map_err(Error::ApiResponseRecv)? - .map_err(Error::ApiVmBoot)?; + response_receiver.recv().map_err(ApiError::ResponseRecv)??; } VmAction::Shutdown => { - response_receiver - .recv() - .map_err(Error::ApiResponseRecv)? - .map_err(Error::ApiVmShutdown)?; + response_receiver.recv().map_err(ApiError::ResponseRecv)??; } VmAction::Reboot => { - response_receiver - .recv() - .map_err(Error::ApiResponseRecv)? - .map_err(Error::ApiVmReboot)?; + response_receiver.recv().map_err(ApiError::ResponseRecv)??; } } Ok(()) } -pub fn vm_boot(api_evt: EventFd, api_sender: Sender) -> Result<()> { +pub fn vm_boot(api_evt: EventFd, api_sender: Sender) -> ApiResult<()> { vm_action(api_evt, api_sender, VmAction::Boot) } -pub fn vm_shutdown(api_evt: EventFd, api_sender: Sender) -> Result<()> { +pub fn vm_shutdown(api_evt: EventFd, api_sender: Sender) -> ApiResult<()> { vm_action(api_evt, api_sender, VmAction::Shutdown) } -pub fn vm_reboot(api_evt: EventFd, api_sender: Sender) -> Result<()> { +pub fn vm_reboot(api_evt: EventFd, api_sender: Sender) -> ApiResult<()> { vm_action(api_evt, api_sender, VmAction::Reboot) } -pub fn vm_info(api_evt: EventFd, api_sender: Sender) -> Result { +pub fn vm_info(api_evt: EventFd, api_sender: Sender) -> ApiResult { let (response_sender, response_receiver) = channel(); // Send the VM request. api_sender .send(ApiRequest::VmInfo(response_sender)) - .map_err(Error::ApiRequestSend)?; - api_evt.write(1).map_err(Error::EventFdWrite)?; + .map_err(ApiError::RequestSend)?; + api_evt.write(1).map_err(ApiError::EventFdWrite)?; - let vm_info = response_receiver - .recv() - .map_err(Error::ApiResponseRecv)? - .map_err(|_| Error::ApiVmInfo)?; + let vm_info = response_receiver.recv().map_err(ApiError::ResponseRecv)??; match vm_info { ApiResponsePayload::VmInfo(info) => Ok(info), - _ => Err(Error::ApiVmInfo), + _ => Err(ApiError::ResponsePayloadType), } } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 1c4bea8f5..4d0b7810c 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -37,30 +37,9 @@ pub enum Error { /// API request receive error ApiRequestRecv(RecvError), - /// API response receive error - ApiResponseRecv(RecvError), - - /// API request send error - ApiRequestSend(SendError), - /// API response send error ApiResponseSend(SendError), - /// Cannot create a VM from the API - ApiVmCreate(ApiError), - - /// Cannot boot a VM from the API - ApiVmBoot(ApiError), - - /// Cannot get the VM info - ApiVmInfo, - - /// Cannot shut a VM down from the API - ApiVmShutdown(ApiError), - - /// Cannot reboot a VM from the API - ApiVmReboot(ApiError), - /// Cannot bind to the UNIX domain socket path Bind(io::Error), @@ -73,9 +52,6 @@ pub enum Error { /// Cannot read from EventFd. EventFdRead(io::Error), - /// Cannot write to EventFd. - EventFdWrite(io::Error), - /// Cannot create epoll context. Epoll(io::Error), @@ -85,17 +61,8 @@ pub enum Error { /// Cannot handle the VM STDIN stream Stdin(VmError), - /// Cannot create a VM - VmCreate(VmError), - - /// Cannot boot a VM - VmBoot(VmError), - - /// Cannot fetch the VM information - VmInfo, - - /// The Vm is not created - VmNotCreated, + /// Cannot reboot the VM + VmReboot(VmError), /// Cannot shut a VM down VmShutdown(VmError), @@ -196,7 +163,7 @@ pub fn start_vmm_thread( // based on the same VM config, boot it and restart // the control loop. - vmm.vm_reboot()?; + vmm.vm_reboot().map_err(Error::VmReboot)?; // Continue and restart the VMM control loop continue 'outer; @@ -264,12 +231,12 @@ impl Vmm { }) } - fn vm_reboot(&mut self) -> Result<()> { + fn vm_reboot(&mut self) -> result::Result<(), VmError> { // Without ACPI, a reset is equivalent to a shutdown #[cfg(not(feature = "acpi"))] { if let Some(ref mut vm) = self.vm { - vm.shutdown().map_err(Error::VmShutdown)?; + vm.shutdown()?; return Ok(()); } } @@ -277,29 +244,29 @@ impl Vmm { // First we stop the current VM and create a new one. if let Some(ref mut vm) = self.vm { let config = vm.get_config(); - vm.shutdown().map_err(Error::VmShutdown)?; + vm.shutdown()?; - let exit_evt = self.exit_evt.try_clone().map_err(Error::EventFdClone)?; - let reset_evt = self.reset_evt.try_clone().map_err(Error::EventFdClone)?; + let exit_evt = self.exit_evt.try_clone().map_err(VmError::EventFdClone)?; + let reset_evt = self.reset_evt.try_clone().map_err(VmError::EventFdClone)?; - self.vm = Some(Vm::new(config, exit_evt, reset_evt).map_err(Error::VmCreate)?); + self.vm = Some(Vm::new(config, exit_evt, reset_evt)?); } // Then we start the new VM. if let Some(ref mut vm) = self.vm { - vm.boot().map_err(Error::VmBoot)?; + vm.boot()?; } else { - return Err(Error::VmNotCreated); + return Err(VmError::VmNotCreated); } Ok(()) } - fn vm_info(&self) -> Result { + fn vm_info(&self) -> result::Result { match &self.vm_config { Some(config) => { let state = match &self.vm { - Some(vm) => vm.get_state().unwrap(), + Some(vm) => vm.get_state()?, None => VmState::Created, }; @@ -308,7 +275,7 @@ impl Vmm { state, }) } - None => Err(Error::VmNotCreated), + None => Err(VmError::VmNotCreated), } } @@ -448,8 +415,7 @@ impl Vmm { ApiRequest::VmReboot(sender) => { let response = match self.vm_reboot() { Ok(_) => Ok(ApiResponsePayload::Empty), - Err(Error::VmNotCreated) => Err(ApiError::VmNotBooted), - Err(_) => Err(ApiError::VmReboot), + Err(e) => Err(ApiError::VmReboot(e)), }; sender.send(response).map_err(Error::ApiResponseSend)?; @@ -457,7 +423,7 @@ impl Vmm { ApiRequest::VmInfo(sender) => { let response = match self.vm_info() { Ok(info) => Ok(ApiResponsePayload::VmInfo(info)), - Err(_) => Err(ApiError::VmInfo), + Err(e) => Err(ApiError::VmInfo(e)), }; sender.send(response).map_err(Error::ApiResponseSend)?; diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index 49dd5f4c5..41854d50f 100755 --- a/vmm/src/vm.rs +++ b/vmm/src/vm.rs @@ -209,6 +209,12 @@ pub enum Error { /// Failed to create a new KVM instance KvmNew(io::Error), + + /// VM is not created + VmNotCreated, + + /// Cannot clone EventFd. + EventFdClone(io::Error), } pub type Result = result::Result;