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

[Benno Lossin <benno.lossin@proton.me>]

On Thu Mar 6, 2025 at 2:29 AM CET, Danilo Krummrich wrote:
> On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote:
>> 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,
>
> It doesn't really matter to the user, since the user never needs to supply it.
> That happens in the module_firmware! macro.
>
> I agree it not good to not mention anything about it at all, but I wouldn't want
> to bother the user with all implemention details.
>
> We can probably just mention that it's used internally and is supplied by
> module_firmware!. (That module_firmware! does that by doing a dry run of the
> builder itself, isn't necessary to know for the user I think.)
>
>> - what `prepare()` does,
>
> Same here, it's an implementation detail not relevant to the user. All the user
> needs to know is that prepare() acts as a separator to be able to supply the
> next firmware path.

How about calling it `new_path`/`new_entry` or similar?

>> - what happens with the `module_name` parameter of `new`
>
> Should probably just mention it's supplied by module_firmware! and used
> internally.

IIUC, that's not the case, the `module_firmware!` macro will call the
`create` function with the name and you're supposed to just pass it onto
the builder.

>> - answer the question "I want that the builder outputs the string `???`
>>   can it do that? If yes, how do I do it?"
>
> All it does is concatenating multiple &str in const context, which I thought is
> clear since there are only push() and prepare() as public methods.
>
> May it be that your request is more about can we add more hints on the
> implementation details rather than user focused documentation?

I am not familiar with MODULE_FIRMWARE in C, and I'd think that someone
that uses this API would know what to put into the `.modinfo` section,
so like "foo\0bar\0\0baz" (no idea if that makes sense, but just add
`firmware` or whatever is needed to make it make sense). And then the
question would be how to translate that into the builder.

I wouldn't be able to piece it together without looking at the
implementation.

---
Cheers,
Benno