Message-ID: <D9DI48C2NC3E.1NX1YBNTYAZ7L@proton.me>
Date: Tue, 22 Apr 2025 21:56:11 +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 11:11 PM CEST, Boqun Feng wrote:
> On Tue, Apr 22, 2025 at 02:45:22PM +0000, Benno Lossin wrote:
>> 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
>
> Since there is no user using them so far, I think these examples can
> serve as regression tests, that is, if someone accidentally breaks
> something to make them not working, we will immediately know.

That's fair.

>> 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 :)
>> 
>
> Maybe you can open issues and see if others could help?

Done: https://github.com/Rust-for-Linux/pin-init/issues/47

>> > 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
>
> I think this is fine and matches my previous understanding. I just
> wanted to be clear that normally if an example/test is requested for a
> patch from a reviewer, the usual response is not "hey, why don't you
> contribute one?" Of course the request has to been reasonble. In other
> words, we are doing a special workflow here.

Maybe I should have also initially stated that I'm a bit short on time.
For the visibility thing, it felt to me a bit like can you please add an
example for the function `fn add(a: u8, b: u8) -> u8 { a + b }`. ie too
trivial. I'll add more tests when I have time :)

---
Cheers,
Benno

Powered by blists - more mailing lists