misc: clippy: add needless_pass_by_value #7519
Merged
+287
−239
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
phip1611
force-pushed
the
clippy
branch
10 times, most recently
from
399af4b to
1022470
Compare
This is a follow-up of [0]. # Advantages - This saves dozens of unneeded clone()s across the whole code base - Makes it much easier to reason about how parameters are used (often we passed owned Arc/Rc versions without actually needing ownership) # Exceptions For certain code paths, the alternatives would require awkward or overly complex code, and in some cases the functions are the logical owners of the values they take. In those cases, I've added #[allow(clippy::needless_pass_by_value)]. This does not mean that one should not improve this in the future. [0] 6a86c15 Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
`impl AsRef<Path>` is the most idiomatic way to consume paths in Rust. I removed the mixture. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
rbradford
reviewed
Nov 27, 2025
| fd: RawFd, | ||
| f: F, | ||
| original_termios_opt: Arc<Mutex<Option<termios>>>, | ||
| original_termios_opt: &Mutex<Option<termios>>, |
Member
There was a problem hiding this comment.
Does it ever make sense to have a Mutex without an Arc ?
Member
Author
There was a problem hiding this comment.
The caller still has Arc<Mutex<T>>. The idea here is that the function doesn't need any of Arc's abstractions, for example no clone(). So I reduced the parameters to what is actually used and needed. It is more explicit and we know there are no clones() to expect in such cases
Member
There was a problem hiding this comment.
A clone on an Arc isn't bad - it's only a refcount increase - I don't have a reference to it now but I think some in the Rust community regret overloading Clone for that purpose on Arc.
I just worry that this might increase the cognitive load - the Arc<Mutex<..>> pattern is so common.
Member
Author
There was a problem hiding this comment.
A clone on an Arc isn't bad - it's only a refcount increase
Yes. My idea was the following: Having &T instead of &Arc<T> helps every developer to understand that this code won't create further strong or weak references. It's not really about the costs of the cheap clones.
Arc<Mutex<..>>
I see your concern! But we will keep Arc<Mutex<..>> in most places. Just some called functions are affected by that in case they never create a strong or weak reference.
(I don't have a strong opinion on this, but a tendency that this is benefictial :)
rbradford
reviewed
Nov 27, 2025
| } | ||
|
|
||
| #[allow(clippy::needless_pass_by_value)] | ||
| fn start_vmm(cmd_arguments: ArgMatches) -> Result<Option<String>, Error> { |
Member
There was a problem hiding this comment.
So, could this be cmd_arguments: &ArgMatches ? I'm sure there was a reason why you didn't change it - can you elaborate?
Member
Author
There was a problem hiding this comment.
It could be, yes. Here, I had a feeling of start_vmm should logically own and consume the type. No strong opinion.
Member
There was a problem hiding this comment.
Okay - I get it. I think we can move forward with this PR.
rbradford
approved these changes
Nov 27, 2025
phip1611
added a commit
to phip1611/cloud-hypervisor
that referenced
this pull request
Dec 10, 2025
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. This was my attempt: ```patch diff --git a/vmm/src/console_devices.rs b/vmm/src/console_devices.rs index 6e1ef20..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 88084cb..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] cloud-hypervisor#7519 Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
phip1611
added a commit
to phip1611/cloud-hypervisor
that referenced
this pull request
Dec 10, 2025
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. This was my attempt: ```patch diff --git a/vmm/src/console_devices.rs b/vmm/src/console_devices.rs index 6e1ef20..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 88084cb..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] cloud-hypervisor#7519 Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Quick follow-up to #7493. I found a pragmatic way to enable
needless_pass_by_valueacross the entire repository: we can keep a small set of targeted#[allow]annotations in places where passing by value is intentional (or not worth the consequences), while still linting the rest of the codebase. With this approach I was able to remove dozens of unnecessary clones, and I also discovered several functions that tookArcorRcby value despite never using any of the smartpointer functionality.Given the clear improvements to overall code quality, I believe this change is worthwhile
Part of #7489