Re: [PATCH v2 2/8] rust: drm: ioctl: Add DRM ioctl abstraction

[Alyssa Rosenzweig <alyssa@rosenzweig.io>]

+/// `user_callback` should have the following prototype:
+///
+/// ```ignore
+/// fn foo(device: &kernel::drm::Device<Self>,
+///        data: &mut bindings::argument_type,
+///        file: &kernel::drm::File<Self::File>,
+/// )

Needs to be `-> Result<u32>`, please update

> +                            // SAFETY: This is just the ioctl argument, which hopefully has the
> +                            // right type (we've done our best checking the size).

"hopefully" in a SAFETY comment raises eyebrows!

The argument has the right type /by definition/ once we know the ioctl
name and the size. If userspace passes the wrong type, that's not our
problem - we're still doing the right cast from the perspective of the
kernel. It's up to the driver to reject bogus values.

Maybe something like

    SAFETY: The ioctl argument has size _IOC_SIZE(cmd), which we
    asserted above matches the size of this type, and all bit patterns of
    UAPI structs must be valid.

This also documents the actual safety invariant we're relying on (that
all bit patterns must be valid... which is "obvious" for correct uapis
but not true for eg repr(Rust)!)

> +                                Ok(i) => i.try_into()
> +                                            .unwrap_or($crate::error::code::ERANGE.to_errno()),

It would be great if we could statically guarantee that the types will
fit to avoid this error path (i.e. static assert that the handler
returns Result<u32> and sizeof(u32) <= sizeof(ffi:c_int)). But I don't
know how to do that in Rust so no action required unless you have a
better idea ;)

---

Anyway, with those two comments updated, this patch is

    Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Thanks!