Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The #[must_use] annotation is missing on Lock::try_lock #1133

Open
Darksonn opened this issue Dec 4, 2024 · 5 comments
Open

The #[must_use] annotation is missing on Lock::try_lock #1133

Darksonn opened this issue Dec 4, 2024 · 5 comments
Labels
easy Expected to be an easy issue to resolve. good first issue Good for newcomers • lib Related to the `rust/` library.

Comments

@Darksonn
Copy link
Collaborator

Darksonn commented Dec 4, 2024

The Guard type for locks is marked with a #[must_use] annotation like this:

#[must_use = "the lock unlocks immediately when the guard is unused"]
pub struct Guard<'a, T: ?Sized, B: Backend> {

This means that trying to use Lock::lock without using the return value will result in a warning. However, this warning does not happen with Lock::try_lock, since it returns an Option<Guard<...>> and the Option type is not #[must_use].

/// Tries to acquire the lock.
///
/// Returns a guard that can be used to access the data protected by the lock if successful.
pub fn try_lock(&self) -> Option<Guard<'_, T, B>> {
// SAFETY: The constructor of the type calls `init`, so the existence of the object proves
// that `init` was called.
unsafe { B::try_lock(self.state.get()).map(|state| Guard::new(self, state)) }
}

To fix this, add a #[must_use] annotation to Lock::try_lock directly.


This requires submitting a proper patch to the LKML and the Rust for Linux mailing list. Please recall to test your changes (including generating the documentation if changed, running the Rust doctests if changed, etc.), to use a proper title for the commit, to sign your commit under the Developer's Certificate of Origin and to add a Suggested-by: tag and a Link: tag to this issue. Please see https://rust-for-linux.com/contributing for details.

Please take this issue only if you are new to the kernel development process and you would like to use it as a test to submit your first patch to the kernel. Please do not take it if you do not plan to make other contributions to the kernel.

@Darksonn Darksonn added • lib Related to the `rust/` library. good first issue Good for newcomers easy Expected to be an easy issue to resolve. labels Dec 4, 2024
@BorisChenCZY
Copy link

I'm new to the kernel development and rust also. Can I take this one?

@ojeda
Copy link
Member

ojeda commented Dec 7, 2024

Of course, please go ahead!

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 8, 2024
The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused. This
could result in the lock being unlocked immediately, which is unsafe
and unintended. This patch adds a `#[must_use]` annotation to
`Lock::try_lock` to prevent this.

Suggested-by: Alice Ryhl <[email protected]>
Link: Rust-for-Linux#1133
Signed-off-by: Jason Devers <[email protected]>
@hz2
Copy link

hz2 commented Dec 8, 2024

@BorisChenCZY apologizes for not seeing this before submitting the patch, i'm also new to kernel development and would enjoy working on this with you.

i can try to cc you on the current mailing list thread for this patch if you're not already on the mailing list?

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 8, 2024
The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Suggested-by: Alice Ryhl <[email protected]>
Link: Rust-for-Linux#1133
Signed-off-by: Jason Devers <[email protected]>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 8, 2024
The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: rust-lang/rust#71368.

Suggested-by: Alice Ryhl <[email protected]>
Link: Rust-for-Linux#1133
Signed-off-by: Jason Devers <[email protected]>
@BorisChenCZY
Copy link

BorisChenCZY commented Dec 9, 2024 via email

@hz2
Copy link

hz2 commented Dec 9, 2024

@BorisChenCZY cc'd you into the email chain for the patch, also can find the in the archive

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 11, 2024
The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: rust-lang/rust#71368.

Suggested-by: Alice Ryhl <[email protected]>
Link: Rust-for-Linux#1133
Signed-off-by: Jason Devers <[email protected]>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 12, 2024
The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: rust-lang/rust#71368.

Suggested-by: Alice Ryhl <[email protected]>
Link: Rust-for-Linux#1133
Signed-off-by: Jason Devers <[email protected]>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 12, 2024
The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: rust-lang/rust#71368.

Suggested-by: Alice Ryhl <[email protected]>
Link: Rust-for-Linux#1133
Signed-off-by: Jason Devers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Expected to be an easy issue to resolve. good first issue Good for newcomers • lib Related to the `rust/` library.
Development

No branches or pull requests

4 participants