MIME-Version: 1.0
References: <20230224-rust-error-v1-0-f8f9a9a87303@asahilina.net>
 <20230224-rust-error-v1-1-f8f9a9a87303@asahilina.net> <20230225221405.62e989c8.gary@garyguo.net>
In-Reply-To: <20230225221405.62e989c8.gary@garyguo.net>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Sun, 26 Feb 2023 15:26:50 +0100
Message-ID: <CANiq72kOb9mbh4HQzH40Ey+Rax3vREsd0Nf2O0apjDpsboE6vQ@mail.gmail.com>
Subject: Re: [PATCH 1/5] rust: error: Add Error::to_ptr()
To:     Gary Guo <gary@garyguo.net>
Cc:     Asahi Lina <lina@asahilina.net>, Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        Sven Van Asbroeck <thesven73@gmail.com>,
        Fox Chen <foxhlchen@gmail.com>, rust-for-linux@vger.kernel.org,
        linux-kernel@vger.kernel.org, asahi@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Feb 25, 2023 at 11:14=E2=80=AFPM Gary Guo <gary@garyguo.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