refactor: centralize activation failure logging

Log activation failures at the only call-site of `activate`, instead of
inside each individual `activate` function. For this, untangle some of
the `ActivationError` variants - `BadActivate` was almost exclusively
used in the case where writing to the activation eventfd failed, except
in the vsock device, where it was also used to indicate that the number
of queues the guest gave us was wrong (which this commit factors out
into its own error variant).

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/vmm/src/devices/virtio/balloon/device.rs

@@ -613,10 +613,9 @@ impl VirtioDevice for Balloon {
     fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> {
         self.device_state = DeviceState::Activated(mem);
         if self.activate_evt.write(1).is_err() {
-            error!("Balloon: Cannot write to activate_evt");
             METRICS.activate_fails.inc();
             self.device_state = DeviceState::Inactive;
-            return Err(ActivateError::BadActivate);
+            return Err(ActivateError::EventFd);
         }
 
         if self.stats_enabled() {

src/vmm/src/devices/virtio/block/virtio/device.rs

@@ -652,8 +652,7 @@ impl VirtioDevice for VirtioBlock {
         }
 
         if self.activate_evt.write(1).is_err() {
-            error!("Block: Cannot write to activate_evt");
-            return Err(ActivateError::BadActivate);
+            return Err(ActivateError::EventFd);
         }
         self.device_state = DeviceState::Activated(mem);
         Ok(())

src/vmm/src/devices/virtio/mmio.rs

@@ -14,7 +14,7 @@ use utils::byte_order;
 use crate::devices::virtio::device::{IrqType, VirtioDevice};
 use crate::devices::virtio::device_status;
 use crate::devices::virtio::queue::Queue;
-use crate::logger::warn;
+use crate::logger::{error, warn};
 use crate::vstate::memory::{GuestAddress, GuestMemoryMmap};
 
 // TODO crosvm uses 0 here, but IIRC virtio specified some other vendor id that should be used
@@ -186,18 +186,21 @@ impl MmioTransport {
             DRIVER_OK if self.device_status == (ACKNOWLEDGE | DRIVER | FEATURES_OK) => {
                 self.device_status = status;
                 let device_activated = self.locked_device().is_activated();
-                if !device_activated
-                    && self.are_queues_valid()
-                    && self.locked_device().activate(self.mem.clone()).is_err()
-                {
-                    self.device_status |= DEVICE_NEEDS_RESET;
-
-                    // Section 2.1.2 of the specification states that we need to send a device
-                    // configuration change interrupt
-                    let _ = self
-                        .locked_device()
-                        .interrupt_trigger()
-                        .trigger_irq(IrqType::Config);
+                if !device_activated && self.are_queues_valid() {
+                    // temporary variable needed for borrow checker
+                    let activate_result = self.locked_device().activate(self.mem.clone());
+                    if let Err(err) = activate_result {
+                        self.device_status |= DEVICE_NEEDS_RESET;
+
+                        // Section 2.1.2 of the specification states that we need to send a device
+                        // configuration change interrupt
+                        let _ = self
+                            .locked_device()
+                            .interrupt_trigger()
+                            .trigger_irq(IrqType::Config);
+
+                        error!("Failed to activate virtio device: {}", err)
+                    }
                 }
             }
             _ if (status & FAILED) != 0 => {
@@ -462,7 +465,7 @@ pub(crate) mod tests {
         fn activate(&mut self, _: GuestMemoryMmap) -> Result<(), ActivateError> {
             self.device_activated = true;
             if self.activate_should_error {
-                Err(ActivateError::BadActivate)
+                Err(ActivateError::EventFd)
             } else {
                 Ok(())
             }

src/vmm/src/devices/virtio/mod.rs

@@ -8,7 +8,6 @@
 //! Implements virtio devices, queues, and transport mechanisms.
 
 use std::any::Any;
-use std::io::Error as IOError;
 
 pub mod balloon;
 pub mod block;
@@ -61,10 +60,10 @@ pub const NOTIFY_REG_OFFSET: u32 = 0x50;
 /// Errors triggered when activating a VirtioDevice.
 #[derive(Debug, thiserror::Error, displaydoc::Display)]
 pub enum ActivateError {
-    /// Epoll error.
-    EpollCtl(IOError),
-    /// General error at activation.
-    BadActivate,
+    /// Wrong number of queue for virtio device: expected {expected}, got {got}
+    QueueMismatch { expected: usize, got: usize },
+    /// Failed to write to activate eventfd
+    EventFd,
     /// Vhost user: {0}
     VhostUser(vhost_user::VhostUserError),
 }

src/vmm/src/devices/virtio/net/device.rs

@@ -862,8 +862,7 @@ impl VirtioDevice for Net {
         }
 
         if self.activate_evt.write(1).is_err() {
-            error!("Net: Cannot write to activate_evt");
-            return Err(super::super::ActivateError::BadActivate);
+            return Err(ActivateError::EventFd);
         }
         self.device_state = DeviceState::Activated(mem);
         Ok(())

src/vmm/src/devices/virtio/rng/device.rs

@@ -284,10 +284,9 @@ impl VirtioDevice for Entropy {
     }
 
     fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> {
-        self.activate_event.write(1).map_err(|err| {
-            error!("entropy: Cannot write to activate_evt: {err}");
+        self.activate_event.write(1).map_err(|_| {
             METRICS.activate_fails.inc();
-            super::super::ActivateError::BadActivate
+            ActivateError::EventFd
         })?;
         self.device_state = DeviceState::Activated(mem);
         Ok(())

src/vmm/src/devices/virtio/vsock/device.rs

@@ -325,18 +325,15 @@ where
     fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> {
         if self.queues.len() != defs::VSOCK_NUM_QUEUES {
             METRICS.activate_fails.inc();
-            error!(
-                "Cannot perform activate. Expected {} queue(s), got {}",
-                defs::VSOCK_NUM_QUEUES,
-                self.queues.len()
-            );
-            return Err(ActivateError::BadActivate);
+            return Err(ActivateError::QueueMismatch {
+                expected: defs::VSOCK_NUM_QUEUES,
+                got: self.queues.len(),
+            });
         }
 
         if self.activate_evt.write(1).is_err() {
             METRICS.activate_fails.inc();
-            error!("Cannot write to activate_evt",);
-            return Err(ActivateError::BadActivate);
+            return Err(ActivateError::EventFd);
         }
 
         self.device_state = DeviceState::Activated(mem);