rust/error: add helper function to convert a C error pointer to a Result

[TheSven73]

Some kernel C API functions return a pointer which embeds an optional
errno. Callers are supposed to check the returned pointer with
IS_ERR() and if this returns true, retrieve the errno using
PTR_ERR().

Create a Rust helper function to implement the Rust equivalent:
transform a *mut T to Result<*mut T>.

Co-Created-By: Boqun Feng boqun.feng@gmail.com
Co-Created-By: Miguel Ojeda ojeda@users.noreply.github.com
Signed-off-by: Sven Van Asbroeck thesven73@gmail.com

v1 -> v2

fbq:

• change function name
• make unsafe sections as small as possible

TheSven73:

• document safety guarantees

[fbq]

I think we don't need // SAFETY: ... comments for safe code

[TheSven73]

Good point. Do we need some sort of marker to explain why casts are valid / won't overflow? @ojeda suggested // NOPANIC for .unwrap()s before. Something along the lines of // NO-OVERFLOW: ?

@ojeda what's your opinion?

[ojeda]

Hmm, good point. I am not sure how noisy it will be (in particular inside rust/kernel/), but it does not hurt to try. After all, it is an operation that usually requires a bit of thinking, and many times there are better ways to do it. Having to write the comment may give people pause and search for alternatives.

For instance, here we could use cast() instead, and then one would not need to write a explicit comment.

In any case, I would call it NOOVERFLOW (we are following the TODO/FIXME etc. in that they do not have spaces/dashes/etc.). But it does look weird and long. Perhaps CAST is enough? i.e. "the justification for a cast".

However, if we go for CAST, should the justification cover more than just overflow? e.g. a pointer cast that changes to a completely unrelated type should be well-documented. But then again, someone using cast() should also justify it, and calling cast() should not be a way to avoid such a comment...

[fbq]

Do we over-engineer on this? I know we want to have our code documented as good as possible. But we already have the restriction that raw pointer dererference is unsafe, so if anyone wants to cast pointer in a weird way, he/she must explain himself/herself at the point of the deref, if the pointer never gets deref, then we are safe.

In this case, the err pointer is actually from C side, and we pass it to a C function, so the safety actually replies on the C side, and we cannot do much about it on the Rust side. And I think this might be the typical case where we use raw pointers, so we might want to rethink about the bar of documenting these.

[TheSven73]

However, if we go for CAST, should the justification cover more than just overflow? e.g. a pointer cast that changes to a completely unrelated type should be well-documented. But then again, someone using cast() should also justify it, and calling cast() should not be a way to avoid such a comment...

I agree. From what I can tell playing with cast() in the Rust playground, that function still allows bad casts, i.e. between unrelated types. It will however catch *const to *mut casts (something that happened in the SPI PR). So it's only a partial solution.

Which means that any use of as or cast() still requires a // CAST: annotation - does that sound right?

It's a real pity we don't have a void_cast() function. Because casting to void is always ok so the function would not need an explicit comment.

[TheSven73]

Also... are we mixing up two completely unrelated concepts here, just because C/Rust pointer uses the word "cast" for both of them?

In my mind, a cast between two numeric or primitive types is conceptually very different from a cast between two pointer types.

[ojeda]

So let me try to split the cases:

• For safety, I agree there is no need. It should go in the proper place (i.e. the point of deref).
• For integer arithmetic overflow (not casts), it would be too verbose and we anyway have -Coverflow-checks enabled by default.
• For pointer casts, I agree it may be too annoying, which is why I was suggesting using cast() for those.
  • Nevertheless, people casting pointer types to wildly different ones should explain it anyhow (not for safety, but to understand what is going on).
  • It is anyway hard to enforce: we would need Clippy to support non-doc comments (same problem as for SAFETY ones), and we cannot use a simple script like for SAFETY in the stdlib because we need to know about the types. What we can do, though, is always request to use cast() for pointer-to-pointer conversions, and then enforce that with a Clippy lint.
• For numeric casts "overflow" (e.g. i32 as i16, what I think @TheSven73 was thinking about), AFAIK there is no compiler option to check if truncation occurred because using as is explicitly asking for truncation and is not considered an error. However, given that in many cases we could be using From/Into and/or TryFrom/TryInto, the idea is that we would hope the rest of the cases would be few enough to make the comment worth it, explaining whether truncation is expected to happen or it is "just" to match the type (without truncation expected).
  • Or, better, we provide explicit "cast-with-expected-truncation" and "cast-without-expected-truncation" functions for numeric types, and then ask people to always use those, instead of using as + a comment. We can then have a Clippy lint in this case, because we can flag any as usage for numeric casts (that may truncate).

[TheSven73]

v2 -> v3

fbq:

• improve // SAFETY: annotations

ojeda:

• use // CAST: annotations and cast() function

TheSven73:

• remove # Safety constraint on the function

[ksquirrel]

Review of 62d3b5841ee7:

• ❌ Commit 62d3b58: Private GitHub email address (@users.noreply.github.com) used in commit message.
• ❌ Commit 62d3b58: Unknown tag. Known ones are: Acked-by, Cc, Co-developed-by, Fixes, Link, Reported-by, Reviewed-by, Signed-off-by, Suggested-by, Tested-by.
• ❌ Commit 62d3b58: Unknown tag. Known ones are: Acked-by, Cc, Co-developed-by, Fixes, Link, Reported-by, Reviewed-by, Signed-off-by, Suggested-by, Tested-by.

[alex]

Looks like there's a merge conflict here now.

[ksquirrel]

Review of eb4fba6cacba:

• ❌ Commit eb4fba6: Private GitHub email address (@users.noreply.github.com) used in commit message.
• ❌ Commit eb4fba6: Unknown tag. Known ones are: Acked-by, Cc, Co-developed-by, Fixes, Link, Reported-by, Reviewed-by, Signed-off-by, Suggested-by, Tested-by.
• ❌ Commit eb4fba6: Unknown tag. Known ones are: Acked-by, Cc, Co-developed-by, Fixes, Link, Reported-by, Reviewed-by, Signed-off-by, Suggested-by, Tested-by.

[ksquirrel]

Review of 74fbaf559657:

• ❌ Commit 74fbaf5: Co-developed-by must be followed by a matching Signed-off-by.
• ❌ Commit 74fbaf5: Co-developed-by must be followed by a matching Signed-off-by.

[ksquirrel]

Review of 45ec62c07623:

• ❌ Commit 45ec62c: The last line of the commit message must be a Signed-off-by and must match the submitter of the PR.
• ❌ Commit 45ec62c: Co-developed-by must be followed by a matching Signed-off-by.
• ❌ Commit 45ec62c: Co-developed-by must be followed by a matching Signed-off-by.

[ksquirrel]

Review of 1ed16fe5e2b9:

• ✔️ Commit 1ed16fe: Looks fine!

[TheSven73]

Rebased to fix merge conflict. Improved commit message to satisfy ksquirrel. Sorry for the noise.

[ojeda]

Thanks for playing with our new bot! :)

(I hope it was not too annoying)