Danilo Krummrich
2025-Mar-06 01:48 UTC
[PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`
On Thu, Mar 06, 2025 at 01:35:52AM +0000, Benno Lossin wrote:> 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?Sure, new_entry() sounds good!> > >> - 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.Yes, but this part is documented by module_firmware!, which I think is the correct place.> > >> - 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.I believe if you come from the perspective of writing a driver, you reach module_firmware! first and then the subsequent stuff makes sense. But I recognize your feedback and will try to make things a bit more obvious by expanding the example of module_firmware! and expanding a few comments here and there. I also think that s/prepare/new_entry/ will help a lot.