[PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Jagdish Gediya]

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(-)

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;
-- 
2.35.1

Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Petr Mladek]

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

Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Jagdish Gediya]

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
> 

Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Petr Mladek]

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

Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Jagdish Gediya]

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
> 
> 
> 

Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko