vmm: api: abstract FD handling behind functions

## TL;DR

This unifies error handling, implementation, and logging of config
objects that are populated with additional FDs received as part of
the HTTP API request from the management software. This prevents current
and future code in repetition.

In the future, if we want to support let's say virtio-blk with external
FDs, the AddBlk API handler simply needs a call to `attach_fds_to_cfg()`
and gets all the magic for free.

## Motivation

The motivation of the new abstraction is to provide a verbose, solid,
and bulletproof solution for a complex domain. The interaction between

- the management layer,
- the passing of file descriptors over UNIX domain sockets via
  SCM_RIGHTS,
- the attachment of configurations to those FDs,
- and the ability to give new developers clear insights into what
  happens under the hood

is not trivial. These factors justify encapsulating the complexity
behind a convenient and well-documented abstraction, making the system
both robust in production and approachable for new developers.

In addition, it allows us to perform unit testing. Further, We get rid
of existing partial code duplication and inconsistencies.

Finally, while this approach may initially result in more code, every
new handler that accepts FDs benefits from reduced duplication and a
correct implementation by relying on the shared abstraction. This will
also enable future functionality, such as virtio-blk devices backed by
FDs which can then be integrated with ease.

Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com

vmm/src/api/http/http_endpoint.rs

@@ -28,11 +28,13 @@
 //! We can receive these FDs as we use a [special HTTP library] that is aware
 //! of the described mechanism.
 //!
+//! Please have a look into the [`fds_helper`] module for the technical
+//! implementation.
+//!
 //! [`cmsg(3)`]: https://man7.org/linux/man-pages/man3/cmsg.3.html
 //! [special HTTP library]: https://github.com/firecracker-microvm/micro-http
 
 use std::fs::File;
-use std::os::unix::io::IntoRawFd;
 use std::sync::mpsc::Sender;
 
 use micro_http::{Body, Method, Request, Response, StatusCode, Version};
@@ -40,6 +42,7 @@ use vmm_sys_util::eventfd::EventFd;
 
 #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))]
 use crate::api::VmCoredump;
+use crate::api::http::http_endpoint::fds_helper::{attach_fds_to_cfg, attach_fds_to_cfgs};
 use crate::api::http::{EndpointHandler, HttpError, error_response};
 use crate::api::{
     AddDisk, ApiAction, ApiError, ApiRequest, NetConfig, VmAddDevice, VmAddFs, VmAddNet, VmAddPmem,
@@ -51,6 +54,206 @@ use crate::config::RestoreConfig;
 use crate::cpu::Error as CpuError;
 use crate::vm::Error as VmError;
 
+/// Helper module for attaching externally opened FDs to config objects.
+///
+/// # Difference between [`ConfigWithFDs`] and [`ConfigWithVariableFDs`]
+///
+/// The base trait [`ConfigWithFDs`] type must be implemented by all config
+/// types that want to take ownership of externally provided FDs.
+///
+/// In the case of restore operations, e.g., after a live-migration, config
+/// objects will know the amount of FDs they need. In this case, they must
+/// also implement [`ConfigWithVariableFDs`]. In other scenarios, such as
+/// hot device attach, the base type is sufficient and the type will take
+/// over all available FDs.
+///
+/// In any case, the management software (e.g., libvirt) is responsible for
+/// providing the exact amount of FDs.
+mod fds_helper {
+    use std::fs::File;
+    use std::os::fd::{IntoRawFd, RawFd};
+
+    use crate::api::http::HttpError;
+
+    /// Abstraction over configuration types received via the HTTP API that
+    /// have associated externally opened FDs.
+    pub trait ConfigWithFDs {
+        /// Returns the ID of the device.
+        ///
+        /// Used for logging.
+        fn id(&self) -> Option<&str>;
+
+        /// Returns any FDs provided in the HTTP body.
+        ///
+        /// They will always be invalid and are used for user-facing logging.
+        fn fds_from_http_body(&self) -> Option<&[RawFd]>;
+
+        /// Assigns the provided file descriptors (`fds`) to this configuration
+        /// object.
+        ///
+        /// After calling this method, the configuration will behave as if it
+        /// had originally been created with these FDs. Next, the configuration
+        /// can be used to properly configure the corresponding device.
+        ///
+        /// # Arguments
+        /// - `fds`: Either a non-empty Vector with corresponding FDs or `None`
+        ///   indicating that no valid FDs were supplied.
+        fn set_fds(&mut self, fds: Option<Vec<RawFd>>);
+    }
+
+    /// Extension of [`ConfigWithFDs`] for config objects that know how many
+    /// FDs they want (e.g., a restore configuration that is aware of the
+    /// previous state).
+    pub trait ConfigWithVariableFDs: ConfigWithFDs {
+        /// Returns how many FDs this type wants to have from the pool of
+        /// available FDs.
+        fn expected_num_fds(&self) -> usize;
+    }
+
+    mod config_with_fds_impls {
+        use std::os::fd::RawFd;
+
+        use super::{ConfigWithFDs, ConfigWithVariableFDs};
+        use crate::config::RestoredNetConfig;
+        use crate::vm_config::NetConfig;
+
+        impl ConfigWithFDs for NetConfig {
+            fn id(&self) -> Option<&str> {
+                self.id.as_deref()
+            }
+
+            fn fds_from_http_body(&self) -> Option<&[RawFd]> {
+                self.fds.as_deref()
+            }
+
+            fn set_fds(&mut self, fds: Option<Vec<RawFd>>) {
+                self.fds = fds;
+            }
+        }
+
+        impl ConfigWithFDs for RestoredNetConfig {
+            fn id(&self) -> Option<&str> {
+                Some(self.id.as_str())
+            }
+
+            fn fds_from_http_body(&self) -> Option<&[RawFd]> {
+                self.fds.as_deref()
+            }
+
+            fn set_fds(&mut self, fds: Option<Vec<RawFd>>) {
+                self.fds = fds;
+            }
+        }
+
+        impl ConfigWithVariableFDs for RestoredNetConfig {
+            fn expected_num_fds(&self) -> usize {
+                self.num_fds
+            }
+        }
+    }
+
+    fn attach_fds_to_cfg_inner<T: ConfigWithFDs>(
+        fds: &mut Vec<RawFd>,
+        fds_amount: usize,
+        cfg: &mut T,
+    ) {
+        if cfg.fds_from_http_body().is_some() {
+            // Only FDs transmitted via an SCM_RIGHTS UNIX Domain Socket message
+            // are valid. Any provided over the HTTP API are set to `-1` in our
+            // specialized serializer callbacks.
+            warn!(
+                "FD numbers were present in HTTP request body for device {:?} but will be ignored",
+                cfg.id()
+            );
+
+            // Reset old value in any case; if there are FDs, they are invalid.
+            cfg.set_fds(None);
+        }
+
+        if fds_amount > 0 {
+            let new_fds = fds.drain(..fds_amount).collect::<Vec<_>>();
+            log::debug!(
+                "Attaching network FDs received via UNIX domain socket to device: id={:?}, fds={new_fds:?}",
+                cfg.id()
+            );
+            cfg.set_fds(Some(new_fds));
+        }
+    }
+
+    /// Applies FDs to configs for their corresponding devices, as part of the special
+    /// handling for devices backed by externally provided FDs.
+    ///
+    /// The FDs (via `files`) must be provided in the exact order matching the
+    /// config struct they belong to.
+    ///
+    /// See [module description] for more info.
+    ///
+    /// # Arguments
+    /// - `device_fds`: Ordered list of all FDs from the request.
+    /// - `cfgs`: List of network configurations where each network can have up to `n` FDs.
+    ///
+    /// [module description]: self
+    pub fn attach_fds_to_cfgs<T: ConfigWithVariableFDs>(
+        device_fds: Vec<File>,
+        cfgs: &mut [&mut T],
+    ) -> Result<(), HttpError> {
+        let expected_fds: usize = cfgs.iter().map(|cfg| cfg.expected_num_fds()).sum();
+
+        if device_fds.len() != expected_fds {
+            error!(
+                "Number of expected FDs: {}, received: {}",
+                expected_fds,
+                device_fds.len()
+            );
+            return Err(HttpError::BadRequest);
+        }
+
+        // We are only interested in the raw FDs. After this operation, we are
+        // responsible for manually closing the FDs eventually.
+        let mut fds = device_fds
+            .into_iter()
+            .map(|f| f.into_raw_fd())
+            .collect::<Vec<_>>();
+
+        // For each config: We drain the FDs vector by the amount of FDs the config expects.
+        for cfg in cfgs {
+            attach_fds_to_cfg_inner(&mut fds, cfg.expected_num_fds(), *cfg);
+        }
+
+        // We checked that `fds.len() == expected_fds`; so if we panic here, we
+        // have a hard programming error
+        assert!(fds.is_empty());
+
+        Ok(())
+    }
+
+    /// Applies FDs to the config for the corresponding device, as part of the special
+    /// handling for devices backed by externally provided FDs.
+    ///
+    /// See [module description] for more info.
+    ///
+    /// # Arguments
+    /// - `device_fds`: Ordered list of all FDs from the request.
+    /// - `cfg`: The config object that wants to take ownership of all available FDs.
+    ///
+    /// [module description]: self
+    pub fn attach_fds_to_cfg<T: ConfigWithFDs>(
+        device_fds: Vec<File>,
+        cfg: &mut T,
+    ) -> Result<(), HttpError> {
+        // We are only interested in the raw FDs.
+        let mut fds = device_fds
+            .into_iter()
+            .map(|f| f.into_raw_fd())
+            .collect::<Vec<_>>();
+
+        let len = fds.len();
+        attach_fds_to_cfg_inner(&mut fds, len, cfg);
+
+        Ok(())
+    }
+}
+
 // /api/v1/vm.create handler
 pub struct VmCreate {}
 
@@ -74,11 +277,18 @@ impl EndpointHandler for VmCreate {
                         };
 
                         if let Some(ref mut nets) = vm_config.net {
-                            if nets.iter().any(|net| net.fds.is_some()) {
-                                warn!("Ignoring FDs sent via the HTTP request body");
-                            }
-                            for net in nets {
-                                net.fds = None;
+                            let mut cfgs = nets.iter_mut().collect::<Vec<&mut _>>();
+                            let cfgs = cfgs.as_mut_slice();
+
+                            // For the VmCreate call, we do not accept FDs from the socket currently.
+                            // This call sets all FDs to null while doing the same logging as
+                            // similar code paths.
+                            for cfg in cfgs {
+                                if let Err(e) = attach_fds_to_cfg(vec![], *cfg)
+                                    .map_err(|e| error_response(e, StatusCode::InternalServerError))
+                                {
+                                    return e;
+                                }
                             }
                         }
 
@@ -228,18 +438,12 @@ impl PutHandler for VmAddNet {
         api_notifier: EventFd,
         api_sender: Sender<ApiRequest>,
         body: &Option<Body>,
-        mut files: Vec<File>,
+        files: Vec<File>,
     ) -> std::result::Result<Option<Body>, HttpError> {
         if let Some(body) = body {
             let mut net_cfg: NetConfig = serde_json::from_slice(body.raw())?;
-            if net_cfg.fds.is_some() {
-                warn!("Ignoring FDs sent via the HTTP request body");
-                net_cfg.fds = None;
-            }
-            if !files.is_empty() {
-                let fds = files.drain(..).map(|f| f.into_raw_fd()).collect();
-                net_cfg.fds = Some(fds);
-            }
+            attach_fds_to_cfg(files, &mut net_cfg)?;
+
             self.send(api_notifier, api_sender, net_cfg)
                 .map_err(HttpError::ApiError)
         } else {
@@ -286,35 +490,15 @@ impl PutHandler for VmRestore {
         api_notifier: EventFd,
         api_sender: Sender<ApiRequest>,
         body: &Option<Body>,
-        mut files: Vec<File>,
+        files: Vec<File>,
     ) -> std::result::Result<Option<Body>, HttpError> {
         if let Some(body) = body {
             let mut restore_cfg: RestoreConfig = serde_json::from_slice(body.raw())?;
 
-            let mut fds = Vec::new();
-            if !files.is_empty() {
-                fds = files.drain(..).map(|f| f.into_raw_fd()).collect();
-            }
-            let expected_fds = match restore_cfg.net_fds {
-                Some(ref net_fds) => net_fds.iter().map(|net| net.num_fds).sum(),
-                None => 0,
-            };
-            if fds.len() != expected_fds {
-                error!(
-                    "Number of FDs expected: {}, but received: {}",
-                    expected_fds,
-                    fds.len()
-                );
-                return Err(HttpError::BadRequest);
-            }
-            if let Some(ref mut nets) = restore_cfg.net_fds {
-                warn!("Ignoring FDs sent via the HTTP request body");
-                let mut start_idx = 0;
-                for restored_net in nets.iter_mut() {
-                    let end_idx = start_idx + restored_net.num_fds;
-                    restored_net.fds = Some(fds[start_idx..end_idx].to_vec());
-                    start_idx = end_idx;
-                }
+            if let Some(cfgs) = restore_cfg.net_fds.as_mut() {
+                let mut cfgs = cfgs.iter_mut().collect::<Vec<&mut _>>();
+                let cfgs = cfgs.as_mut_slice();
+                attach_fds_to_cfgs(files, cfgs)?;
             }
 
             self.send(api_notifier, api_sender, restore_cfg)
@@ -442,3 +626,115 @@ impl EndpointHandler for VmmShutdown {
         }
     }
 }
+
+#[cfg(test)]
+mod external_fds_tests {
+    use super::*;
+    use crate::api::http::http_endpoint::fds_helper::{ConfigWithFDs, ConfigWithVariableFDs};
+
+    struct DummyNewDeviceCfg {
+        http_fds: Option<Vec<i32>>,
+    }
+
+    impl ConfigWithFDs for DummyNewDeviceCfg {
+        fn id(&self) -> Option<&str> {
+            Some("dummy")
+        }
+
+        fn fds_from_http_body(&self) -> Option<&[i32]> {
+            self.http_fds.as_deref()
+        }
+
+        fn set_fds(&mut self, fds: Option<Vec<i32>>) {
+            self.http_fds = fds;
+        }
+    }
+
+    struct DummyRestoreDeviceCfg {
+        http_fds: Option<Vec<i32>>,
+        num_fds: usize,
+    }
+
+    impl ConfigWithFDs for DummyRestoreDeviceCfg {
+        fn id(&self) -> Option<&str> {
+            Some("dummy")
+        }
+
+        fn fds_from_http_body(&self) -> Option<&[i32]> {
+            self.http_fds.as_deref()
+        }
+
+        fn set_fds(&mut self, fds: Option<Vec<i32>>) {
+            self.http_fds = fds;
+        }
+    }
+
+    impl ConfigWithVariableFDs for DummyRestoreDeviceCfg {
+        fn expected_num_fds(&self) -> usize {
+            self.num_fds
+        }
+    }
+
+    #[test]
+    fn test_fds_provided_via_http_api_are_reset() {
+        let mut config = DummyNewDeviceCfg {
+            http_fds: Some(vec![1, 2, 3]),
+        };
+
+        attach_fds_to_cfg(vec![], &mut config).unwrap();
+        assert_eq!(config.http_fds, None);
+    }
+
+    #[test]
+    fn test_new_device_cfg_takes_all_fds() {
+        let path = "/dev/null";
+
+        let new_fds = vec![
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+        ];
+        let mut config = DummyNewDeviceCfg {
+            http_fds: Some(vec![1, 2, 3]),
+        };
+
+        attach_fds_to_cfg(new_fds, &mut config).unwrap();
+        assert_eq!(config.http_fds.unwrap().len(), 3);
+    }
+
+    #[test]
+    fn test_restore_cfgs_take_only_their_fds() {
+        let path = "/dev/null";
+        let new_fds = vec![
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+            File::open(path).unwrap(),
+        ];
+        let mut config1 = DummyRestoreDeviceCfg {
+            http_fds: None,
+            num_fds: 3,
+        };
+        let mut config2 = DummyRestoreDeviceCfg {
+            http_fds: None,
+            num_fds: 1,
+        };
+        let mut config3 = DummyRestoreDeviceCfg {
+            http_fds: None,
+            num_fds: 0,
+        };
+        let mut config4 = DummyRestoreDeviceCfg {
+            http_fds: None,
+            num_fds: 2,
+        };
+        let mut configs = [&mut config1, &mut config2, &mut config3, &mut config4];
+
+        attach_fds_to_cfgs(new_fds, &mut configs).unwrap();
+        assert_eq!(config1.http_fds.unwrap().len(), 3);
+        assert_eq!(config2.http_fds.unwrap().len(), 1);
+        assert!(config3.http_fds.is_none());
+        assert_eq!(config4.http_fds.unwrap().len(), 2);
+    }
+}