Rust-for-Linux / linux

Adding support for the Rust language to the Linux kernel.
https://rust-for-linux.com
Other
3.92k stars 417 forks source link

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

Open Darksonn opened 3 weeks ago

Darksonn commented 3 weeks ago

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

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:

https://github.com/Rust-for-Linux/linux/blob/e26fa546042add70944d018b930530d16b3cf626/rust/kernel/error.rs#L271-L273

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.

felipeagger commented 1 week ago

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

Link ML

@Darksonn @ojeda