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

Change pub(crate) methods in kernel::error to be pub #1105

Closed
Darksonn opened this issue Aug 21, 2024 · 2 comments
Closed

Change pub(crate) methods in kernel::error to be pub #1105

Darksonn opened this issue Aug 21, 2024 · 2 comments
Labels
good first issue Good for newcomers • lib Related to the `rust/` library.

Comments

@Darksonn
Copy link
Collaborator

Change the following methods from pub(crate) to pub:

  • from_result
  • from_err_ptr
  • to_ptr

These methods will most likely be used from outside of the kernel crate, so using the pub marker is the correct way to mark them. Furthermore, using the pub marker instead of pub(crate) has the advantage that it doesn't produce dead-code warnings, avoiding the need for #[allow] and TODO annotations:

linux/rust/kernel/error.rs

Lines 271 to 273 in e26fa54

// TODO: Remove `dead_code` marker once an in-kernel client is available.
#[allow(dead_code)]
pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {

One could argue that these methods shouldn't be used outside of the kernel crate because that means that someone is writing an abstraction directly in their driver. However, there are reasons to do that in some cases, so we cannot apply that rule so strictly that we can make these methods pub(crate). (And if we did apply that rule that strictly, then to_result should be changed to be pub(crate).)

We could make this change later when the methods gain a user outside of rust/kernel directory. However, I propose changing it now because:

  1. Removing #[allow] and TODO annotations from error.rs simplifies it, making the code easier to read.
  2. This will reduce clutter in any future patches that use the methods outside of the kernel crate.

I don't know whether all three methods will gain a user outside of the rust/kernel directory, but I expect that from_err_ptr will. However, I don't think there's any harm in changing all of them for consistency. After all, a pub marker is not incorrect for something only used in rust/kernel if we don't need to prevent users outside of rust/kernel from using it.

@ojeda ojeda added the • lib Related to the `rust/` library. label Aug 25, 2024
@felipeagger
Copy link

felipeagger commented Sep 7, 2024

Hi, I made these adjusts and submitted the Patch, can you check please? Thank you.

Link ML

@Darksonn @ojeda

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Sep 9, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: Rust-for-Linux#1105
Signed-off-by: Filipe Xavier <felipe_life@live.com>
@ojeda ojeda added the good first issue Good for newcomers label Sep 10, 2024
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Sep 13, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: Rust-for-Linux#1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
ojeda pushed a commit to ojeda/linux that referenced this issue Oct 3, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: Rust-for-Linux#1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/DM4PR14MB7276E6948E67B3B23D8EA847E9652@DM4PR14MB7276.namprd14.prod.outlook.com
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda pushed a commit to ojeda/linux that referenced this issue Oct 3, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: Rust-for-Linux#1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/DM4PR14MB7276E6948E67B3B23D8EA847E9652@DM4PR14MB7276.namprd14.prod.outlook.com
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@ojeda
Copy link
Member

ojeda commented Oct 3, 2024

Applied to rust-next -- thanks!

@ojeda ojeda closed this as completed Oct 3, 2024
ojeda pushed a commit that referenced this issue Oct 7, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: #1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/DM4PR14MB7276E6948E67B3B23D8EA847E9652@DM4PR14MB7276.namprd14.prod.outlook.com
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda pushed a commit that referenced this issue Oct 7, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: #1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/DM4PR14MB7276E6948E67B3B23D8EA847E9652@DM4PR14MB7276.namprd14.prod.outlook.com
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
alistair23 pushed a commit to alistair23/linux that referenced this issue Nov 11, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: Rust-for-Linux#1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/DM4PR14MB7276E6948E67B3B23D8EA847E9652@DM4PR14MB7276.namprd14.prod.outlook.com
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
alistair23 pushed a commit to alistair23/linux that referenced this issue Nov 12, 2024
Change visibility to public of functions in error.rs:
from_err_ptr, from_errno, from_result and to_ptr.
Additionally, remove dead_code annotations.

Link: Rust-for-Linux#1105
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/DM4PR14MB7276E6948E67B3B23D8EA847E9652@DM4PR14MB7276.namprd14.prod.outlook.com
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers • lib Related to the `rust/` library.
Development

No branches or pull requests

3 participants