Re: [PATCH] rust: task: add `as_raw()` to `Task`

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Jan 15, 2024 at 09:28:22PM -0500, Antonio Hickey wrote:
> Added new function `Task::as_raw()` which returns the raw pointer
> for the underlying task struct. I also refactored `Task` to instead
> use the newly created function instead of `self.0.get()` as I feel
> like `self.as_raw()` is more intuitive.
> 
> Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
> ---
>  rust/kernel/task.rs | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index d2f2615fe4a1..72737f5d86ab 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -124,11 +124,16 @@ fn deref(&self) -> &Self::Target {
>          }
>      }
>  
> +    /// Returns a raw pointer to the underlying C task struct.
> +    pub fn as_raw(&self) -> *mut bindings::task_struct {
> +        self.0.get()
> +    }
> +
>      /// Returns the group leader of the given task.
>      pub fn group_leader(&self) -> &Task {
> -        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> +        // SAFETY: By the type invariant, we know that `self.as_raw()` is a valid task. Valid tasks always

You will need to update the type invariant on `as_raw()` then, if you
want to use it here ;-)

>          // have a valid group_leader.
> -        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
> +        let ptr = unsafe { *ptr::addr_of!((*self.as_raw()).group_leader) };
>  
>          // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
>          // and given that a task has a reference to its group leader, we know it must be valid for
> @@ -138,43 +143,43 @@ pub fn group_leader(&self) -> &Task {
>  
>      /// Returns the PID of the given task.
>      pub fn pid(&self) -> Pid {
> -        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> +        // SAFETY: By the type invariant, we know that `self.as_raw()` is a valid task. Valid tasks always
>          // have a valid pid.
> -        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
> +        unsafe { *ptr::addr_of!((*self.as_raw()).pid) }
[...]
>      /// Wakes up the task.
>      pub fn wake_up(&self) {
> -        // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid.
> +        // SAFETY: By the type invariant, we know that `self.raw()` is non-null and valid.

While we are at it, maybe we can clarify the terminology a bit,
according to:

	https://doc.rust-lang.org/core/ptr/index.html#safety

"valid" implies "non-null", so here we can just say:

	we know that `self.raw()` is a valid task pointer.

Of course, this only applies when we are talking about the validity of
a pointer in Rust. In other words, it's OK to say "null is the valid
input for this C function".

Regards,
Boqun

>          // And `wake_up_process` is safe to be called for any valid task, even if the task is
>          // running.
> -        unsafe { bindings::wake_up_process(self.0.get()) };
> +        unsafe { bindings::wake_up_process(self.as_raw()) };
>      }
>  }
>  
> -- 
> 2.43.0
>