2021-04-12 11:35:23

by Gioh Kim

[permalink] [raw]
Subject: [PATCH] lib/string: sysfs_streq works case insensitively

As the name shows, it checks the strings inputed from sysfs.
It should work for both case-sensitive filesystem and
case-insensitive filesystem. Therefore sysfs_streq should work
case-insensitively.

Signed-off-by: Gioh Kim <[email protected]>
---
lib/string.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index 7548eb715ddb..d0914dffdaae 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -688,7 +688,8 @@ EXPORT_SYMBOL(strsep);
#endif

/**
- * sysfs_streq - return true if strings are equal, modulo trailing newline
+ * sysfs_streq - return true if strings are equal case-insentively,
+ * modulo trailing newline
* @s1: one string
* @s2: another string
*
@@ -696,10 +697,11 @@ EXPORT_SYMBOL(strsep);
* NUL and newline-then-NUL as equivalent string terminations. It's
* geared for use with sysfs input strings, which generally terminate
* with newlines but are compared against values without newlines.
+ * And case does not matter for the sysfs input strings comparison.
*/
bool sysfs_streq(const char *s1, const char *s2)
{
- while (*s1 && *s1 == *s2) {
+ while (*s1 && tolower(*s1) == tolower(*s2)) {
s1++;
s2++;
}
--
2.25.1


2021-04-28 05:43:50

by Gioh Kim

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On Mon, Apr 12, 2021 at 1:33 PM Gioh Kim <[email protected]> wrote:
>
> As the name shows, it checks the strings inputed from sysfs.
> It should work for both case-sensitive filesystem and
> case-insensitive filesystem. Therefore sysfs_streq should work
> case-insensitively.
>
> Signed-off-by: Gioh Kim <[email protected]>
> ---
> lib/string.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/string.c b/lib/string.c
> index 7548eb715ddb..d0914dffdaae 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -688,7 +688,8 @@ EXPORT_SYMBOL(strsep);
> #endif
>
> /**
> - * sysfs_streq - return true if strings are equal, modulo trailing newline
> + * sysfs_streq - return true if strings are equal case-insentively,
> + * modulo trailing newline
> * @s1: one string
> * @s2: another string
> *
> @@ -696,10 +697,11 @@ EXPORT_SYMBOL(strsep);
> * NUL and newline-then-NUL as equivalent string terminations. It's
> * geared for use with sysfs input strings, which generally terminate
> * with newlines but are compared against values without newlines.
> + * And case does not matter for the sysfs input strings comparison.
> */
> bool sysfs_streq(const char *s1, const char *s2)
> {
> - while (*s1 && *s1 == *s2) {
> + while (*s1 && tolower(*s1) == tolower(*s2)) {
> s1++;
> s2++;
> }
> --
> 2.25.1
>

Hi Andrew,

I changed sysfs_streq to be case-insensitive as you requested.
Is there any progress?

2021-04-28 06:41:21

by Rasmus Villemoes

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On 12/04/2021 13.33, Gioh Kim wrote:
> As the name shows, it checks the strings inputed from sysfs.
> It should work for both case-sensitive filesystem and
> case-insensitive filesystem. Therefore sysfs_streq should work
> case-insensitively.

What in the whole wide world does case-sensitivity or lack thereof of
filesystems have to do with whether or not the sysfs_streq API should
work case sensitively or not?

The sysfs filesystem itself (1) works like any real UNIX file system
(i.e. case sensitively) and (2) sysfs_streq is not in any way involved
in readdir or lookups or other such filename operations in sysfs.

Very confused,
Rasmus

2021-04-28 07:34:58

by Gioh Kim

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
<[email protected]> wrote:
>
>
>
> On Wednesday, April 28, 2021, Gioh Kim <[email protected]> wrote:
>>
>> On Mon, Apr 12, 2021 at 1:33 PM Gioh Kim <[email protected]> wrote:
>> >
>> > As the name shows, it checks the strings inputed from sysfs.
>> > It should work for both case-sensitive filesystem and
>> > case-insensitive filesystem. Therefore sysfs_streq should work
>> > case-insensitively.
>> >
>> > Signed-off-by: Gioh Kim <[email protected]>
>> > ---
>> > lib/string.c | 6 ++++--
>> > 1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/lib/string.c b/lib/string.c
>> > index 7548eb715ddb..d0914dffdaae 100644
>> > --- a/lib/string.c
>> > +++ b/lib/string.c
>> > @@ -688,7 +688,8 @@ EXPORT_SYMBOL(strsep);
>> > #endif
>> >
>> > /**
>> > - * sysfs_streq - return true if strings are equal, modulo trailing newline
>> > + * sysfs_streq - return true if strings are equal case-insentively,
>> > + * modulo trailing newline
>> > * @s1: one string
>> > * @s2: another string
>> > *
>> > @@ -696,10 +697,11 @@ EXPORT_SYMBOL(strsep);
>> > * NUL and newline-then-NUL as equivalent string terminations. It's
>> > * geared for use with sysfs input strings, which generally terminate
>> > * with newlines but are compared against values without newlines.
>> > + * And case does not matter for the sysfs input strings comparison.
>> > */
>> > bool sysfs_streq(const char *s1, const char *s2)
>> > {
>> > - while (*s1 && *s1 == *s2) {
>> > + while (*s1 && tolower(*s1) == tolower(*s2)) {
>> > s1++;
>> > s2++;
>> > }
>> > --
>> > 2.25.1
>> >
>>
>> Hi Andrew,
>>
>
> Are you sure it’s good change? Sysfs is used for an ABI and you are opening a can of worms. From me NAK to this change without a very good background description that tells why it is safe to do.

https://www.spinics.net/lists/kernel/msg3898123.html
My initial idea was making a new function: sysfs_streqcase.
Andrew and Greg suggested making sysfs_streq to be case-insensitive.
I would like to have a discussion about it.

>
>
>>
>> I changed sysfs_streq to be case-insensitive as you requested.
>> Is there any progress?
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

2021-04-28 07:49:14

by Rasmus Villemoes

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On 28/04/2021 09.31, Gioh Kim wrote:
> On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
> <[email protected]> wrote:
>>

>>
>> Are you sure it’s good change? Sysfs is used for an ABI and you are opening a can of worms. From me NAK to this change without a very good background description that tells why it is safe to do.
>
> https://www.spinics.net/lists/kernel/msg3898123.html
> My initial idea was making a new function: sysfs_streqcase.
> Andrew and Greg suggested making sysfs_streq to be case-insensitive.
> I would like to have a discussion about it.

1. That information should be in the commit log, not some random
babbling about case sensitivity of file systems.

2. So as Andy says, this is changing ABI for a whole lot of users in one
go. While it's _probably_ true that nobody would care (because it just
ends up accepting more strings, not fewer), your motivation seems to be
to replace uses of strncasecmp() to prevent "[email protected]#[email protected]#@" to be
accepted as equivalent to "disable". I.e., those potential new users of
sysfs_streq() would have their ABI changed towards being less
permissive. That's a bigger change, with a higher chance of breaking
something. Do you even know if the maintainers of those drivers would
accept a switch to a case-insensitive sysfs_streq()?

Sorry, I really think you need a lot stronger motivation for introducing
either this change or a sysfs_strcaseeq().

Rasmus

2021-04-28 07:49:26

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On Wed, Apr 28, 2021 at 10:32 AM Gioh Kim <[email protected]> wrote:
> On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Wednesday, April 28, 2021, Gioh Kim <[email protected]> wrote:
> >> On Mon, Apr 12, 2021 at 1:33 PM Gioh Kim <[email protected]> wrote:
> >> >
> >> > As the name shows, it checks the strings inputed from sysfs.
> >> > It should work for both case-sensitive filesystem and
> >> > case-insensitive filesystem. Therefore sysfs_streq should work
> >> > case-insensitively.

...

> > Are you sure it’s good change? Sysfs is used for an ABI and you are opening a can of worms. From me NAK to this change without a very good background description that tells why it is safe to do.
>
> https://www.spinics.net/lists/kernel/msg3898123.html
> My initial idea was making a new function: sysfs_streqcase.
> Andrew and Greg suggested making sysfs_streq to be case-insensitive.
> I would like to have a discussion about it.

This patch even doesn't have users, but suddenly changes behaviour of
hundreds of existing users without any good justification. What
discussion here is needed? No way this goes to the kernel.

> >> I changed sysfs_streq to be case-insensitive as you requested.
> >> Is there any progress?


--
With Best Regards,
Andy Shevchenko

2021-04-28 08:34:07

by Gioh Kim

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On Wed, Apr 28, 2021 at 9:47 AM Rasmus Villemoes
<[email protected]> wrote:
>
> On 28/04/2021 09.31, Gioh Kim wrote:
> > On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
> > <[email protected]> wrote:
> >>
>
> >>
> >> Are you sure it’s good change? Sysfs is used for an ABI and you are opening a can of worms. From me NAK to this change without a very good background description that tells why it is safe to do.
> >
> > https://www.spinics.net/lists/kernel/msg3898123.html
> > My initial idea was making a new function: sysfs_streqcase.
> > Andrew and Greg suggested making sysfs_streq to be case-insensitive.
> > I would like to have a discussion about it.
>
> 1. That information should be in the commit log, not some random
> babbling about case sensitivity of file systems.

I don't think it is a good idea to write who suggested the concept of the patch.

>
> 2. So as Andy says, this is changing ABI for a whole lot of users in one
> go. While it's _probably_ true that nobody would care (because it just
> ends up accepting more strings, not fewer), your motivation seems to be
> to replace uses of strncasecmp() to prevent "[email protected]#[email protected]#@" to be
> accepted as equivalent to "disable". I.e., those potential new users of
> sysfs_streq() would have their ABI changed towards being less
> permissive. That's a bigger change, with a higher chance of breaking
> something. Do you even know if the maintainers of those drivers would
> accept a switch to a case-insensitive sysfs_streq()?

I don't know what all maintainers would think about the patch.
I thought sending a patch to the mailing list is asking for it, isn't it?
I am asking with this email.

And Andrew and Greg are maintainers.
I am sending this patch because I accepted their feedback.
I don't know other maintainers personally,
so I cannot contact them to ask what they think about my idea before
sending this email.


>
> Sorry, I really think you need a lot stronger motivation for introducing
> either this change or a sysfs_strcaseeq().

I found out a problem of sysfs_streq when I tried to use it for
modules I am working on (RTRS/RNBD).
Then I thought it would be better if there is something like sysfs_streqcase.
That's why I sent the path.
If nobody needs the change, it is ok for me to ignore the patch.

Maybe I misunderstand when I can send a patch to Linux kernel community.
But the only motivation of me is sharing my idea just in case there is
someone who also needs it.
I am sorry if I misunderstand how Linux kernel community works
but I thought I could suggest something good to me.

>
> Rasmus

2021-04-28 09:16:31

by Rasmus Villemoes

[permalink] [raw]
Subject: Re: [PATCH] lib/string: sysfs_streq works case insensitively

On 28/04/2021 10.31, Gioh Kim wrote:
> On Wed, Apr 28, 2021 at 9:47 AM Rasmus Villemoes
> <[email protected]> wrote:
>>
>> On 28/04/2021 09.31, Gioh Kim wrote:
>>> On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
>>> <[email protected]> wrote:
>>>>
>>
>>>>
>>>> Are you sure it’s good change? Sysfs is used for an ABI and you are opening a can of worms. From me NAK to this change without a very good background description that tells why it is safe to do.
>>>
>>> https://www.spinics.net/lists/kernel/msg3898123.html
>>> My initial idea was making a new function: sysfs_streqcase.
>>> Andrew and Greg suggested making sysfs_streq to be case-insensitive.
>>> I would like to have a discussion about it.
>>
>> 1. That information should be in the commit log, not some random
>> babbling about case sensitivity of file systems.
>
> I don't think it is a good idea to write who suggested the concept of the patch.

Sorry if it wasn't clear. I was referring to the information in that
link (i.e., your commit log for the first suggested patch).

>> Sorry, I really think you need a lot stronger motivation for introducing
>> either this change or a sysfs_strcaseeq().
>
> I found out a problem of sysfs_streq when I tried to use it for
> modules I am working on (RTRS/RNBD).

Again, this is the kind of motivation that would need to be in the
commit log. What problem are you solving in those modules that warrants
changing the semantics of sysfs_streq()? Why does that module's sysfs
interface need case-insensitive string comparison?

> Then I thought it would be better if there is something like sysfs_streqcase.
> That's why I sent the path.
> If nobody needs the change, it is ok for me to ignore the patch.

It's a lot easier to assess the merits of this new functionality
(whether in the form of a new separate function, or changed semantics of
sysfs_streq()) if we also see a few use cases. New functions are not
added without users. Bells and whistles are not added without users.

> Maybe I misunderstand when I can send a patch to Linux kernel community.
> But the only motivation of me is sharing my idea just in case there is
> someone who also needs it.

That's fine, but you need to include proper information on why a new
function is added or semantics of an existing one is changed (and for
the latter, some words on why you believe all existing callers would be
ok with that).

Rasmus