Messages in this thread | ![/](/images/icornerl.gif) | | From | Miguel Ojeda <> | Date | Sun, 26 Feb 2023 15:26:50 +0100 | Subject | Re: [PATCH 1/5] rust: error: Add Error::to_ptr() |
| |
On Sat, Feb 25, 2023 at 11:14 PM 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
| ![\](/images/icornerr.gif) |