api_server: fix error propagating

The old IOError and EventFdError in the api_server's
enum Error are not actually used.
They have been at some point used but we later introduced
the logci where bind_and_run would call std::proces::exit on
errors on HttpServer creation so in reality the IOError and
EventFD error would not be thrown ever.

There are still some things to figure out: bind_and_run will
not return any type of error except for the one we add in this
commit. All the error processing happens inside bind and run
 in reality.

Signed-off-by: Diana Popa <dpopa@amazon.com>

Author: dianpopa

src/api_server/src/lib.rs

@@ -14,7 +14,6 @@ mod request;
 
 use std::path::PathBuf;
 use std::sync::mpsc;
-use std::{fmt, io};
 
 use logger::{
     debug, error, info, update_metric_with_elapsed_time, warn, ProcessTimeReporter, METRICS,
@@ -30,30 +29,18 @@ use vmm::rpc_interface::{VmmAction, VmmActionError, VmmData};
 use vmm::vmm_config::snapshot::SnapshotType;
 
 use crate::parsed_request::{ParsedRequest, RequestAction};
+use crate::Error::ServerCreation;
 
 /// Shorthand type for a request containing a boxed VmmAction.
 pub type ApiRequest = Box<VmmAction>;
 /// Shorthand type for a response containing a boxed Result.
 pub type ApiResponse = Box<std::result::Result<VmmData, VmmActionError>>;
 
 /// Errors thrown when binding the API server to the socket path.
-#[derive(thiserror::Error)]
+#[derive(Debug)]
 pub enum Error {
-    /// IO related error.
-    #[error("IO error: {0}")]
-    Io(io::Error),
-    /// EventFD related error.
-    #[error("EventFd error: {0}")]
-    Eventfd(io::Error),
-}
-
-impl fmt::Debug for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            Error::Io(ref err) => write!(f, "IO error: {}", err),
-            Error::Eventfd(ref err) => write!(f, "EventFd error: {}", err),
-        }
-    }
+    /// HTTP Server creation related error.
+    ServerCreation(ServerError),
 }
 
 type Result<T> = std::result::Result<T, Error>;
@@ -137,7 +124,7 @@ impl ApiServer {
     ///     .spawn(move || {
     ///         ApiServer::new(api_request_sender, vmm_response_receiver, to_vmm_fd)
     ///             .bind_and_run(
-    ///                 PathBuf::from(api_thread_path_to_socket),
+    ///                 &PathBuf::from(api_thread_path_to_socket),
     ///                 time_reporter,
     ///                 seccomp_filters.get("api").unwrap(),
     ///                 payload_limit,
@@ -161,16 +148,14 @@ impl ApiServer {
     /// ```
     pub fn bind_and_run(
         &mut self,
-        path: PathBuf,
+        path: &PathBuf,
         process_time_reporter: ProcessTimeReporter,
         seccomp_filter: BpfProgramRef,
         api_payload_limit: usize,
         socket_ready: mpsc::Sender<bool>,
     ) -> Result<()> {
-        let mut server = HttpServer::new(path).unwrap_or_else(|err| {
-            error!("Error creating the HTTP server: {}", err);
-            std::process::exit(vmm::FcExitCode::GenericError as i32);
-        });
+        let mut server = HttpServer::new(path).map_err(ServerCreation)?;
+
         // Announce main thread that the socket path was created.
         // As per the doc, "A send operation can only fail if the receiving end of a channel is
         // disconnected". so this means that the main thread has exited.
@@ -338,34 +323,6 @@ mod tests {
 
     use super::*;
 
-    #[test]
-    fn test_error_messages() {
-        let err = Error::Io(io::Error::from_raw_os_error(0));
-        assert_eq!(
-            format!("{}", err),
-            format!("IO error: {}", io::Error::from_raw_os_error(0))
-        );
-        let err = Error::Eventfd(io::Error::from_raw_os_error(0));
-        assert_eq!(
-            format!("{}", err),
-            format!("EventFd error: {}", io::Error::from_raw_os_error(0))
-        );
-    }
-
-    #[test]
-    fn test_error_debug() {
-        let err = Error::Io(io::Error::from_raw_os_error(0));
-        assert_eq!(
-            format!("{:?}", err),
-            format!("IO error: {}", io::Error::from_raw_os_error(0))
-        );
-        let err = Error::Eventfd(io::Error::from_raw_os_error(0));
-        assert_eq!(
-            format!("{:?}", err),
-            format!("EventFd error: {}", io::Error::from_raw_os_error(0))
-        );
-    }
-
     #[test]
     fn test_serve_vmm_action_request() {
         let to_vmm_fd = EventFd::new(libc::EFD_NONBLOCK).unwrap();
@@ -491,7 +448,7 @@ mod tests {
             .spawn(move || {
                 ApiServer::new(api_request_sender, vmm_response_receiver, to_vmm_fd)
                     .bind_and_run(
-                        PathBuf::from(api_thread_path_to_socket),
+                        &PathBuf::from(api_thread_path_to_socket),
                         ProcessTimeReporter::new(Some(1), Some(1), Some(1)),
                         seccomp_filters.get("api").unwrap(),
                         vmm::HTTP_MAX_PAYLOAD_SIZE,
@@ -539,7 +496,7 @@ mod tests {
             .spawn(move || {
                 ApiServer::new(api_request_sender, vmm_response_receiver, to_vmm_fd)
                     .bind_and_run(
-                        PathBuf::from(api_thread_path_to_socket),
+                        &PathBuf::from(&api_thread_path_to_socket),
                         ProcessTimeReporter::new(Some(1), Some(1), Some(1)),
                         seccomp_filters.get("api").unwrap(),
                         50,

src/api_server/src/parsed_request.rs

@@ -257,10 +257,10 @@ pub(crate) fn method_to_error(method: Method) -> Result<ParsedRequest, Error> {
 
 #[derive(Debug, derive_more::From)]
 pub(crate) enum Error {
-    // A generic error, with a given status code and message to be turned into a fault message.
-    Generic(StatusCode, String),
     // The resource ID is empty.
     EmptyID,
+    // A generic error, with a given status code and message to be turned into a fault message.
+    Generic(StatusCode, String),
     // The resource ID must only contain alphanumeric characters and '_'.
     InvalidID,
     // The HTTP method & request path combination is not valid.
@@ -272,8 +272,8 @@ pub(crate) enum Error {
 impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
         match self {
-            Error::Generic(_, ref desc) => write!(f, "{}", desc),
             Error::EmptyID => write!(f, "The ID cannot be empty."),
+            Error::Generic(_, ref desc) => write!(f, "{desc}"),
             Error::InvalidID => write!(
                 f,
                 "API Resource IDs can only contain alphanumeric characters and underscores."
@@ -286,8 +286,7 @@ impl std::fmt::Display for Error {
             ),
             Error::SerdeJson(ref err) => write!(
                 f,
-                "An error occurred when deserializing the json body of a request: {}.",
-                err
+                "An error occurred when deserializing the json body of a request: {err}."
             ),
         }
     }

src/arch/src/x86_64/interrupts.rs

@@ -8,6 +8,7 @@
 use kvm_bindings::kvm_lapic_state;
 use kvm_ioctls::VcpuFd;
 use utils::byte_order;
+
 /// Errors thrown while configuring the LAPIC.
 #[derive(Debug, thiserror::Error, PartialEq, Eq)]
 pub enum Error {

src/firecracker/src/api_server_adapter.rs

@@ -9,7 +9,7 @@ use std::sync::mpsc::{channel, Receiver, Sender, TryRecvError};
 use std::sync::{Arc, Mutex};
 use std::thread;
 
-use api_server::{ApiRequest, ApiResponse, ApiServer};
+use api_server::{ApiRequest, ApiResponse, ApiServer, ServerError};
 use event_manager::{EventOps, Events, MutEventSubscriber, SubscriberOps};
 use logger::{error, warn, ProcessTimeReporter};
 use seccompiler::BpfThreadMap;
@@ -149,25 +149,26 @@ pub(crate) fn run_with_api(
         .name("fc_api".to_owned())
         .spawn(move || {
             match ApiServer::new(to_vmm, from_vmm, to_vmm_event_fd).bind_and_run(
-                api_bind_path,
+                &api_bind_path,
                 process_time_reporter,
                 &api_seccomp_filter,
                 api_payload_limit,
                 socket_ready_sender,
             ) {
                 Ok(_) => (),
-                Err(api_server::Error::Io(inner)) => match inner.kind() {
-                    std::io::ErrorKind::AddrInUse => panic!(
-                        "Failed to open the API socket: {:?}",
-                        api_server::Error::Io(inner)
-                    ),
-                    _ => panic!(
-                        "Failed to communicate with the API socket: {:?}",
-                        api_server::Error::Io(inner)
-                    ),
-                },
-                Err(eventfd_err @ api_server::Error::Eventfd(_)) => {
-                    panic!("Failed to open the API socket: {:?}", eventfd_err)
+                Err(api_server::Error::ServerCreation(ServerError::IOError(inner)))
+                    if inner.kind() == std::io::ErrorKind::AddrInUse =>
+                {
+                    let sock_path = api_bind_path.display().to_string();
+                    error!(
+                        "Failed to open the API socket at: {sock_path}. Check that it is not \
+                         already used."
+                    );
+                    std::process::exit(vmm::FcExitCode::GenericError as i32);
+                }
+                Err(api_server::Error::ServerCreation(err)) => {
+                    error!("Failed to bind and run the HTTP server: {err}");
+                    std::process::exit(vmm::FcExitCode::GenericError as i32);
                 }
             }
         })

tests/integration_tests/functional/test_api_server.py

@@ -0,0 +1,29 @@
+# Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+# SPDX-License-Identifier: Apache-2.0
+"""Tests scenario exercising api server functionality."""
+
+import socket
+from framework.utils import run_cmd
+
+
+def test_api_socket_in_use(test_microvm_with_api):
+    """
+    Test error message when api socket is already in use.
+
+    This is a very frequent scenario when Firecracker cannot
+    start due to the socket being left open from previous runs.
+    Check that the error message is a fixed one and that it also
+    contains the name of the path.
+
+    @type: functional
+    """
+    microvm = test_microvm_with_api
+
+    cmd = "mkdir {}/run".format(microvm.chroot())
+    run_cmd(cmd)
+
+    sock = socket.socket(socket.AF_UNIX)
+    sock.bind(microvm.jailer.api_socket_path())
+    microvm.spawn()
+    msg = "Failed to open the API socket at: /run/firecracker.socket. Check that it is not already used."
+    microvm.check_log_message(msg)