Received-SPF: pass (google.com: domain of linux-kernel+bounces-199753-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240603211538.289765-1-andriy.shevchenko@linux.intel.com>
In-Reply-To: <20240603211538.289765-1-andriy.shevchenko@linux.intel.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 3 Jun 2024 15:06:00 -0700
Message-ID: <CAHk-=wg5F99-GZPETsasJd0JB0JGcdmmPeHRxCtT4_i83h8avg@mail.gmail.com>
Subject: Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Bjorn Helgaas <helgaas@kernel.org>
Content-Type: text/plain; charset="UTF-8"

On Mon, 3 Jun 2024 at 14:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Make two APIs look similar. Hence convert match_string() to be
> a 2-argument macro. In order to avoid unneeded churn, convert
> all users as well. There is no functional change intended.

No.

First off, please don't cc hundreds of people. It's just annoying.
That's what mailing lists are for. And no, that doesn't mean "add
every mailing list". If you can't find a specific one, use lkml.

Secondly, we're not going to encourage people to use some double
underscore version of a function just because they don't have an
explicitly sized array. The existing users of "match_string()" are
fine, and changing them to use a new - and *bad* - name for the same
thing is crazy.

IOW, if we have two names for these things, where one is for a "fixed
array with a fixed size", then it's the *new* use that needs to have a
new name that actually describes that. Not the old use that gets
renamed to use a worse name.

Thirdly, for something like this, "no functional change intended" is
not sufficient. It needs to show that it really is the same, because
I'm not willing to take some patch that touches 50+ files for some
syntactic cleanup with the _intention_ of not changing anything.

In other words, what would fix all these issues - apart from the crazy
cc list - is to use a coccinelle to make a provably identical
transformation, ie thave

   #define match_string_array(a,b) match_string(a ARRAY_SIZE(a), b)

and have the coccinelle scrtipt that does the obvious identity
transformation from

    match_string(xyzzy, ARRAY_SIZE(xyzzy), name);

into

    match_string_array(xyzzy, name);

and not make other places uglier and worse.

And then you can separately take the ones that you have to *think*
about and that aren't locally obvious, and fix them individually, ie

-       ret = match_string(vmpressure_str_levels, VMPRESSURE_NUM_LEVELS, token);
+       ret = match_string_array(vmpressure_str_levels, token);

but where it's important to somehow verify that yes,
VMPRESSURE_NUM_LEVELS is indeed ARRAY_SIZE(vmpressure_str_levels).

Because your patch also has

-               idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);
+               idx = __match_string(hash_algo_name, HASH_ALGO__LAST, token);

and I have *NO* idea why you did that completely garbage
transformation. Because we have

   const char *const hash_algo_name[HASH_ALGO__LAST] = {

so as far as I can see, that one should have used the "automatic array
size" model too.

And these kinds of examples are *exactly* why I refuse to apply this
patch. The patch not only makes several places uglier, it *also* is
incomprehensible in which ones it converts and why.

So a big NAK on this, and it very much needs to be done differently.

             Linus