linux-kernel - Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <D88R7HI1Z6GG.ZOQ9A1VQOR28@proton.me>
Date: Thu, 06 Mar 2025 00:24:21 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>
Cc: airlied@...il.com, simona@...ll.ch, corbet@....net, maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, ajanulgu@...hat.com, lyude@...hat.com, pstanner@...hat.com, zhiw@...dia.com, cjia@...dia.com, jhubbard@...dia.com, bskeggs@...dia.com, acurrid@...dia.com, ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu, gregkh@...uxfoundation.org, mcgrof@...nel.org, russ.weight@...ux.dev, dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
> On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
>> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
>> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
>> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>> >> > +    /// Push an additional path component.
>> >> > +    ///
>> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
>> >> > +    /// be called before adding path components.
>> >> > +    pub const fn push(self, s: &str) -> Self {
>> >> > +        if N != 0 && self.n == 0 {
>> >> > +            crate::build_error!("Must call prepare() before push().");
>> >>
>> >> This will only prevent the first `prepare` call being missed, right?
>> >
>> > Correct, unfortunately there's no way to detect subsequent ones.
>>
>> Does it make sense to do that one in the constructor?
>>
>> (After looking at the example below) Ah maybe you can't do that, since
>> then you would have two `prepare()` calls for the example below...?
>
> Exactly.
>
>> >> If you always have to call this before `push`, why not inline it there?
>> >
>> > You can push() multiple times to compose the firmware path string (which is the
>> > whole purpose :).
>>
>> Ah I see, I only looked at the example you have in the next patch. All
>> in all, I think this patch could use some better documentation, since I
>> had to read a lot of the code to understand what everything is supposed
>> to do...
>
> I can expand the example in module_firmware! to make things a bit more obvious.
>
> Otherwise, what information do you think is missing?

Abstractly: what `ModInfoBuilder` *does*, concretely:
- why the generic constant `N` exists,
- what `prepare()` does,
- what happens with the `module_name` parameter of `new`
- answer the question "I want that the builder outputs the string `???`
  can it do that? If yes, how do I do it?"

>> It might also make sense to make this more generic, since one probably
>> also needs this in other places? Or do you think this will only be
>> required for modinfo?
>
> Currently, I don't think there's any more need for a generic const string
> builder. For now, I think we're good. Let's factor it out, once we have actual
> need for that.

Sounds good.

---
Cheers,
Benno