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

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

[jtostler1]

From: Jimmy Ostler <jtostler1@gmail.com>

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@gmail.com>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1131
---
 rust/kernel/alloc/layout.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
index 4b3cd7fdc816..4265f92f8af0 100644
--- a/rust/kernel/alloc/layout.rs
+++ b/rust/kernel/alloc/layout.rs
@@ -7,6 +7,7 @@
 use core::{alloc::Layout, marker::PhantomData};
 
 /// Error when constructing an [`ArrayLayout`].
+#[derive(Debug)]
 pub struct LayoutError;
 
 /// A layout for an array `[T; n]`.
@@ -43,6 +44,19 @@ pub const fn empty() -> Self {
     /// # Errors
     ///
     /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
+    ///
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::alloc::layout::ArrayLayout;
+    ///
+    /// let layout = ArrayLayout::<i32>::new(15);
+    /// assert_eq!(layout.expect("len * size_of::<i32>() does not overflow").len(), 15);
+    ///
+    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
+    /// assert!(layout.is_err());
+    /// ```
     pub const fn new(len: usize) -> Result<Self, LayoutError> {
         match len.checked_mul(core::mem::size_of::<T>()) {
             Some(size) if size <= ISIZE_MAX => {

base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
-- 
2.47.1

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

[Alice Ryhl]

On Tue, Dec 3, 2024 at 6:19 AM <jtostler1@gmail.com> wrote:
>
> From: Jimmy Ostler <jtostler1@gmail.com>
>
> 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@gmail.com>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1131
> ---
>  rust/kernel/alloc/layout.rs | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
> index 4b3cd7fdc816..4265f92f8af0 100644
> --- a/rust/kernel/alloc/layout.rs
> +++ b/rust/kernel/alloc/layout.rs
> @@ -7,6 +7,7 @@
>  use core::{alloc::Layout, marker::PhantomData};
>
>  /// Error when constructing an [`ArrayLayout`].
> +#[derive(Debug)]
>  pub struct LayoutError;
>
>  /// A layout for an array `[T; n]`.
> @@ -43,6 +44,19 @@ pub const fn empty() -> Self {
>      /// # Errors
>      ///
>      /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
> +    ///
> +    ///
> +    /// # Examples

There should only be one empty line before # Examples, not two.

> +    ///
> +    /// ```rust
> +    /// use kernel::alloc::layout::ArrayLayout;
> +    ///
> +    /// let layout = ArrayLayout::<i32>::new(15);
> +    /// assert_eq!(layout.len(), 15);

I think it's less confusing to move this expect() to the line before.
let layout = ArrayLayout::<i32>::new(15).expect(...);

> +    ///
> +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
> +    /// assert!(layout.is_err());
> +    /// ```
>      pub const fn new(len: usize) -> Result<Self, LayoutError> {
>          match len.checked_mul(core::mem::size_of::<T>()) {
>              Some(size) if size <= ISIZE_MAX => {
>
> base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
> --
> 2.47.1
>
>

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

[Miguel Ojeda]

On Tue, Dec 3, 2024 at 6:19 AM <jtostler1@gmail.com> wrote:
>
> From: Jimmy Ostler <jtostler1@gmail.com>
>
> 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@gmail.com>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> 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.

> +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);

Perhaps we could consider an example with an smaller argument that
still overflows, to drive home the multiplication involved. It could
perhaps be a third one.

Thanks!

Cheers,
Miguel

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

[jtostler1]

On Tue, Dec 3, 2024 at 12:48 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> Thanks for the patch!

You're welcome! Thank you once again for creating these opportunities!

> 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.

Thanks for the pointer, I've got that fixed up now.

> 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.

Looking at the documentation for sending patches it's unclear whether
the commit message for a v2 of a patch should be modified for cases 
like this, or the only changes should be the in patch changelogs,
after the marker line. What would usually be the preferred course of
action in cases like this?

> >  /// 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`.

Same as above about v2 patch messages. Should I add it to commit, or 
include that in patch changelogs?

> > +    ///
> > +    ///
>
> Please use a single line.
>
> > +    /// ```rust
>
> You can remove "rust" since it is the default.

Fixed both of these.

> > +    /// 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.

Done.

> > +    /// 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.

Changing it to use `?`.

>
> > +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
>
> Perhaps we could consider an example with an smaller argument that
> still overflows, to drive home the multiplication involved. It could
> perhaps be a third one.

I agree, I think adding a third where the length is set to
`isize::MAX as usize / 2` illustrates how when `len < isize::MAX`,
the overflow can still occur.

>
> Thanks!
>
> Cheers,
> Miguel

Thanks,
Jimmy

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

[Miguel Ojeda]

On Wed, Dec 4, 2024 at 2:57 AM <jtostler1@gmail.com> wrote:
>
> Looking at the documentation for sending patches it's unclear whether
> the commit message for a v2 of a patch should be modified for cases
> like this, or the only changes should be the in patch changelogs,
> after the marker line. What would usually be the preferred course of
> action in cases like this?
>
> Same as above about v2 patch messages. Should I add it to commit, or
> include that in patch changelogs?

The changelog between series goes outside the commit, and is meant to
help reviewers see what you changed (otherwise they would need to run
e.g. a range diff and visually inspect it). The commits themselves
should describe the changes in that commit, ignoring what happened in
other versions, and it is meant for the future to explain what the
change is and why it was introduced in the tree.

So if you do A in v1 and B in v2, then the commit message of v1 would
explain A, v2 would explain B, and the changelog v1->v2 for reviewers
simply states the A->B change (i.e. briefly) so that reviewers know
what happened.

[ Sometimes it is important to explain in the commit message
nevertheless why an approach like B is better than A, but you would do
that in order to clarify why B was picked, not just because A happened
to appear in a previous version because it was tried first and
discarded. ]

I hope that makes sense!

> I agree, I think adding a third where the length is set to
> `isize::MAX as usize / 2` illustrates how when `len < isize::MAX`,
> the overflow can still occur.

Great, thanks!

Cheers,
Miguel

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@gmail.com> wrote:
>>
>> From: Jimmy Ostler <jtostler1@gmail.com>
>>
>> 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@gmail.com>
>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>> 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
 ---------------------

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

[Miguel Ojeda]

On Wed, Dec 4, 2024 at 12:57 PM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> 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 :)

Sure, please send it! :)

(In general, I think we should avoid repeating "general Rust
knowledge", but as long as there is kernel-specific content, like your
paragraphs below, then it is good to have.)

> +``Error`` as its error type.

I see this first part comes from the `Result` docs we have, right? I
think the rest makes sense in Doc/ as you have it; on the other hand,
we try to avoid duplication. We could perhaps move everything to the
`Result` (or `kernel::error` module) docs, and just link it from Doc/
-- that would give us the ability to easily have intra-doc links on
the methods like `unwrap()` and to test the examples.

(I noticed because I was about to suggest linking/qualifying the type
above, but if we had it in the code docs, we would already get that
for free.)

> +The ``?``-operator versus ``unwrap(``) and ``expect()``
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Calling a function that returns ``Result`` needs the caller to handle
> +the returned ``Result``.

A (kernel-specific) example could help here, even if "abstract", e.g.:

    fn f() -> Result {
        if ... {
            return Err(EINVAL);
        }

        Ok(())
    }

This immediately maps to the usual approach in C code. In fact, you
could add the C equivalent here for comparison.

Perhaps even give another abstract one where kernel C would need
`goto` to cleanup.

> +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.

Another one would be great here too, so that they see the verbosity vs. `?`.

> +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.

Perhaps link to any relevant C side docs here.

Perhaps we could also mention briefly other approaches used sometimes,
e.g. `unwrap_or()` and `unwrap_unchecked()`, ideally linking to the
Rust book or similar.

Thanks!

Cheers,
Miguel