linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lib/string: sysfs_streq works case insensitively
@ 2021-04-12 11:33 Gioh Kim
  2021-04-28  5:41 ` Gioh Kim
  2021-04-28  6:37 ` Rasmus Villemoes
  0 siblings, 2 replies; 8+ messages in thread
From: Gioh Kim @ 2021-04-12 11:33 UTC (permalink / raw)
  To: linux-kernel, akpm; +Cc: ndesaulniers, gregkh, Gioh Kim

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 <gi-oh.kim@ionos.com>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
  2021-04-12 11:33 [PATCH] lib/string: sysfs_streq works case insensitively Gioh Kim
@ 2021-04-28  5:41 ` Gioh Kim
       [not found]   ` <CAHp75Vf2yJ5=zdxRcPKmKGCKeF8As=Nv2S9fm0ciVXL5HGbWDg@mail.gmail.com>
  2021-04-28  6:37 ` Rasmus Villemoes
  1 sibling, 1 reply; 8+ messages in thread
From: Gioh Kim @ 2021-04-28  5:41 UTC (permalink / raw)
  To: LKML, Andrew Morton; +Cc: Nick Desaulniers, Greg Kroah-Hartman

On Mon, Apr 12, 2021 at 1:33 PM Gioh Kim <gi-oh.kim@ionos.com> 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 <gi-oh.kim@ionos.com>
> ---
>  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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
  2021-04-12 11:33 [PATCH] lib/string: sysfs_streq works case insensitively Gioh Kim
  2021-04-28  5:41 ` Gioh Kim
@ 2021-04-28  6:37 ` Rasmus Villemoes
  1 sibling, 0 replies; 8+ messages in thread
From: Rasmus Villemoes @ 2021-04-28  6:37 UTC (permalink / raw)
  To: Gioh Kim, linux-kernel, akpm; +Cc: ndesaulniers, gregkh

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
       [not found]   ` <CAHp75Vf2yJ5=zdxRcPKmKGCKeF8As=Nv2S9fm0ciVXL5HGbWDg@mail.gmail.com>
@ 2021-04-28  7:31     ` Gioh Kim
  2021-04-28  7:46       ` Andy Shevchenko
  2021-04-28  7:47       ` Rasmus Villemoes
  0 siblings, 2 replies; 8+ messages in thread
From: Gioh Kim @ 2021-04-28  7:31 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: LKML, Andrew Morton, Nick Desaulniers, Greg Kroah-Hartman

On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Wednesday, April 28, 2021, Gioh Kim <gi-oh.kim@ionos.com> wrote:
>>
>> On Mon, Apr 12, 2021 at 1:33 PM Gioh Kim <gi-oh.kim@ionos.com> 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 <gi-oh.kim@ionos.com>
>> > ---
>> >  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
>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
  2021-04-28  7:31     ` Gioh Kim
@ 2021-04-28  7:46       ` Andy Shevchenko
  2021-04-28  7:47       ` Rasmus Villemoes
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2021-04-28  7:46 UTC (permalink / raw)
  To: Gioh Kim; +Cc: LKML, Andrew Morton, Nick Desaulniers, Greg Kroah-Hartman

On Wed, Apr 28, 2021 at 10:32 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wednesday, April 28, 2021, Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >> On Mon, Apr 12, 2021 at 1:33 PM Gioh Kim <gi-oh.kim@ionos.com> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
  2021-04-28  7:31     ` Gioh Kim
  2021-04-28  7:46       ` Andy Shevchenko
@ 2021-04-28  7:47       ` Rasmus Villemoes
  2021-04-28  8:31         ` Gioh Kim
  1 sibling, 1 reply; 8+ messages in thread
From: Rasmus Villemoes @ 2021-04-28  7:47 UTC (permalink / raw)
  To: Gioh Kim, Andy Shevchenko
  Cc: LKML, Andrew Morton, Nick Desaulniers, Greg Kroah-Hartman

On 28/04/2021 09.31, Gioh Kim wrote:
> On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> 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 "disableGARBAGE@#$@#@" 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
  2021-04-28  7:47       ` Rasmus Villemoes
@ 2021-04-28  8:31         ` Gioh Kim
  2021-04-28  9:13           ` Rasmus Villemoes
  0 siblings, 1 reply; 8+ messages in thread
From: Gioh Kim @ 2021-04-28  8:31 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Andy Shevchenko, LKML, Andrew Morton, Nick Desaulniers,
	Greg Kroah-Hartman

On Wed, Apr 28, 2021 at 9:47 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 28/04/2021 09.31, Gioh Kim wrote:
> > On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> 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 "disableGARBAGE@#$@#@" 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] lib/string: sysfs_streq works case insensitively
  2021-04-28  8:31         ` Gioh Kim
@ 2021-04-28  9:13           ` Rasmus Villemoes
  0 siblings, 0 replies; 8+ messages in thread
From: Rasmus Villemoes @ 2021-04-28  9:13 UTC (permalink / raw)
  To: Gioh Kim, Rasmus Villemoes
  Cc: Andy Shevchenko, LKML, Andrew Morton, Nick Desaulniers,
	Greg Kroah-Hartman

On 28/04/2021 10.31, Gioh Kim wrote:
> On Wed, Apr 28, 2021 at 9:47 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> On 28/04/2021 09.31, Gioh Kim wrote:
>>> On Wed, Apr 28, 2021 at 8:42 AM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-28  9:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 11:33 [PATCH] lib/string: sysfs_streq works case insensitively Gioh Kim
2021-04-28  5:41 ` Gioh Kim
     [not found]   ` <CAHp75Vf2yJ5=zdxRcPKmKGCKeF8As=Nv2S9fm0ciVXL5HGbWDg@mail.gmail.com>
2021-04-28  7:31     ` Gioh Kim
2021-04-28  7:46       ` Andy Shevchenko
2021-04-28  7:47       ` Rasmus Villemoes
2021-04-28  8:31         ` Gioh Kim
2021-04-28  9:13           ` Rasmus Villemoes
2021-04-28  6:37 ` Rasmus Villemoes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).