Skip to content

Conversation

@phip1611
Copy link
Member

In [0] we refactored some Arc<Mutex> parameters to &Mutex> to satisfy clippy's needless_pass_by_value lint. Nevertheless, this is also not so idiomatic, so as a follow-up, we put the responsibility to lock objects to the caller side (only where this is not strictly needed by the callee).

While on it, I also tried to pass vm_config directly into pre_create_console_devices() which would clean up some code, but then we have interleaving mutable and immutable borrows of the Vmm.

This was my attempt:

diff --git a/vmm/src/console_devices.rs b/vmm/src/console_devices.rs
index 6e1ef2039..105d005dc 100644
--- a/vmm/src/console_devices.rs
+++ b/vmm/src/console_devices.rs
@@ -24,7 +24,7 @@ use thiserror::Error;

 use crate::Vmm;
 use crate::sigwinch_listener::listen_for_sigwinch_on_tty;
-use crate::vm_config::ConsoleOutputMode;
+use crate::vm_config::{ConsoleOutputMode, VmConfig};

 const TIOCSPTLCK: libc::c_int = 0x4004_5431;
 const TIOCGPTPEER: libc::c_int = 0x5441;
diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs
index 88084cb38..1ef6d7b63 100644
--- a/vmm/src/lib.rs
+++ b/vmm/src/lib.rs
@@ -1002,9 +1002,16 @@ impl Vmm {

         let config = vm_migration_config.vm_config.clone();
         self.vm_config = Some(vm_migration_config.vm_config);
-        self.console_info = Some(pre_create_console_devices(self).map_err(|e| {
-            MigratableError::MigrateReceive(anyhow!("Error creating console devices: {e:?}"))
-        })?);
+        self.console_info = {
+            let mut vmconfig = self.vm_config.as_ref().unwrap().lock().unwrap();
+            Some(
+                pre_create_console_devices(self, &mut *vmconfig).map_err(|e| {
+                    MigratableError::MigrateReceive(anyhow!(
+                        "Error creating console devices: {e:?}"
+                    ))
+                })?,
+            )
+        };

         if self
             .vm_config
@@ -1443,7 +1450,7 @@ impl Vmm {
         self.vm_check_cpuid_compatibility(&vm_config, &vm_snapshot.common_cpuid)
             .map_err(VmError::Restore)?;

-        self.vm_config = Some(Arc::clone(&vm_config));
+        self.vm_config = Some(vm_config);

         // Always re-populate the 'console_info' based on the new 'vm_config'
         self.console_info =
@@ -1613,25 +1620,23 @@ fn apply_landlock(vm_config: &mut VmConfig) -> result::Result<(), LandlockError>
 }

 impl RequestHandler for Vmm {
-    fn vm_create(&mut self, config: Box<VmConfig>) -> result::Result<(), VmError> {
+    fn vm_create(&mut self, mut config: Box<VmConfig>) -> result::Result<(), VmError> {
         // We only store the passed VM config.
         // The VM will be created when being asked to boot it.
         if self.vm_config.is_some() {
             return Err(VmError::VmAlreadyCreated);
         }

-        self.vm_config = Some(Arc::new(Mutex::new(*config)));
-        self.console_info =
-            Some(pre_create_console_devices(self).map_err(VmError::CreateConsoleDevices)?);
+        self.console_info = Some(
+            pre_create_console_devices(self, &mut *config)
+                .map_err(VmError::CreateConsoleDevices)?,
+        );

-        if self
-            .vm_config
-            .as_ref()
-            .is_some_and(|config| config.lock().unwrap().landlock_enable)
-        {
-            let mut config = self.vm_config.as_ref().unwrap().lock().unwrap();
+        if config.landlock_enable {
             apply_landlock(&mut *config).map_err(VmError::ApplyLandlock)?;
         }
+
+        self.vm_config = Some(Arc::new(Mutex::new(*config)));
         Ok(())
     }

@@ -1648,8 +1653,11 @@ impl RequestHandler for Vmm {

             // console_info is set to None in vm_shutdown. re-populate here if empty
             if self.console_info.is_none() {
-                self.console_info =
-                    Some(pre_create_console_devices(self).map_err(VmError::CreateConsoleDevices)?);
+                let mut vmconfig = self.vm_config.as_ref().unwrap().lock().unwrap();
+                self.console_info = Some(
+                    pre_create_console_devices(self, &mut *vmconfig)
+                        .map_err(VmError::CreateConsoleDevices)?,
+                );
             }

             // Create a new VM if we don't have one yet.

[0] #7519

@phip1611 phip1611 requested a review from a team as a code owner December 10, 2025 06:04
@phip1611 phip1611 mentioned this pull request Dec 10, 2025
@phip1611 phip1611 self-assigned this Dec 10, 2025
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - not sure if I like having the patch suggestion in the commit message though

In [0] we refactored some Arc<Mutex<T>> parameters to &Mutex<T>> to
satisfy clippy's needless_pass_by_value lint. Nevertheless, this is also
not so idiomatic, so as a follow-up, we put the responsibility to lock
objects to the caller side (only where this is not strictly needed by
the callee).

While on it, I also tried to pass vm_config directly into
pre_create_console_devices() which would clean up some code, but then
we have interleaving mutable and immutable borrows of the Vmm, which
are denied by the borrow checker.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@rbradford rbradford added this pull request to the merge queue Dec 10, 2025
Merged via the queue into cloud-hypervisor:main with commit 6bda654 Dec 10, 2025
43 checks passed
@phip1611 phip1611 deleted the mutex branch December 12, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants