* [PATCH] lib/string: Introduce sysfs_streqcase
@ 2021-04-02 9:40 Gioh Kim
2021-04-02 17:59 ` kernel test robot
2021-04-02 18:17 ` Nick Desaulniers
0 siblings, 2 replies; 10+ messages in thread
From: Gioh Kim @ 2021-04-02 9:40 UTC (permalink / raw)
To: linux-kernel; +Cc: ndesaulniers, jinpu.wang, Gioh Kim
As the name shows, it checks if strings are equal in case insensitive
manner. I found some cases using strncasecmp to check the entire
strings and they would not work as intended.
For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
strncasecmp to check that the input via sysfs is "mi". But it would
work even-if the input is "min-wrongcommand".
And also drivers/pnp/interface.c checks "disable" command with
strncasecmp but it would also work if the command is "disable-wrong".
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
lib/string.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/lib/string.c b/lib/string.c
index 7548eb715ddb..5e6bc0d3d5c6 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
}
EXPORT_SYMBOL(sysfs_streq);
+/**
+ * sysfs_streqcase - same to sysfs_streq and case insensitive
+ * @s1: one string
+ * @s2: another string
+ *
+ */
+bool sysfs_streqcase(const char *s1, const char *s2)
+{
+ while (*s1 && tolower(*s1) == tolower(*s2)) {
+ s1++;
+ s2++;
+ }
+
+ if (*s1 == *s2)
+ return true;
+ if (!*s1 && *s2 == '\n' && !s2[1])
+ return true;
+ if (*s1 == '\n' && !s1[1] && !*s2)
+ return true;
+ return false;
+}
+EXPORT_SYMBOL(sysfs_streqcase);
+
/**
* match_string - matches given string in an array
* @array: array of strings
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-02 9:40 [PATCH] lib/string: Introduce sysfs_streqcase Gioh Kim
@ 2021-04-02 17:59 ` kernel test robot
2021-04-02 18:17 ` Nick Desaulniers
1 sibling, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-04-02 17:59 UTC (permalink / raw)
To: Gioh Kim, linux-kernel
Cc: kbuild-all, clang-built-linux, ndesaulniers, jinpu.wang, Gioh Kim
[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]
Hi Gioh,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gioh-Kim/lib-string-Introduce-sysfs_streqcase/20210402-174251
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5e46d1b78a03d52306f21f77a4e4a144b6d31486
config: mips-randconfig-r006-20210402 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project b23a314146956dd29b719ab537608ced736fc036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/0day-ci/linux/commit/b18d0dc7264473a4023926c510a67adfd7eaf119
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gioh-Kim/lib-string-Introduce-sysfs_streqcase/20210402-174251
git checkout b18d0dc7264473a4023926c510a67adfd7eaf119
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> lib/string.c:723:6: warning: no previous prototype for function 'sysfs_streqcase' [-Wmissing-prototypes]
bool sysfs_streqcase(const char *s1, const char *s2)
^
lib/string.c:723:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool sysfs_streqcase(const char *s1, const char *s2)
^
static
1 warning generated.
vim +/sysfs_streqcase +723 lib/string.c
716
717 /**
718 * sysfs_streqcase - same to sysfs_streq and case insensitive
719 * @s1: one string
720 * @s2: another string
721 *
722 */
> 723 bool sysfs_streqcase(const char *s1, const char *s2)
724 {
725 while (*s1 && tolower(*s1) == tolower(*s2)) {
726 s1++;
727 s2++;
728 }
729
730 if (*s1 == *s2)
731 return true;
732 if (!*s1 && *s2 == '\n' && !s2[1])
733 return true;
734 if (*s1 == '\n' && !s1[1] && !*s2)
735 return true;
736 return false;
737 }
738 EXPORT_SYMBOL(sysfs_streqcase);
739
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20960 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
@ 2021-04-02 17:59 ` kernel test robot
0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-04-02 17:59 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]
Hi Gioh,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gioh-Kim/lib-string-Introduce-sysfs_streqcase/20210402-174251
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5e46d1b78a03d52306f21f77a4e4a144b6d31486
config: mips-randconfig-r006-20210402 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project b23a314146956dd29b719ab537608ced736fc036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/0day-ci/linux/commit/b18d0dc7264473a4023926c510a67adfd7eaf119
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gioh-Kim/lib-string-Introduce-sysfs_streqcase/20210402-174251
git checkout b18d0dc7264473a4023926c510a67adfd7eaf119
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> lib/string.c:723:6: warning: no previous prototype for function 'sysfs_streqcase' [-Wmissing-prototypes]
bool sysfs_streqcase(const char *s1, const char *s2)
^
lib/string.c:723:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool sysfs_streqcase(const char *s1, const char *s2)
^
static
1 warning generated.
vim +/sysfs_streqcase +723 lib/string.c
716
717 /**
718 * sysfs_streqcase - same to sysfs_streq and case insensitive
719 * @s1: one string
720 * @s2: another string
721 *
722 */
> 723 bool sysfs_streqcase(const char *s1, const char *s2)
724 {
725 while (*s1 && tolower(*s1) == tolower(*s2)) {
726 s1++;
727 s2++;
728 }
729
730 if (*s1 == *s2)
731 return true;
732 if (!*s1 && *s2 == '\n' && !s2[1])
733 return true;
734 if (*s1 == '\n' && !s1[1] && !*s2)
735 return true;
736 return false;
737 }
738 EXPORT_SYMBOL(sysfs_streqcase);
739
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 20960 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-02 9:40 [PATCH] lib/string: Introduce sysfs_streqcase Gioh Kim
2021-04-02 17:59 ` kernel test robot
@ 2021-04-02 18:17 ` Nick Desaulniers
2021-04-02 18:23 ` Kees Cook
2021-04-02 19:41 ` Gioh Kim
1 sibling, 2 replies; 10+ messages in thread
From: Nick Desaulniers @ 2021-04-02 18:17 UTC (permalink / raw)
To: Gioh Kim; +Cc: LKML, jinpu.wang, Andrew Morton, Kees Cook
Thanks for the patch!
+ akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
>
> As the name shows, it checks if strings are equal in case insensitive
> manner. I found some cases using strncasecmp to check the entire
> strings and they would not work as intended.
>
> For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> strncasecmp to check that the input via sysfs is "mi". But it would
> work even-if the input is "min-wrongcommand".
> And also drivers/pnp/interface.c checks "disable" command with
> strncasecmp but it would also work if the command is "disable-wrong".
Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
Also, if they're being liberal in accepting either case, I don't see
why the sysfs nodes should be strict in rejecting trailing input at
that point.
>
> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> ---
> lib/string.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/lib/string.c b/lib/string.c
> index 7548eb715ddb..5e6bc0d3d5c6 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
> }
> EXPORT_SYMBOL(sysfs_streq);
>
> +/**
> + * sysfs_streqcase - same to sysfs_streq and case insensitive
> + * @s1: one string
> + * @s2: another string
> + *
> + */
> +bool sysfs_streqcase(const char *s1, const char *s2)
> +{
> + while (*s1 && tolower(*s1) == tolower(*s2)) {
> + s1++;
> + s2++;
> + }
> +
> + if (*s1 == *s2)
> + return true;
> + if (!*s1 && *s2 == '\n' && !s2[1])
> + return true;
> + if (*s1 == '\n' && !s1[1] && !*s2)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL(sysfs_streqcase);
This should be declared in
include/linux/string.h
in order for others to use this (as 0day bot notes).
> +
> /**
> * match_string - matches given string in an array
> * @array: array of strings
> --
> 2.25.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-02 18:17 ` Nick Desaulniers
@ 2021-04-02 18:23 ` Kees Cook
2021-04-02 19:43 ` Gioh Kim
2021-04-02 19:41 ` Gioh Kim
1 sibling, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-04-02 18:23 UTC (permalink / raw)
To: Nick Desaulniers; +Cc: Gioh Kim, LKML, jinpu.wang, Andrew Morton
On Fri, Apr 02, 2021 at 11:17:13AM -0700, Nick Desaulniers wrote:
> Thanks for the patch!
>
> + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
>
> On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner. I found some cases using strncasecmp to check the entire
> > strings and they would not work as intended.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> > And also drivers/pnp/interface.c checks "disable" command with
> > strncasecmp but it would also work if the command is "disable-wrong".
>
> Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
>
> Also, if they're being liberal in accepting either case, I don't see
> why the sysfs nodes should be strict in rejecting trailing input at
> that point.
I think this shouldn't be prefixed "sysfs_" -- name it for what it does,
not where it gets used, if it's a general utility function.
-Kees
>
> >
> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > ---
> > lib/string.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/lib/string.c b/lib/string.c
> > index 7548eb715ddb..5e6bc0d3d5c6 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
> > }
> > EXPORT_SYMBOL(sysfs_streq);
> >
> > +/**
> > + * sysfs_streqcase - same to sysfs_streq and case insensitive
> > + * @s1: one string
> > + * @s2: another string
> > + *
> > + */
> > +bool sysfs_streqcase(const char *s1, const char *s2)
> > +{
> > + while (*s1 && tolower(*s1) == tolower(*s2)) {
> > + s1++;
> > + s2++;
> > + }
> > +
> > + if (*s1 == *s2)
> > + return true;
> > + if (!*s1 && *s2 == '\n' && !s2[1])
> > + return true;
> > + if (*s1 == '\n' && !s1[1] && !*s2)
> > + return true;
> > + return false;
> > +}
> > +EXPORT_SYMBOL(sysfs_streqcase);
>
> This should be declared in
> include/linux/string.h
> in order for others to use this (as 0day bot notes).
>
> > +
> > /**
> > * match_string - matches given string in an array
> > * @array: array of strings
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Kees Cook
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-02 18:17 ` Nick Desaulniers
2021-04-02 18:23 ` Kees Cook
@ 2021-04-02 19:41 ` Gioh Kim
1 sibling, 0 replies; 10+ messages in thread
From: Gioh Kim @ 2021-04-02 19:41 UTC (permalink / raw)
To: Nick Desaulniers; +Cc: LKML, Jinpu Wang, Andrew Morton, Kees Cook
On Fri, Apr 2, 2021 at 8:17 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Thanks for the patch!
>
> + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
>
> On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner. I found some cases using strncasecmp to check the entire
> > strings and they would not work as intended.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> > And also drivers/pnp/interface.c checks "disable" command with
> > strncasecmp but it would also work if the command is "disable-wrong".
>
> Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
>
> Also, if they're being liberal in accepting either case, I don't see
> why the sysfs nodes should be strict in rejecting trailing input at
> that point.
>
On Fri, Apr 2, 2021 at 8:17 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Thanks for the patch!
>
> + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
>
> On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner. I found some cases using strncasecmp to check the entire
> > strings and they would not work as intended.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> > And also drivers/pnp/interface.c checks "disable" command with
> > strncasecmp but it would also work if the command is "disable-wrong".
>
> Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
>
> Also, if they're being liberal in accepting either case, I don't see
> why the sysfs nodes should be strict in rejecting trailing input at
> that point.
>
strcasecmp does not work when a user inputs the command with echo.
We can force the human to use 'echo -n' but there are also some applications
that pass the command with \n. If the command includes \n, strcasecmp does
not work.
In short, I need a function working well for both case-insensitive string and
a string followed by '\n'.
I am not native speaker of English. I think the below example can show
my problem.
Below is the original code. That code does not work because of the \n
in the command.
char buf[] = "mi\n";
if (strcasecmp(buf, "min-inflight") == 0 ||
strcasecmp(buf, "mi") == 0)
printf("inflight\n");
else if (strcasecmp(buf, "min-latency") == 0 ||
strcasecmp(buf, "ml") == 0)
printf("latency\n");
else
printf("wrong\n");
Below is the current code in RTRS module. We replaced strcasecmp with
strncasecmp.
That works well but ugly and error-prone.
size_t len = 0;
len = strlen(buf);
if (buf[len - 1] == '\n')
len--;
if (strncasecmp(buf, "min-inflight", 12) == 0 ||
(len == 2 && strncasecmp(buf, "mi", 2) == 0))
printf("inflight\n");
else if (strncasecmp(buf, "min-latency", 11) == 0 ||
(len == 2 && strncasecmp(buf, "ml", 2)) == 0)
printf("latency\n");
else
printf("wrong\n");
I think sysfs_streqcase could be the best option as below.
if (sysfs_streqcase(buf, "min-inflight") ||
sysfs_streqcase(buf, "mi"))
printf("inflight\n");
else if (sysfs_streqcase(buf, "min-latency") ||
sysfs_streqcase(buf, "ml"))
printf("latency\n");
else
printf("wrong\n");
I think that case is not my own problem.
I think some code handling debugfs and sysfs also have the same problem.
>
> This should be declared in
> include/linux/string.h
> in order for others to use this (as 0day bot notes).
Thank you for the kind review.
I will add the declaration if I get the positive feedback for sysfs_streqcase.
>
> > +
> > /**
> > * match_string - matches given string in an array
> > * @array: array of strings
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-02 18:23 ` Kees Cook
@ 2021-04-02 19:43 ` Gioh Kim
0 siblings, 0 replies; 10+ messages in thread
From: Gioh Kim @ 2021-04-02 19:43 UTC (permalink / raw)
To: Kees Cook; +Cc: Nick Desaulniers, LKML, Jinpu Wang, Andrew Morton
On Fri, Apr 2, 2021 at 8:24 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Apr 02, 2021 at 11:17:13AM -0700, Nick Desaulniers wrote:
> > Thanks for the patch!
> >
> > + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
> >
> > On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> > >
> > > As the name shows, it checks if strings are equal in case insensitive
> > > manner. I found some cases using strncasecmp to check the entire
> > > strings and they would not work as intended.
> > >
> > > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > > strncasecmp to check that the input via sysfs is "mi". But it would
> > > work even-if the input is "min-wrongcommand".
> > > And also drivers/pnp/interface.c checks "disable" command with
> > > strncasecmp but it would also work if the command is "disable-wrong".
> >
> > Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
> >
> > Also, if they're being liberal in accepting either case, I don't see
> > why the sysfs nodes should be strict in rejecting trailing input at
> > that point.
>
> I think this shouldn't be prefixed "sysfs_" -- name it for what it does,
> not where it gets used, if it's a general utility function.
>
I think sysfs_streqcase is another version of sysfs_streq.
That is why I use "sysfs_" prefix.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-07 20:06 ` Nick Desaulniers
@ 2021-04-08 7:25 ` Gioh Kim
0 siblings, 0 replies; 10+ messages in thread
From: Gioh Kim @ 2021-04-08 7:25 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Andrew Morton, LKML, Dan Williams, Andrey Konovalov,
laniel_francis, Kees Cook, Daniel Axtens
On Wed, Apr 7, 2021 at 10:07 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Apr 6, 2021 at 11:15 PM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> >
> > I found some more cases using strncasecmp to check the entire string
> > such as rtrs-clt-sysfs.c does. drivers/pnp/interface.c checks
> > "disable" command with strncasecmp but it would also work if the
> > command is "disable-wrong".
>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> I do wonder if these (sysfs_streqcase and sysfs_streq) could or should
> be conditionally available on CONFIG_SYSFS=y; don't pay for what you
> don't use (without needing CONFIG_LD_DEAD_CODE_DATA_ELIMINATION=y)?
Good idea.
Thank you.
>
> Also, it might be nice to share the second half of the function with
> sysfs_streq via a new static function. Though it will just get
> inlined in both for CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y, it might
> help the compiler if CONFIG_CC_OPTIMIZE_FOR_SIZE=y was instead chosen
> if the compiler cannot outline/deduplicate the shared code. At the
> least, there's less duplication between two very similar functions; if
> one changes then authors may need to be careful to update both.
Yes, they are exactly the same.
I will make an inline function for the common code.
>
> Are either of those concerns worth a v3? ¯\_(ツ)_/¯
Sure, I will not forget to add 'V2'.
Thank you for kind review.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] lib/string: Introduce sysfs_streqcase
2021-04-07 6:14 Gioh Kim
@ 2021-04-07 20:06 ` Nick Desaulniers
2021-04-08 7:25 ` Gioh Kim
0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2021-04-07 20:06 UTC (permalink / raw)
To: Gioh Kim, Andrew Morton
Cc: LKML, Dan Williams, Andrey Konovalov, laniel_francis, Kees Cook,
Daniel Axtens
On Tue, Apr 6, 2021 at 11:15 PM Gioh Kim <gi-oh.kim@ionos.com> wrote:
>
> As the name shows, it checks if strings are equal in case insensitive
> manner.
>
> For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> strncasecmp to check that the input via sysfs is "mi". But it would
> work even-if the input is "min-wrongcommand".
>
> I found some more cases using strncasecmp to check the entire string
> such as rtrs-clt-sysfs.c does. drivers/pnp/interface.c checks
> "disable" command with strncasecmp but it would also work if the
> command is "disable-wrong".
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
I do wonder if these (sysfs_streqcase and sysfs_streq) could or should
be conditionally available on CONFIG_SYSFS=y; don't pay for what you
don't use (without needing CONFIG_LD_DEAD_CODE_DATA_ELIMINATION=y)?
Also, it might be nice to share the second half of the function with
sysfs_streq via a new static function. Though it will just get
inlined in both for CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y, it might
help the compiler if CONFIG_CC_OPTIMIZE_FOR_SIZE=y was instead chosen
if the compiler cannot outline/deduplicate the shared code. At the
least, there's less duplication between two very similar functions; if
one changes then authors may need to be careful to update both.
Are either of those concerns worth a v3? ¯\_(ツ)_/¯
>
> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> ---
> include/linux/string.h | 1 +
> lib/string.c | 23 +++++++++++++++++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4fcfb56abcf5..36d00ff8013e 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -184,6 +184,7 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> extern void argv_free(char **argv);
>
> extern bool sysfs_streq(const char *s1, const char *s2);
> +extern bool sysfs_streqcase(const char *s1, const char *s2);
> extern int kstrtobool(const char *s, bool *res);
> static inline int strtobool(const char *s, bool *res)
> {
> diff --git a/lib/string.c b/lib/string.c
> index 7548eb715ddb..5e6bc0d3d5c6 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
> }
> EXPORT_SYMBOL(sysfs_streq);
>
> +/**
> + * sysfs_streqcase - same to sysfs_streq and case insensitive
> + * @s1: one string
> + * @s2: another string
> + *
> + */
> +bool sysfs_streqcase(const char *s1, const char *s2)
> +{
> + while (*s1 && tolower(*s1) == tolower(*s2)) {
> + s1++;
> + s2++;
> + }
> +
> + if (*s1 == *s2)
> + return true;
> + if (!*s1 && *s2 == '\n' && !s2[1])
> + return true;
> + if (*s1 == '\n' && !s1[1] && !*s2)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL(sysfs_streqcase);
> +
> /**
> * match_string - matches given string in an array
> * @array: array of strings
> --
> 2.25.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] lib/string: Introduce sysfs_streqcase
@ 2021-04-07 6:14 Gioh Kim
2021-04-07 20:06 ` Nick Desaulniers
0 siblings, 1 reply; 10+ messages in thread
From: Gioh Kim @ 2021-04-07 6:14 UTC (permalink / raw)
To: linux-kernel
Cc: ndesaulniers, dan.j.williams, andreyknvl, laniel_francis,
keescook, dja, akpm, Gioh Kim
As the name shows, it checks if strings are equal in case insensitive
manner.
For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
strncasecmp to check that the input via sysfs is "mi". But it would
work even-if the input is "min-wrongcommand".
I found some more cases using strncasecmp to check the entire string
such as rtrs-clt-sysfs.c does. drivers/pnp/interface.c checks
"disable" command with strncasecmp but it would also work if the
command is "disable-wrong".
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
include/linux/string.h | 1 +
lib/string.c | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index 4fcfb56abcf5..36d00ff8013e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -184,6 +184,7 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
extern void argv_free(char **argv);
extern bool sysfs_streq(const char *s1, const char *s2);
+extern bool sysfs_streqcase(const char *s1, const char *s2);
extern int kstrtobool(const char *s, bool *res);
static inline int strtobool(const char *s, bool *res)
{
diff --git a/lib/string.c b/lib/string.c
index 7548eb715ddb..5e6bc0d3d5c6 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
}
EXPORT_SYMBOL(sysfs_streq);
+/**
+ * sysfs_streqcase - same to sysfs_streq and case insensitive
+ * @s1: one string
+ * @s2: another string
+ *
+ */
+bool sysfs_streqcase(const char *s1, const char *s2)
+{
+ while (*s1 && tolower(*s1) == tolower(*s2)) {
+ s1++;
+ s2++;
+ }
+
+ if (*s1 == *s2)
+ return true;
+ if (!*s1 && *s2 == '\n' && !s2[1])
+ return true;
+ if (*s1 == '\n' && !s1[1] && !*s2)
+ return true;
+ return false;
+}
+EXPORT_SYMBOL(sysfs_streqcase);
+
/**
* match_string - matches given string in an array
* @array: array of strings
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-04-08 7:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 9:40 [PATCH] lib/string: Introduce sysfs_streqcase Gioh Kim
2021-04-02 17:59 ` kernel test robot
2021-04-02 17:59 ` kernel test robot
2021-04-02 18:17 ` Nick Desaulniers
2021-04-02 18:23 ` Kees Cook
2021-04-02 19:43 ` Gioh Kim
2021-04-02 19:41 ` Gioh Kim
2021-04-07 6:14 Gioh Kim
2021-04-07 20:06 ` Nick Desaulniers
2021-04-08 7:25 ` Gioh Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.