When `populate()` succeeds, there's no way infallible way to get the value that was just inserted. By returning &T in this case, such infallible access methods become possible.
Additionally, when `populate()` fails, the provided value is dropped. This has two disadvantages: 1. If the caller holds a lock, the value is dropped under said lock. 2. If the caller wishes to use the same value for something else, they can't, because it's lost. Changing the return value to Result<&T, T> handles all of these cases. Rust Binder is updated to avoid a warning about an unused Result. Additionally, ModuleParam is updated to correctly translate the new return value to the right target values. Signed-off-by: Alice Ryhl <[email protected]> --- drivers/android/binder/process.rs | 5 +++-- rust/kernel/module_param.rs | 8 ++++---- rust/kernel/sync/set_once.rs | 15 +++++++++------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs index 1778628d8acd..d486bf7c0b8a 100644 --- a/drivers/android/binder/process.rs +++ b/drivers/android/binder/process.rs @@ -1801,8 +1801,9 @@ pub(crate) fn poll( let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?; // Reuse our existing lock to synchronize callers initializing. - let _guard = this.node_refs.lock(); - this.poll.populate(poll); + let guard = this.node_refs.lock(); + let _ret = this.poll.populate(poll); + drop(guard); }; table.register_wait(file, poll); diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs index 6541af218390..8f0bd085badf 100644 --- a/rust/kernel/module_param.rs +++ b/rust/kernel/module_param.rs @@ -77,10 +77,10 @@ pub trait ModuleParam: Sized + Copy { // SAFETY: By function safety requirements, this access is safe. let container = unsafe { &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<T>>()) }; - container - .populate(new_value) - .then_some(0) - .ok_or(kernel::error::code::EEXIST) + match container.populate(new_value) { + Ok(_) => Ok(0), + Err(_) => Err(EEXIST), + } }) } diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs index 139cef05e935..a78f8c8e87db 100644 --- a/rust/kernel/sync/set_once.rs +++ b/rust/kernel/sync/set_once.rs @@ -31,12 +31,12 @@ /// assert_eq!(None, value.as_ref()); /// /// let status = value.populate(42u8); -/// assert_eq!(true, status); +/// assert_eq!(Ok(&42u8), status); /// assert_eq!(Some(&42u8), value.as_ref()); /// assert_eq!(Some(42u8), value.copy()); /// /// let status = value.populate(101u8); -/// assert_eq!(false, status); +/// assert_eq!(Err(101u8), status); /// assert_eq!(Some(&42u8), value.as_ref()); /// assert_eq!(Some(42u8), value.copy()); /// ``` @@ -78,8 +78,9 @@ pub fn as_ref(&self) -> Option<&T> { /// Populate the [`SetOnce`]. /// - /// Returns `true` if the [`SetOnce`] was successfully populated. - pub fn populate(&self, value: T) -> bool { + /// Returns `Ok(value)` if the [`SetOnce`] was successfully populated with the provided value. + /// Otherwise returns an error containing the value that this call attempted to insert. + pub fn populate(&self, value: T) -> Result<&T, T> { // INVARIANT: If the swap succeeds: // - We increase `init`. // - We write the valid value `1` to `init`. @@ -95,9 +96,11 @@ pub fn populate(&self, value: T) -> bool { // - We release our exclusive access to `self.value` and it is now valid for shared // access. self.init.store(2, Release); - true + // SAFETY: By the type invariants of `Self`, the value is initialized and will stay + // that way. + Ok(unsafe { &*self.value.get().cast() }) } else { - false + Err(value) } } -- 2.55.0.229.g6434b31f56-goog
