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: <Z8jk3qs6nCIJz-39@pollux>
Date: Thu, 6 Mar 2025 00:57:18 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
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 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?

> 
> 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.