Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust/kernel: move from_kernel_result! macro to error.rs #266

Merged
merged 3 commits into from
May 19, 2021

Conversation

TheSven73
Copy link
Collaborator

@TheSven73 TheSven73 commented May 12, 2021

This macro is well-suited for use elsewhere in the Rust core.
Move it out to a suitable place, error.rs.

(I'd like to use this in #254)

v1 -> v2

  • split up in logically consistent commits (ojeda)
  • add commit to improve safety and eliminate possible panics (TheSven73)

rust/kernel/error.rs Outdated Show resolved Hide resolved
rust/kernel/error.rs Show resolved Hide resolved
}
}

/// Transforms a `crate::error::Result<T>` to a kernel `C` integer result.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C is not code, i.e.:

Suggested change
/// Transforms a `crate::error::Result<T>` to a kernel `C` integer result.
/// Transforms a `crate::error::Result<T>` to a kernel C integer result.

Does an intra-doc link work here?

Suggested change
/// Transforms a `crate::error::Result<T>` to a kernel `C` integer result.
/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.


/// Transforms a `crate::error::Result<T>` to a kernel `C` integer result.
///
/// This is useful when calling Rust functions that return `Result<T>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intra-doc link perhaps?

rust/kernel/error.rs Show resolved Hide resolved
Sven Van Asbroeck added 2 commits May 12, 2021 16:11
This macro is well-suited for use elsewhere in the Rust core.
Move it out to a suitable place, error.rs.

Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
This is required to pass the CI.

Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
@TheSven73 TheSven73 force-pushed the rust-for-linux-from-kernel-result branch 4 times, most recently from 5b4b7a4 to 3c009a7 Compare May 13, 2021 00:36
@@ -106,15 +106,20 @@ impl From<AllocError> for Error {
}
}

// # Invariant: `-bindings::MAX_ERRNO` fits in an `i16`.
crate::static_assert!(bindings::MAX_ERRNO <= -(i16::MIN as i32) as u32);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the nagging feeling that this method is too contrived. Is there an idiomatic way to verify whether a value fits in a type? I tried Try or TryFrom but that doesn't work, because those types can't run inside a static_assert!.

Copy link
Member

@ojeda ojeda May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have taken a look at this -- it is indeed a bit painful. After having tried several approaches, I am preparing a PR with a fun one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am grabbing the popcorn !! :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See if you like #269 :)

It would allow you to do:

static_assert!((-(bindings::MAX_ERRNO as i128)) fits in i16);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it a lot !! Will definitely rebase to that after it gets merged !! 💯

`from_kernel_result!` could panic if the return integer type (`T`) is
unable to hold the negative `errno`. Since `errno`s range from
`1` to `4095`, functions returning integer types unable to hold
values in the [-4095, -1] range could potentially panic.

This includes all unsigned integers, and signed integers with
insufficient bits, such as `c_char`.

Fix by making sure that the return integer type is always suitable
to hold the negative `errno`. Use the Rust type system to verify this
at build time.

Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
@TheSven73 TheSven73 force-pushed the rust-for-linux-from-kernel-result branch from 3c009a7 to eb4c6e8 Compare May 18, 2021 12:57
@TheSven73
Copy link
Collaborator Author

Corrected minor nit in commit message to restart CI (which failed due to inability to download packages).

@ksquirrel
Copy link
Member

Review of eb4c6e8f9a75:

  • ✔️ Commit 7dbb95f: Looks fine!
  • ✔️ Commit 2ce3372: Looks fine!
  • ✔️ Commit eb4c6e8: Looks fine!

@alex
Copy link
Member

alex commented May 19, 2021

Going to merge this now, and we can improve the static_assert in a follow-up PR.

@alex alex merged commit fb672e0 into Rust-for-Linux:rust May 19, 2021
@TheSven73 TheSven73 deleted the rust-for-linux-from-kernel-result branch May 19, 2021 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants