Message-ID: <D9D8YDFLD98E.D8DZEIIW4EN5@proton.me>
Date: Tue, 22 Apr 2025 14:45:22 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Fiona Behrens <me@...enk.dev>, Alban Kurti <kurti@...icto.ai>, Michael Vetter <jubalh@...oru.org>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/8] rust: pin-init: allow `pub` fields in `derive(Zeroable)`

On Tue Apr 22, 2025 at 4:14 PM CEST, Boqun Feng wrote:
> On Tue, Apr 22, 2025 at 08:30:40AM +0000, Benno Lossin wrote:
>> On Tue Apr 22, 2025 at 6:55 AM CEST, Boqun Feng wrote:
>> > On Mon, Apr 21, 2025 at 10:18:33PM +0000, Benno Lossin wrote:
>> >> Add support for parsing `pub`, `pub(crate)` and `pub(super)` to the
>> >> derive macro `Zeroable`.
>> >> 
>> >> Link: https://github.com/Rust-for-Linux/pin-init/pull/42/commits/e8311e52ca57273e7ed6d099144384971677a0ba
>> >> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
>> >
>> > Kindly request tests/examples for this patch and the following one
>> > (patch #7) ;-)
>> 
>> If you send a patch, I'll take it :)
>> 
>
> First, I'm happy to help improve pin-init, *if* I fully understand the
> changes and have the cycle ;-)
>
> However, here we are at the review process, so I need these examples to
> close the gaps between the implementation and the usage to provide any
> meaningful review. There's no example/test in the commit log, the kernel
> code and (I've checked) the GitHub repo. Although I fully trust you, but
> there is no second source that could help me verify the changes easily.

Maybe this is just a case of me being too familiar with the code, but
the change in this commit and #7 are very trivial. I'm not too sure what
I should use as an example because of this. I could do something like:

    #[derive(Zeroable)]
    pub struct Foo {
        pub a: usize,
        b: u64,
    }

    #[derive(Zeroable)]
    pub union Bar {
        pub a: u64,
        pub b: i64,
    }

But I don't see a lot of value in adding those either as doc-tests or as
examples. Rust users normally expect that derive macros can handle any
kind of visibility for fields (there are exceptions of course, but they
don't apply to `Zeroable`).

The union case is a bit different in that not all derive macros support
them, so I agree that the docs should reflect that better. I can add a
patch when I find the time, as I'm stretched pretty thin (hence I
suggested you submit a patch :)

> In this case, it may be special, because you're in fact syncing an
> external repo with the kernel part, i.e. the development is done, so if
> we trust the external repo and of course, if no obvious error is
> founded during review (from the people who can review), we should merge
> it in. If that's the case, this patchset is more of an "FYI" instead of
> a development process IMO. Is this the case here?

I'm not 100% sure on the workflow for pin-init. Ideally all changes made
to the pin-init repo can be ported 1:1 into the kernel. There are of
course smaller things such as commit references in commit messages that
need to be adjusted. But aside from such smaller administrative things,
the idea with the sync was to only have one singular version. If you
want to spend the time looking at the pin-init PRs then feel free to do
so :)

Since I port the history from the repo and not do one single commit with
"sync with version v... of pin-init", I do think that kernel review can
indeed change things in the upstream repository, but I'm not sure by
which means it should do so. I want to avoid to rewrite history
upstream, so there it has to be a new patch.

---
Cheers,
Benno

Powered by blists - more mailing lists