Date: Wed, 11 Oct 2023 11:59:01 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: gregkh@...uxfoundation.org, netdev@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, 
	wedsonaf@...il.com
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

On Mon, Oct 9, 2023 at 5:50 PM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
>
> What feedback? enum stuff? I think that it's a long-term issue.

Not just that. There has been other feedback, and since this message,
we got new reviews too.

But, yes, the `--rustified-enum` is one of those. I am still
uncomfortable with it. It is not a huge deal for a while, and things
will work, and the risk of UB is low. But why do we want to risk it?
The point of using Rust is precisely to avoid this sort of thing.

Why cannot we use one of the alternatives? If we really want to catch,
right now, the "addition of new variant in the C enum" case, cannot we
add a temporary check for that? e.g. it occurs to me we could make
`bindgen` generate the `--rustified-enum` into a temporary file and
compile a fixed `match` somewhere or something like that, for the
purposes of checking. That way we avoid the UB in the actual code.

But the best would be to work on adding to `bindgen` something like
the `--safe-rustified-enum` I suggested (because we already know the
maintainers find the idea reasonable -- thanks Trevor for creating the
issue!), even if only to validate the idea with a prototype.

In short, what is the rush?

> I'm not sure about it. For example, we reviewed the locking issue
> three times. It can't be reviewed only on Rust side. It's mainly about
> how the C side works.

We have never said it has to be reviewed only on the Rust side. In
fact, our instructions for contributing explain very clearly the
opposite:

    https://rust-for-linux.com/contributing#the-rust-subsystem

The instructions also say that the code must be warning-free and so
on, and yet after several iterations and pushing for merging several
times, there are still "surface-level" things like missing `// SAFETY`
comments and `bindings::` in public APIs; which we consider very
important -- we want to get them enforced by the compiler in the
future.

Not only that, when I saw Wedson mentioning yesterday the
`#[must_use]` bit, I wondered how this was even not being noticed by
the compiler.

So I just took the v3 patches and compiled them and, indeed, Clippy gives you:

    error: this function has an empty `#[must_use]` attribute, but
returns a type already marked as `#[must_use]`
    --> rust/kernel/net/phy.rs:547:5
        |
    547 | /     pub fn register(
    548 | |         module: &'static crate::ThisModule,
    549 | |         drivers: &'static [Opaque<bindings::phy_driver>],
    550 | |     ) -> Result<Self> {
        | |_____________________^
        |
        = help: either add some descriptive text or remove the attribute
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#double_must_use
        = note: `-D clippy::double-must-use` implied by `-D clippy::style`

    error: length comparison to zero
    --> rust/kernel/net/phy.rs:551:12
        |
    551 |         if drivers.len() == 0 {
        |            ^^^^^^^^^^^^^^^^^^ help: using `is_empty` is
clearer and more explicit: `drivers.is_empty()`
        |
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
        = note: `-D clippy::len-zero` implied by `-D clippy::style`

    error: methods called `as_*` usually take `self` by reference or
`self` by mutable reference
    --> rust/kernel/net/phy.rs:642:21
        |
    642 |     const fn as_int(self) -> u32 {
        |                     ^^^^
        |
        = help: consider choosing a less ambiguous name
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
        = note: `-D clippy::wrong-self-convention` implied by `-D clippy::style`

And from `rustdoc`:

    error: unresolved link to `module_phy_driver`
    --> rust/kernel/net/phy.rs:408:23
        |
    408 | /// This is used by [`module_phy_driver`] macro to create a
static array of phy_driver`.
        |                       ^^^^^^^^^^^^^^^^^ no item named
`module_phy_driver` in scope
        |
        = note: `macro_rules` named `module_phy_driver` exists in this
crate, but it is not in scope at this link's location
        = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`

    error: unresolved link to `PHY_DEVICE_ID`
    --> rust/kernel/net/phy.rs:494:52
        |
    494 |     /// If not implemented, matching is based on [`PHY_DEVICE_ID`].
        |
^^^^^^^^^^^^^ no item named `PHY_DEVICE_ID` in scope
        |
        = help: to escape `[` and `]` characters, add '\' before them
like `\[` or `\]`

So, no, it is not ready for merge. Yes, those things above are
trivial, but fixing them is also trivial, and after several revisions
it has not been done. And this sort of thing should be done before
even submitting the very first version.

Cheers,
Miguel

Powered by blists - more mailing lists