On Thu, May 23, 2024 at 11:47 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> On 5/22/24 9:32 PM, Andy Shevchenko wrote:
> > On Wed, May 22, 2024 at 9:40 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >> On 5/22/24 7:19 PM, Andy Shevchenko wrote:
> >>> On Wed, May 22, 2024 at 06:48:07PM +0200, Hans de Goede wrote:
...
> >>>> + /*
> >>>> + * str is part of the static_command_line from init/main.c and poking
> >>>> + * holes in that by writing 0 to it is allowed, as is taking long
> >>>> + * lasting references to it.
> >>>> + */
> >>>> + ts_cmdline_data.acpi_name = strsep(&str, ",");
> >>>> +
> >>>> + for (i = 0; i < MAX_CMDLINE_PROPS; i++) {
> >>>> + name = strsep(&str, ",");
> >>>> + if (!name)
> >>>> + break;
> >>>> +
> >>>> + /* Replace '=' with 0 and make value point past '=' or NULL */
> >>>> + value = name;
> >>>> + strsep(&value, "=");
> >>>> + if (!value) {
> >>>> + ts_cmdline_props[i] = PROPERTY_ENTRY_BOOL(name);
> >>>> + } else if (isdigit(value[0])) {
> >>>> + ret = kstrtou32(value, 10, &u32val);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + ts_cmdline_props[i] = PROPERTY_ENTRY_U32(name, u32val);
> >>>> + } else {
> >>>> + ts_cmdline_props[i] = PROPERTY_ENTRY_STRING(name, value);
> >>>> + }
> >>>> + }
> >>>
> >>> This reminds me a lot from the next_arg(), can we not reinvent a wheel?
> >>
> >> next_arg is meant for parsing different arguments on the kernel cmdline
> >> split by spaces. It has space as separator hardcoded so it cannot be
> >> used here.
> >
> > I believe it's not the first time I hear such an excuse for
> > duplicating Yet Another (Same) Parser.
> > If you think you really need another separator, we may patch
> > next_arg() or add next_arg_any(is_separator_fn *fn) and make
> > next_arg() to be a wrapper of the other one.
>
> The kernel already has a generic parser for most things in the form of
> include/linux/parser.h but that will not work here since that assumes
> a list of fixed keywords while in this case I want to allow any keyword
> and change it into a device-property with that name.
I know about that and that's why I haven't suggested it.
> Also the actual splitting into key=value code here is maybe 5 lines,
> the whole patch itself is not that big and most of the parsing is
> figuring out if value represents a bool, uint or string.
Yeah, 5 strings here, 5 bugs there and duplicating all over in the
kernel in zillions copies. This is not good. We should not duplicate
things, we should deduplicate them. And as I said, your excuse is
being repeated not the first time. This is also not good.
> And the kind of refactoring of next_arg() you are asking for here
> is way out of scope,
Of course, of course, but why introduce Yet Another Parser to begin with?
> so sorry but I don't plan to change this part
> of the patch.
This is not good.
But I have no power to stop it, while being very sad about this attitude.
--
With Best Regards,
Andy Shevchenko
