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

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

Posted by Jimmy Ostler 1 year ago

Add a rustdoc example and Kunit test to the `ArrayLayout` struct's
`ArrayLayout::new()` function.

Add an implementation of `From<LayoutError> for Error` for the
`kernel::alloc::LayoutError`. This is necessary for the new test to
compile.

Change the `From` implementation on `core::alloc::LayoutError` to avoid
collisions with `kernel::alloc::LayoutError`, and modify imports to
explicitly import `kernel::alloc::LayoutError` instead.

Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1131
Signed-off-by: Jimmy Ostler <jtostler1@gmail.com>
---
v1: https://lore.kernel.org/lkml/20241203051843.291729-1-jtostler1@gmail.com/T/#u
v1 -> v2 changes:
- Add third assert where length is smaller but still overflows
- Remove rustdoc markdown codeblock languge signifier
- Change tests to return results using `?` instead of panic
- Remove `#[derive(Debug)]` for `LayoutError`
- Add `From<LayoutError> for Error` implementation
---
 rust/kernel/alloc/layout.rs | 19 +++++++++++++++++++
 rust/kernel/error.rs        | 13 ++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
index 4b3cd7fdc816..0e053dcc7941 100644
--- a/rust/kernel/alloc/layout.rs
+++ b/rust/kernel/alloc/layout.rs
@@ -43,6 +43,25 @@ pub const fn empty() -> Self {
     /// # Errors
     ///
     /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::alloc::layout::{ArrayLayout, LayoutError};
+    /// let layout = ArrayLayout::<i32>::new(15)?;
+    /// assert_eq!(layout.len(), 15);
+    ///
+    /// // Errors because `len * size_of::<T>()` overflows
+    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
+    /// assert!(layout.is_err());
+    ///
+    /// // Errors because `len * size_of::<i32>() > isize::MAX`,
+    /// // even though `len < isize::MAX`
+    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize / 2);
+    /// assert!(layout.is_err());
+    ///
+    /// # Ok::<(), Error>(())
+    /// ```
     pub const fn new(len: usize) -> Result<Self, LayoutError> {
         match len.checked_mul(core::mem::size_of::<T>()) {
             Some(size) if size <= ISIZE_MAX => {
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 52c502432447..ac8526140d7a 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -4,9 +4,10 @@
 //!
 //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
 
-use crate::{alloc::AllocError, str::CStr};
-
-use core::alloc::LayoutError;
+use crate::{
+    alloc::{layout::LayoutError, AllocError},
+    str::CStr,
+};
 
 use core::fmt;
 use core::num::NonZeroI32;
@@ -223,6 +224,12 @@ fn from(_: LayoutError) -> Error {
     }
 }
 
+impl From<core::alloc::LayoutError> for Error {
+    fn from(_: core::alloc::LayoutError) -> Error {
+        code::ENOMEM
+    }
+}
+
 impl From<core::fmt::Error> for Error {
     fn from(_: core::fmt::Error) -> Error {
         code::EINVAL

base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
-- 
2.47.1

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

Posted by Danilo Krummrich 1 year ago

Hi Jimmy,

Thanks for the patch!

On Thu, Dec 05, 2024 at 02:56:27AM -0800, Jimmy Ostler wrote:
> Add a rustdoc example and Kunit test to the `ArrayLayout` struct's
> `ArrayLayout::new()` function.
> 
> Add an implementation of `From<LayoutError> for Error` for the
> `kernel::alloc::LayoutError`. This is necessary for the new test to
> compile.

Please split this into a separate patch.

> 
> Change the `From` implementation on `core::alloc::LayoutError` to avoid
> collisions with `kernel::alloc::LayoutError`, and modify imports to
> explicitly import `kernel::alloc::LayoutError` instead.
> 
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1131
> Signed-off-by: Jimmy Ostler <jtostler1@gmail.com>
> ---
> v1: https://lore.kernel.org/lkml/20241203051843.291729-1-jtostler1@gmail.com/T/#u
> v1 -> v2 changes:
> - Add third assert where length is smaller but still overflows
> - Remove rustdoc markdown codeblock languge signifier
> - Change tests to return results using `?` instead of panic
> - Remove `#[derive(Debug)]` for `LayoutError`
> - Add `From<LayoutError> for Error` implementation
> ---
>  rust/kernel/alloc/layout.rs | 19 +++++++++++++++++++
>  rust/kernel/error.rs        | 13 ++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
> index 4b3cd7fdc816..0e053dcc7941 100644
> --- a/rust/kernel/alloc/layout.rs
> +++ b/rust/kernel/alloc/layout.rs
> @@ -43,6 +43,25 @@ pub const fn empty() -> Self {
>      /// # Errors
>      ///
>      /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::alloc::layout::{ArrayLayout, LayoutError};
> +    /// let layout = ArrayLayout::<i32>::new(15)?;
> +    /// assert_eq!(layout.len(), 15);
> +    ///
> +    /// // Errors because `len * size_of::<T>()` overflows
> +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
> +    /// assert!(layout.is_err());
> +    ///
> +    /// // Errors because `len * size_of::<i32>() > isize::MAX`,
> +    /// // even though `len < isize::MAX`
> +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize / 2);
> +    /// assert!(layout.is_err());
> +    ///
> +    /// # Ok::<(), Error>(())
> +    /// ```
>      pub const fn new(len: usize) -> Result<Self, LayoutError> {
>          match len.checked_mul(core::mem::size_of::<T>()) {
>              Some(size) if size <= ISIZE_MAX => {
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 52c502432447..ac8526140d7a 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -4,9 +4,10 @@
>  //!
>  //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
>  
> -use crate::{alloc::AllocError, str::CStr};
> -
> -use core::alloc::LayoutError;
> +use crate::{
> +    alloc::{layout::LayoutError, AllocError},
> +    str::CStr,
> +};

I think this part of the change would be enough, since we don't make use of the
`From` implementation of `core::alloc::LayoutError` anywhere.

I think we can add it (again), once it's needed.

>  
>  use core::fmt;
>  use core::num::NonZeroI32;
> @@ -223,6 +224,12 @@ fn from(_: LayoutError) -> Error {
>      }
>  }
>  
> +impl From<core::alloc::LayoutError> for Error {
> +    fn from(_: core::alloc::LayoutError) -> Error {
> +        code::ENOMEM
> +    }
> +}
> +
>  impl From<core::fmt::Error> for Error {
>      fn from(_: core::fmt::Error) -> Error {
>          code::EINVAL
> 
> base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
> -- 
> 2.47.1
>

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

Posted by Jimmy Ostler 1 year ago

On Thu, Dec 5, 2024 at 3:35 AM Danilo Krummrich <dakr@kernel.org> wrote:

> Hi Jimmy,
>
> Thanks for the patch!
>
> On Thu, Dec 05, 2024 at 02:56:27AM -0800, Jimmy Ostler wrote:
> > Add a rustdoc example and Kunit test to the `ArrayLayout` struct's
> > `ArrayLayout::new()` function.
> >
> > Add an implementation of `From<LayoutError> for Error` for the
> > `kernel::alloc::LayoutError`. This is necessary for the new test to
> > compile.
>
> Please split this into a separate patch.

Got it, the next version will be split into separate patches.

> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index 52c502432447..ac8526140d7a 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -4,9 +4,10 @@
> >  //!
> >  //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
> > 
> > -use crate::{alloc::AllocError, str::CStr};
> > -
> > -use core::alloc::LayoutError;
> > +use crate::{
> > +    alloc::{layout::LayoutError, AllocError},
> > +    str::CStr,
> > +};
>
> I think this part of the change would be enough, since we don't make use of the
> `From` implementation of `core::alloc::LayoutError` anywhere.
>
> I think we can add it (again), once it's needed.

Okay, that makes sense. It is still used in the documentation for the
macro `stack_try_pin_init`, and it is hidden and not used as a test, but
it would probably be prudent to change that for consistency, as
`Box::new` no longer returns `core::alloc::AllocError`.
I can add that into the v3 patchset, unless there's some reason we
should leave it.

> > 
> >  use core::fmt;
> >  use core::num::NonZeroI32;
> > @@ -223,6 +224,12 @@ fn from(_: LayoutError) -> Error {
> >      }
> >  }
> > 
> > +impl From<core::alloc::LayoutError> for Error {
> > +    fn from(_: core::alloc::LayoutError) -> Error {
> > +        code::ENOMEM
> > +    }
> > +}
> > +
> >  impl From<core::fmt::Error> for Error {
> >      fn from(_: core::fmt::Error) -> Error {
> >          code::EINVAL
> >
> > base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
> > --
> > 2.47.1
> >

Thanks!

Jimmy Ostler

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

Posted by Danilo Krummrich 1 year ago

On Thu, Dec 05, 2024 at 03:04:28PM -0800, Jimmy Ostler wrote:
> On Thu, Dec 5, 2024 at 3:35 AM Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > Hi Jimmy,
> >
> > Thanks for the patch!
> >
> > On Thu, Dec 05, 2024 at 02:56:27AM -0800, Jimmy Ostler wrote:
> > > Add a rustdoc example and Kunit test to the `ArrayLayout` struct's
> > > `ArrayLayout::new()` function.
> > >
> > > Add an implementation of `From<LayoutError> for Error` for the
> > > `kernel::alloc::LayoutError`. This is necessary for the new test to
> > > compile.
> >
> > Please split this into a separate patch.
> 
> Got it, the next version will be split into separate patches.
> 
> > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > index 52c502432447..ac8526140d7a 100644
> > > --- a/rust/kernel/error.rs
> > > +++ b/rust/kernel/error.rs
> > > @@ -4,9 +4,10 @@
> > >  //!
> > >  //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
> > > 
> > > -use crate::{alloc::AllocError, str::CStr};
> > > -
> > > -use core::alloc::LayoutError;
> > > +use crate::{
> > > +    alloc::{layout::LayoutError, AllocError},
> > > +    str::CStr,
> > > +};
> >
> > I think this part of the change would be enough, since we don't make use of the
> > `From` implementation of `core::alloc::LayoutError` anywhere.
> >
> > I think we can add it (again), once it's needed.
> 
> Okay, that makes sense. It is still used in the documentation for the
> macro `stack_try_pin_init`, and it is hidden and not used as a test, but
> it would probably be prudent to change that for consistency, as
> `Box::new` no longer returns `core::alloc::AllocError`.

It seems you're confusing `LayoutError` and `AllocError` here.

This is about the former. But you're right that `AllocError` can be fixed up in
a few places too.

Do you plan to send a patch for this as well?

> I can add that into the v3 patchset, unless there's some reason we
> should leave it.
> 
> > > 
> > >  use core::fmt;
> > >  use core::num::NonZeroI32;
> > > @@ -223,6 +224,12 @@ fn from(_: LayoutError) -> Error {
> > >      }
> > >  }
> > > 
> > > +impl From<core::alloc::LayoutError> for Error {
> > > +    fn from(_: core::alloc::LayoutError) -> Error {
> > > +        code::ENOMEM
> > > +    }
> > > +}
> > > +
> > >  impl From<core::fmt::Error> for Error {
> > >      fn from(_: core::fmt::Error) -> Error {
> > >          code::EINVAL
> > >
> > > base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
> > > --
> > > 2.47.1
> > >
> 
> Thanks!
> 
> Jimmy Ostler

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

Posted by Jimmy Ostler 1 year ago

On Fri, Dec 6, 2024 at 2:57 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu, Dec 05, 2024 at 03:04:28PM -0800, Jimmy Ostler wrote:
> > On Thu, Dec 5, 2024 at 3:35 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > > Hi Jimmy,
> > >
> > > Thanks for the patch!
> > >
> > > On Thu, Dec 05, 2024 at 02:56:27AM -0800, Jimmy Ostler wrote:
> > > > Add a rustdoc example and Kunit test to the `ArrayLayout` struct's
> > > > `ArrayLayout::new()` function.
> > > >
> > > > Add an implementation of `From<LayoutError> for Error` for the
> > > > `kernel::alloc::LayoutError`. This is necessary for the new test to
> > > > compile.
> > >
> > > Please split this into a separate patch.
> >
> > Got it, the next version will be split into separate patches.
> >
> > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > index 52c502432447..ac8526140d7a 100644
> > > > --- a/rust/kernel/error.rs
> > > > +++ b/rust/kernel/error.rs
> > > > @@ -4,9 +4,10 @@
> > > >  //!
> > > >  //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
> > > >
> > > > -use crate::{alloc::AllocError, str::CStr};
> > > > -
> > > > -use core::alloc::LayoutError;
> > > > +use crate::{
> > > > +    alloc::{layout::LayoutError, AllocError},
> > > > +    str::CStr,
> > > > +};
> > >
> > > I think this part of the change would be enough, since we don't make use of the
> > > `From` implementation of `core::alloc::LayoutError` anywhere.
> > >
> > > I think we can add it (again), once it's needed.
> >
> > Okay, that makes sense. It is still used in the documentation for the
> > macro `stack_try_pin_init`, and it is hidden and not used as a test, but
> > it would probably be prudent to change that for consistency, as
> > `Box::new` no longer returns `core::alloc::AllocError`.
>
> It seems you're confusing `LayoutError` and `AllocError` here.
>
> This is about the former. But you're right that `AllocError` can be fixed up in
> a few places too.

Oops, you're totally right, I noticed it when grepping through error 
types in documentation, and mixed them up.

> Do you plan to send a patch for this as well?

Yes, I've already got a fix for that locally.

> > I can add that into the v3 patchset, unless there's some reason we
> > should leave it.
> >
> > > >
> > > >  use core::fmt;
> > > >  use core::num::NonZeroI32;
> > > > @@ -223,6 +224,12 @@ fn from(_: LayoutError) -> Error {
> > > >      }
> > > >  }
> > > >
> > > > +impl From<core::alloc::LayoutError> for Error {
> > > > +    fn from(_: core::alloc::LayoutError) -> Error {
> > > > +        code::ENOMEM
> > > > +    }
> > > > +}
> > > > +
> > > >  impl From<core::fmt::Error> for Error {
> > > >      fn from(_: core::fmt::Error) -> Error {
> > > >          code::EINVAL
> > > >
> > > > base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
> > > > --
> > > > 2.47.1
> > > >
> >
> > Thanks!
> >
> > Jimmy Ostler