| Message ID | 20220426064001.14241-1-jvgediya@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool | expand |
On Tue 2022-04-26 12:10:01, Jagdish Gediya wrote: > At many places in kernel, It is necessary to convert sysfs input > to corrosponding bool value e.g. "false" or "0" need to be converted > to bool false, "true" or "1" need to be converted to bool true, > places where such conversion is needed currently check the input > string manually, kstrtobool can be utilized at such places but > currently kstrtobool doesn't have support to "false"/"true". > > Add "false"/"true" support to kstrtobool while string conversion > to bool. Modify existing manual sysfs conversions to use kstrtobool(). It looks reasonable. I would just do it slightly other way, see below. > This patch doesn't have any functionality change. This is not true. All kstrtobool() callers will react differently on the "true"/"false" input. > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Richard Fitzgerald <rf@opensource.cirrus.com> > Cc: Petr Mladek <pmladek@suse.com> > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com> > --- > change in v2: > Modified kstrtobool to handle "false"/"true". Removed > new function sysfs_strbool introduced in v1. > > lib/kstrtox.c | 7 +++++++ > mm/migrate.c | 6 +----- > mm/swap_state.c | 6 +----- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/lib/kstrtox.c b/lib/kstrtox.c > index 886510d248e5..3a5e29557838 100644 > --- a/lib/kstrtox.c > +++ b/lib/kstrtox.c > @@ -377,6 +377,13 @@ int kstrtobool(const char *s, bool *res) > } > break; > default: > + if (!strncmp(s, "true", 4)) { > + *res = true; > + return 0; > + } else if (!strncmp(s, "false", 5)) { > + *res = false; > + return 0; It should be enough to check the first letter like we do in the other cases. I mean to set true when s[0] is 'T' or 't' and false when s[0] is 'F' or 'f'. Also please update comment above the function definition. > + } > break; > } > Best Regards, Petr
On Tue, Apr 26, 2022 at 11:39:57AM +0200, Petr Mladek wrote: > On Tue 2022-04-26 12:10:01, Jagdish Gediya wrote: > > At many places in kernel, It is necessary to convert sysfs input > > to corrosponding bool value e.g. "false" or "0" need to be converted > > to bool false, "true" or "1" need to be converted to bool true, > > places where such conversion is needed currently check the input > > string manually, kstrtobool can be utilized at such places but > > currently kstrtobool doesn't have support to "false"/"true". > > > > Add "false"/"true" support to kstrtobool while string conversion > > to bool. Modify existing manual sysfs conversions to use kstrtobool(). > > It looks reasonable. I would just do it slightly other way, see > below. > > > This patch doesn't have any functionality change. > > This is not true. All kstrtobool() callers will react differently > on the "true"/"false" input. how? Is it related to performance as more characters are compared? otherwise semantic wise they will get the expected response, correct? > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Richard Fitzgerald <rf@opensource.cirrus.com> > > Cc: Petr Mladek <pmladek@suse.com> > > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com> > > --- > > change in v2: > > Modified kstrtobool to handle "false"/"true". Removed > > new function sysfs_strbool introduced in v1. > > > > lib/kstrtox.c | 7 +++++++ > > mm/migrate.c | 6 +----- > > mm/swap_state.c | 6 +----- > > 3 files changed, 9 insertions(+), 10 deletions(-) > > > > diff --git a/lib/kstrtox.c b/lib/kstrtox.c > > index 886510d248e5..3a5e29557838 100644 > > --- a/lib/kstrtox.c > > +++ b/lib/kstrtox.c > > @@ -377,6 +377,13 @@ int kstrtobool(const char *s, bool *res) > > } > > break; > > default: > > + if (!strncmp(s, "true", 4)) { > > + *res = true; > > + return 0; > > + } else if (!strncmp(s, "false", 5)) { > > + *res = false; > > + return 0; > > It should be enough to check the first letter like we do in > the other cases. I mean to set true when s[0] is 'T' or 't' > and false when s[0] is 'F' or 'f'. For "on" and "off", 2 characters are matched, so is it good enough to compare only single character for strings "true" and "false"? > Also please update comment above the function definition. > > > > + } > > break; > > } > > > > Best Regards, > Petr >
On Tue, Apr 26, 2022 at 12:10:01PM +0530, Jagdish Gediya wrote: > At many places in kernel, It is necessary to convert sysfs input > to corrosponding bool value e.g. "false" or "0" need to be converted > to bool false, "true" or "1" need to be converted to bool true, > places where such conversion is needed currently check the input > string manually, kstrtobool can be utilized at such places but kstrtobool() > currently kstrtobool doesn't have support to "false"/"true". Ditto. > Add "false"/"true" support to kstrtobool while string conversion Ditto. > to bool. Modify existing manual sysfs conversions to use kstrtobool(). > This patch doesn't have any functionality change. > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Richard Fitzgerald <rf@opensource.cirrus.com> > Cc: Petr Mladek <pmladek@suse.com> You may use --cc parameter to `git send-email`. ... > lib/kstrtox.c | 7 +++++++ > mm/migrate.c | 6 +----- > mm/swap_state.c | 6 +----- Please, split to two. Also Documentation update is missed.
On Tue 2022-04-26 15:33:52, Jagdish Gediya wrote: > On Tue, Apr 26, 2022 at 11:39:57AM +0200, Petr Mladek wrote: > > On Tue 2022-04-26 12:10:01, Jagdish Gediya wrote: > > > At many places in kernel, It is necessary to convert sysfs input > > > to corrosponding bool value e.g. "false" or "0" need to be converted > > > to bool false, "true" or "1" need to be converted to bool true, > > > places where such conversion is needed currently check the input > > > string manually, kstrtobool can be utilized at such places but > > > currently kstrtobool doesn't have support to "false"/"true". > > > > > > Add "false"/"true" support to kstrtobool while string conversion > > > to bool. Modify existing manual sysfs conversions to use kstrtobool(). > > > > It looks reasonable. I would just do it slightly other way, see > > below. > > > > > This patch doesn't have any functionality change. > > > > This is not true. All kstrtobool() callers will react differently > > on the "true"/"false" input. > > how? Is it related to performance as more characters are compared? > otherwise semantic wise they will get the expected response, correct? kstrtobool() returned -EINVAL for "true"/"false" strings before this patch. It will successfully handle them after this patch. This is a behavior/functional change that will affect all existing kstrtobool() callers. The change makes sense and most likely will not cause any regression. But are you 100% sure? People do crazy things. > > > --- a/lib/kstrtox.c > > > +++ b/lib/kstrtox.c > > > @@ -377,6 +377,13 @@ int kstrtobool(const char *s, bool *res) > > > } > > > break; > > > default: > > > + if (!strncmp(s, "true", 4)) { > > > + *res = true; > > > + return 0; > > > + } else if (!strncmp(s, "false", 5)) { > > > + *res = false; > > > + return 0; > > > > It should be enough to check the first letter like we do in > > the other cases. I mean to set true when s[0] is 'T' or 't' > > and false when s[0] is 'F' or 'f'. > > For "on" and "off", 2 characters are matched, so is it good enough > to compare only single character for strings "true" and "false"? Yes, the 1st character is enough to distinguish "true" and "false". Two characters are needed for "on" and "off" because the 1st character is the same. Best Regards, Petr
On Tue, Apr 26, 2022 at 04:27:01PM +0300, Andy Shevchenko wrote: > On Tue, Apr 26, 2022 at 12:10:01PM +0530, Jagdish Gediya wrote: > > At many places in kernel, It is necessary to convert sysfs input > > to corrosponding bool value e.g. "false" or "0" need to be converted > > to bool false, "true" or "1" need to be converted to bool true, > > places where such conversion is needed currently check the input > > string manually, kstrtobool can be utilized at such places but > > kstrtobool() > > > currently kstrtobool doesn't have support to "false"/"true". > > > Ditto. > > > Add "false"/"true" support to kstrtobool while string conversion > > Ditto. > > > to bool. Modify existing manual sysfs conversions to use kstrtobool(). > > > This patch doesn't have any functionality change. > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Richard Fitzgerald <rf@opensource.cirrus.com> > > Cc: Petr Mladek <pmladek@suse.com> > > You may use --cc parameter to `git send-email`. Sure, Will remove these from here. > ... > > > lib/kstrtox.c | 7 +++++++ > > mm/migrate.c | 6 +----- > > mm/swap_state.c | 6 +----- > > Please, split to two. Sure > Also Documentation update is missed. I am not finding any related documentation. I can update the comment on the function for true/false change. --Jagdish > > >
On Tue, Apr 26, 2022 at 8:49 PM Jagdish Gediya <jvgediya@linux.ibm.com> wrote: > On Tue, Apr 26, 2022 at 04:27:01PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 26, 2022 at 12:10:01PM +0530, Jagdish Gediya wrote: > > > At many places in kernel, It is necessary to convert sysfs input > > > to corrosponding bool value e.g. "false" or "0" need to be converted corresponding > > > to bool false, "true" or "1" need to be converted to bool true, > > > places where such conversion is needed currently check the input > > > string manually, kstrtobool can be utilized at such places but > > > > kstrtobool() > > > > > currently kstrtobool doesn't have support to "false"/"true". > > > > > > Ditto. > > > > > Add "false"/"true" support to kstrtobool while string conversion > > > > Ditto. > > > > > to bool. Modify existing manual sysfs conversions to use kstrtobool(). ... > > Also Documentation update is missed. > I am not finding any related documentation. I can update > the comment on the function for true/false change. Yes, please.
diff --git a/lib/kstrtox.c b/lib/kstrtox.c index 886510d248e5..3a5e29557838 100644 --- a/lib/kstrtox.c +++ b/lib/kstrtox.c @@ -377,6 +377,13 @@ int kstrtobool(const char *s, bool *res) } break; default: + if (!strncmp(s, "true", 4)) { + *res = true; + return 0; + } else if (!strncmp(s, "false", 5)) { + *res = false; + return 0; + } break; } diff --git a/mm/migrate.c b/mm/migrate.c index 6c31ee1e1c9b..1de39bbfd6f9 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2523,11 +2523,7 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - if (!strncmp(buf, "true", 4) || !strncmp(buf, "1", 1)) - numa_demotion_enabled = true; - else if (!strncmp(buf, "false", 5) || !strncmp(buf, "0", 1)) - numa_demotion_enabled = false; - else + if (kstrtobool(buf, &numa_demotion_enabled)) return -EINVAL; return count; diff --git a/mm/swap_state.c b/mm/swap_state.c index 013856004825..dba10045a825 100644 --- a/mm/swap_state.c +++ b/mm/swap_state.c @@ -865,11 +865,7 @@ static ssize_t vma_ra_enabled_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - if (!strncmp(buf, "true", 4) || !strncmp(buf, "1", 1)) - enable_vma_readahead = true; - else if (!strncmp(buf, "false", 5) || !strncmp(buf, "0", 1)) - enable_vma_readahead = false; - else + if (kstrtobool(buf, &enable_vma_readahead)) return -EINVAL; return count;
At many places in kernel, It is necessary to convert sysfs input to corrosponding bool value e.g. "false" or "0" need to be converted to bool false, "true" or "1" need to be converted to bool true, places where such conversion is needed currently check the input string manually, kstrtobool can be utilized at such places but currently kstrtobool doesn't have support to "false"/"true". Add "false"/"true" support to kstrtobool while string conversion to bool. Modify existing manual sysfs conversions to use kstrtobool(). This patch doesn't have any functionality change. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Richard Fitzgerald <rf@opensource.cirrus.com> Cc: Petr Mladek <pmladek@suse.com> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com> --- change in v2: Modified kstrtobool to handle "false"/"true". Removed new function sysfs_strbool introduced in v1. lib/kstrtox.c | 7 +++++++ mm/migrate.c | 6 +----- mm/swap_state.c | 6 +----- 3 files changed, 9 insertions(+), 10 deletions(-)