Re: [PATCH] rust: error: Extend the Result documentation

[Daniel Sedlak <daniel@sedlak.dev>]

On 1/7/25 7:21 AM, Dirk Behme wrote:
> Extend the Result documentation by some guidelines and examples how
> to handle Result error cases gracefully. And how to not handle them.
> 
> Link: https://lore.kernel.org/rust-for-linux/CANiq72keOdXy0LFKk9SzYWwSjiD710v=hQO4xi+5E4xNALa6cA@mail.gmail.com/
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>   rust/kernel/error.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 0b01975c2286c..456487d4a8ed8 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -256,6 +256,72 @@ fn from(e: core::convert::Infallible) -> Error {
>   /// Note that even if a function does not return anything when it succeeds,
>   /// it should still be modeled as returning a `Result` rather than
>   /// just an [`Error`].
> +///
> +/// Calling a function that returns [`Result`] needs the caller to handle
> +/// the returned [`Result`].
> +///
> +/// This can be done "manually" by using [`match`](https://doc.rust-lang.org/reference/expressions/match-expr.html)
> +/// Using [`match`](https://doc.rust-lang.org/reference/expressions/match-expr.html) to decode
> +/// the [`Result`] is similar to C where all the return value decoding and the
> +/// error handling is done explicitly by writing handling code for each
> +/// error to cover. Using [`match`](https://doc.rust-lang.org/reference/expressions/match-expr.html)
> +/// the error and success handling can be implemented in all detail as required.
> +/// For example (inspired by [samples/rust/rust_minimal.rs](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/rust/rust_minimal.rs)):
> +/// ```
> +/// fn example () -> Result {
> +///     let mut numbers = KVec::new();
> +///     match numbers.push(72, GFP_KERNEL) {
> +///         Err(e) => {pr_err!("Error pushing 72: {:?}", e); return Err(e.into());},
> +///         Ok(()) => (), // Do nothing, continue
> +///     }
> +///     match numbers.push(108, GFP_KERNEL){
> +///         Err(e) => {pr_err!("Error pushing 108: {:?}", e); return Err(e.into());},
> +///         Ok(()) => (), // Do nothing, continue
> +///     }
> +///     match numbers.push(200, GFP_KERNEL){
> +///         Err(e) => {pr_err!("Error pushing 200: {:?}", e); return Err(e.into());},
> +///         Ok(()) => (), // Do nothing, continue
> +///     }
> +///     Ok(())
> +/// }
> +/// ```
> +/// Instead of the verbose [`match`](https://doc.rust-lang.org/reference/expressions/match-expr.html)
> +/// the [`?`](https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator)-operator
> +/// or [`unwrap()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap)/
> +/// [`expect()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.expect)
> +/// can be used to handle the [`Result`] "automatically". However, in the kernel
> +/// context, the usage of [`unwrap()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap) or
> +/// [`expect()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.expect) has a side effect which is often
> +/// not wanted: The [`panic`](https://docs.kernel.org/driver-api/basics.html#c.panic) called when using
> +/// [`unwrap()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap) or
> +/// [`expect()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.expect). While the
> +/// console output from [`panic`](https://docs.kernel.org/driver-api/basics.html#c.panic) is
> +/// nice and quite helpful for debugging the error, stopping the whole Linux system due to the kernel
> +/// panic is often **not** desired:
> +/// ```
> +/// fn example () -> Result {
> +///     let mut numbers = KVec::new();
> +///     numbers.push(72, GFP_KERNEL).expect("Error pushing 72"); // Panics the system in case of an error
> +///     numbers.push(108, GFP_KERNEL).expect("Error pushing 108"); // Panics the system in case of an error
> +///     numbers.push(200, GFP_KERNEL).expect("Error pushing 200"); // Panics the system in case of an error
> +///     Ok(())
> +/// }
> +/// ```
> +/// Instead [`unwrap_or()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or),
> +/// [`unwrap_or_else()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else) or
> +/// [`unwrap_or_default()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default)
> +/// can be used. But in consequence, using the [`?`](https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator)-operator
> +/// is often the best choice to handle [`Result`] in a non-verbose way as done in
> +/// [samples/rust/rust_minimal.rs](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/rust/rust_minimal.rs)):
> +/// ```
> +/// fn example () -> Result {
> +///     let mut numbers = KVec::new();
> +///     numbers.push(72, GFP_KERNEL)?;
> +///     numbers.push(108, GFP_KERNEL)?;
> +///     numbers.push(200, GFP_KERNEL)?;
> +///     Ok(())
> +/// }
> +/// ```
>   pub type Result<T = (), E = Error> = core::result::Result<T, E>;
>   
>   /// Converts an integer as returned by a C kernel function to an error if it's negative, and

You are duplicating some links. You can take advantage of positional 
parameters [1] and put the URLs at the end of the comment, which would 
solve the link duplication and IMO increase readability, because 
scattered links in the comments decreases readability a lot (in non HTML 
version).

[1]: 
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html#valid-links

Daniel