Re: [PATCH 1/5] rust: error: Add Error::to_ptr()

[Miguel Ojeda]

On Sat, Feb 25, 2023 at 11:14 PM Gary Guo <gary@xxxxxxxxxxx> wrote:
>
> I know that we already have `IS_ERR` in helpers.c, but having to go
> through FFI and helper functions for something as simple as a cast
> feels awkward to me.
>
> Given that `ERR_PTR`'s C definition is very unlike to change, would it
> be problematic if we just reimplement it in Rust as
>
> ```rust
> fn ERR_PTR(error: core::ffi::c_long) -> *mut core::ffi::c_void {
>     error as _
>     // Or `core::ptr::invalid(error as _)` with strict provenance
> }
> ```
> ?
>
> I personally think it should be fine, but I'll leave the decision to
> Miguel.

On one hand, we have tried to minimize duplication (and, in general,
any changes to the C side) so far where possible, especially
pre-merge, doing it only when needed, e.g. for `const` purposes.

On the other hand, being in the kernel opens up a few possibilities to
consider, and it is true it feels like some of these could get
reimplemented, even if not strictly needed. If we can show a
performance/text size difference on e.g. a non-trivial subsystem or
module, I think we should do it.

If we do it, then I think we should add a note on the C side so that
it is clear there is a duplicated implementation elsewhere, avoiding
future problems. In fact, it would be ideal to do it consistently,
e.g. also for the ioctl ones. Something like:

    /* Rust: reimplemented as `kernel::error::ERR_PTR`. */
    static inline void * __must_check ERR_PTR(long error)
    {
        return (void *) error;
    }

Or perhaps something even smaller.

But I don't want to block the rest of the work on this, which may need
some extra/parallel discussion, so let's keep the helper for the time
being. That way we can also do that change independently and justify
the change showing the difference in performance/text, if any, in the
commit message.

Cheers,
Miguel