Re: [PATCH] rust: alloc: Add doctest for `ArrayLayout`

[Dirk Behme]

On 03/12/2024 09:48, Miguel Ojeda wrote:
> On Tue, Dec 3, 2024 at 6:19 AM <jtostler1@xxxxxxxxx> wrote:
>>
>> From: Jimmy Ostler <jtostler1@xxxxxxxxx>
>>
>> Added a rustdoc example and Kunit test to the `ArrayLayout` struct's
>> `ArrayLayout::new()` function.
>>
>> Kunit tests ran using `./tools/testing/kunit/kunit.py run \
>> --make_options LLVM=1 \
>> --kconfig_add CONFIG_RUST=y` passed.
>>
>> Generated documentation looked as expected.
>>
>> Signed-off-by: Jimmy Ostler <jtostler1@xxxxxxxxx>
>> Suggested-by: Boqun Feng <boqun.feng@xxxxxxxxx>
>> Link: https://github.com/Rust-for-Linux/linux/issues/1131
> 
> Thanks for the patch!
> 
> A few procedural nits: please Cc the maintainers/reviewers, especially
> the main one (Danilo) -- for that, please see
> `scripts/get_maintainer.pl` as well as e.g.
> https://rust-for-linux.com/contributing#submitting-patches for one way
> to generate the arguments.
> 
> The "Signed-off-by" tag normally would be the last one -- that way
> people see that you added the other two rather than the next person in
> the chain. It is good to mention the tests etc. that you have done,
> although normally for a patch like this it would normally not be
> mentioned (since all patches that add an example need to be tested
> anyway).
> 
> Finally, a nit on the commit message: normally they are written in the
> imperative mood.
> 
> By the way, the "From:" tag on the top would not need to be there if
> your "From:" in the email headers is configured properly.
> 
>>  /// Error when constructing an [`ArrayLayout`].
>> +#[derive(Debug)]
>>  pub struct LayoutError;
> 
> Ideally you would mention this change in the commit message too -- it
> is the only non-comment/doc change, after all :) It is also important
> because, in general, so far, we have not been using `expect`.
> 
>> +    ///
>> +    ///
> 
> Please use a single line.
> 
>> +    /// ```rust
> 
> You can remove "rust" since it is the default.
> 
>> +    /// use kernel::alloc::layout::ArrayLayout;
> 
> This line could be hidden -- it is `Self`, after all, so it is not
> adding much for the reader. We are not fully consistent on this yet
> though.
> 
>> +    /// let layout = ArrayLayout::<i32>::new(15);
>> +    /// assert_eq!(layout.expect("len * size_of::<i32>() does not overflow").len(), 15);
> 
> See above on `expect`.
> 
> Moreover, since it is a test, it is fine to panic, but recently we
> were discussing that examples should ideally show how "real code"
> would be written, thus using `?` etc. instead.

Slightly off-topic here, but should we try to document that somehow?

What's about something like [1] below? If it is ok, I can make a proper
patch for it :)

Thanks

Dirk

[1]

diff --git a/Documentation/rust/coding-guidelines.rst
b/Documentation/rust/coding-guidelines.rst
index a2e326b42410f..f09af264fff12 100644
--- a/Documentation/rust/coding-guidelines.rst
+++ b/Documentation/rust/coding-guidelines.rst
@@ -373,3 +373,58 @@ triggered due to non-local changes (such as
``dead_code``).
 For more information about diagnostics in Rust, please see:

 	https://doc.rust-lang.org/stable/reference/attributes/diagnostics.html
+
+Error handling
+--------------
+
+In C, it is common that functions indicate success or failure through
+their return value; modifying or returning extra data through non-``const``
+pointer parameters. In particular, in the kernel, functions that may fail
+typically return an ``int`` that represents a generic error code. In Rust
+this is modeled as ``Error``.
+
+Use Result
+~~~~~~~~~~
+
+In Rust, it is idiomatic to model functions that may fail as returning
+a ``Result``. Since in the kernel many functions return an error code,
+``Result`` is a type alias for a ``core::result::Result`` that uses
+``Error`` as its error type.
+
+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``.
+
+The ``?``-operator versus ``unwrap(``) and ``expect()``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Calling a function that returns ``Result`` needs the caller to handle
+the returned ``Result``.
+
+This can be done "manually" by using  ``match``. Using ``match`` 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`` the error and success handling can be
implemented
+in all detail as required.
+
+Instead of the verbose ``match`` the ``?``-operator or
``unwrap()``/``expect()``
+can be used to handle the ``Result`` "automatically". However, in the
kernel
+context, the usage of ``unwrap()`` or ``expect()`` has a side effect
which is often
+not wanted: The ``panic!`` called when using ``unwrap()`` or
``expect()``. While the
+console output from ``panic!`` is nice and quite helpful for debugging
the error,
+stopping the whole Linux system due to the kernel panic is often not
desired.
+
+In consequence, using the ``?``-operator is often the best choice to handle
+``Result`` in a non-verbose way. For example (inspired by
samples/rust/rust_minimal.rs):
+
+.. code-block:: rust
+
+	use kernel::prelude::*;
+
+	fn example () -> Result {
+	    let mut numbers = Vec::new();
+	    numbers.try_push(72)?;
+	    numbers.try_push(108)?;
+	    numbers.try_push(200)?;
+	    Ok(())
+	}
diff --git a/Documentation/rust/testing.rst b/Documentation/rust/testing.rst
index 568b71b415a45..aed50070c979f 100644
--- a/Documentation/rust/testing.rst
+++ b/Documentation/rust/testing.rst
@@ -123,6 +123,11 @@ A current limitation is that KUnit does not support
assertions in other tasks.
 Thus, we presently simply print an error to the kernel log if an assertion
 actually failed. Additionally, doctests are not run for nonpublic
functions.

+As these example tests might be used as examples for "real code" they
should
+be written like "real code". For example, instead of using
``unwrap()``/``expect()``
+use the ``?``-operator. See Documentation/rust/coding-guidelines.rst
(=> Error handling)
+for some background.
+
 The ``#[test]`` tests
 ---------------------