[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANiq72kOb9mbh4HQzH40Ey+Rax3vREsd0Nf2O0apjDpsboE6vQ@mail.gmail.com>
Date: Sun, 26 Feb 2023 15:26:50 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: Asahi Lina <lina@...hilina.net>, Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Sven Van Asbroeck <thesven73@...il.com>,
Fox Chen <foxhlchen@...il.com>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH 1/5] rust: error: Add Error::to_ptr()
On Sat, Feb 25, 2023 at 11:14 PM Gary Guo <gary@...yguo.net> 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
Powered by blists - more mailing lists