* [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
[parent not found: <CAHp75Vf2yJ5=zdxRcPKmKGCKeF8As=Nv2S9fm0ciVXL5HGbWDg@mail.gmail.com>]
* 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
* 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
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).