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

[PATCH v2 1/2] 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 it doesn't have support to accept "false"/"true".

Add support to accept "false"/"true" as valid string in kstrtobool().

Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
---
Chnage in v2:
- kstrtobool to kstrtobool() in commit message.
- Split single patch into 2
- Remove strcmp usage and instead compare 1st character only.

 lib/kstrtox.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 886510d248e5..465e31e4d70d 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -340,7 +340,7 @@ EXPORT_SYMBOL(kstrtos8);
  * @s: input string
  * @res: result
  *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
+ * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or
  * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
  * pointed to by res is updated upon finding a match.
  */
@@ -353,11 +353,15 @@ int kstrtobool(const char *s, bool *res)
 	switch (s[0]) {
 	case 'y':
 	case 'Y':
+	case 't':
+	case 'T':
 	case '1':
 		*res = true;
 		return 0;
 	case 'n':
 	case 'N':
+	case 'f':
+	case 'F':
 	case '0':
 		*res = false;
 		return 0;
-- 
2.35.1

[PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Jagdish Gediya]

Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
to false, "true" or "1" to true are currently handled through strncmp
at multiple places. Use kstrtobool() to convert sysfs input to bool
value.

Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
---
Chnage in v2:
- kstrtobool to kstrtobool() in commit message.
- Split single patch into 2
- Remove strcmp usage and instead compare 1st character only.

 mm/migrate.c    | 6 +-----
 mm/swap_state.c | 6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

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 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Matthew Wilcox]

On Tue, Apr 26, 2022 at 10:30:39PM +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
> currently it doesn't have support to accept "false"/"true".
> 
> Add support to accept "false"/"true" as valid string in kstrtobool().
> 
> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

... except ...

> - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> + * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or

https://en.wikipedia.org/wiki/If_and_only_if

Re: [PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Matthew Wilcox]

On Tue, Apr 26, 2022 at 10:30:40PM +0530, Jagdish Gediya wrote:
> Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
> to false, "true" or "1" to true are currently handled through strncmp
> at multiple places. Use kstrtobool() to convert sysfs input to bool
> value.
> 
> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Andy Shevchenko]

On Tue, Apr 26, 2022 at 10:30:40PM +0530, Jagdish Gediya wrote:
> Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
> to false, "true" or "1" to true are currently handled through strncmp
> at multiple places. Use kstrtobool() to convert sysfs input to bool
> value.

...

> +	if (kstrtobool(buf, &numa_demotion_enabled))
>  		return -EINVAL;

Hmm... The commit message doesn't explain what's wrong with the error codes
returned by kstrtobool(). Can't it be

	ret = kstrtobool();
	if (ret)
		return ret;

?

...

> +	if (kstrtobool(buf, &enable_vma_readahead))
>  		return -EINVAL;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Andrew Morton]

On Wed, 27 Apr 2022 18:17:29 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Apr 26, 2022 at 10:30:40PM +0530, Jagdish Gediya wrote:
> > Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
> > to false, "true" or "1" to true are currently handled through strncmp
> > at multiple places. Use kstrtobool() to convert sysfs input to bool
> > value.
> 
> ...
> 
> > +	if (kstrtobool(buf, &numa_demotion_enabled))
> >  		return -EINVAL;
> 
> Hmm... The commit message doesn't explain what's wrong with the error codes
> returned by kstrtobool(). Can't it be
> 
> 	ret = kstrtobool();
> 	if (ret)
> 		return ret;
> 
> ?

Jagdish fell asleep.

Yes, I agree.  It has no practical difference at present because
kstrtobool() can only return 0 or -EINVAL.  I did this:

--- a/mm/migrate.c~mm-convert-sysfs-input-to-bool-using-kstrtobool-fix
+++ a/mm/migrate.c
@@ -2523,8 +2523,10 @@ static ssize_t numa_demotion_enabled_sto
 					   struct kobj_attribute *attr,
 					   const char *buf, size_t count)
 {
-	if (kstrtobool(buf, &numa_demotion_enabled))
-		return -EINVAL;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &numa_demotion_enabled);
+		return ret;
 
 	return count;
 }
--- a/mm/swap_state.c~mm-convert-sysfs-input-to-bool-using-kstrtobool-fix
+++ a/mm/swap_state.c
@@ -874,8 +874,11 @@ static ssize_t vma_ra_enabled_store(stru
 				      struct kobj_attribute *attr,
 				      const char *buf, size_t count)
 {
-	if (kstrtobool(buf, &enable_vma_readahead))
-		return -EINVAL;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &enable_vma_readahead);
+	if (ret)
+		return ret;
 
 	return count;
 }
_

Re: [PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Alexey Dobriyan]

On Thu, May 12, 2022 at 06:05:37PM -0700, Andrew Morton wrote:
> On Wed, 27 Apr 2022 18:17:29 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Tue, Apr 26, 2022 at 10:30:40PM +0530, Jagdish Gediya wrote:
> > > Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
> > > to false, "true" or "1" to true are currently handled through strncmp
> > > at multiple places. Use kstrtobool() to convert sysfs input to bool
> > > value.
> > 
> > ...
> > 
> > > +	if (kstrtobool(buf, &numa_demotion_enabled))
> > >  		return -EINVAL;
> > 
> > Hmm... The commit message doesn't explain what's wrong with the error codes
> > returned by kstrtobool(). Can't it be
> > 
> > 	ret = kstrtobool();
> > 	if (ret)
> > 		return ret;
> > 
> > ?
> 
> Jagdish fell asleep.
> 
> Yes, I agree.  It has no practical difference at present because
> kstrtobool() can only return 0 or -EINVAL.  I did this:

	ret = f();
	if (ret)
		return ret;

is slightly better because it doesn't require to load EINVAL into
register:

	call f
	test	eax, eax
	js	1f
	...
1:
	ret

Re: [PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Jagdish Gediya]

On Wed, Apr 27, 2022 at 06:17:29PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 26, 2022 at 10:30:40PM +0530, Jagdish Gediya wrote:
> > Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
> > to false, "true" or "1" to true are currently handled through strncmp
> > at multiple places. Use kstrtobool() to convert sysfs input to bool
> > value.
> 
> ...
> 
> > +	if (kstrtobool(buf, &numa_demotion_enabled))
> >  		return -EINVAL;
> 
> Hmm... The commit message doesn't explain what's wrong with the error codes
> returned by kstrtobool(). Can't it be
> 
> 	ret = kstrtobool();
> 	if (ret)
> 		return ret;
> 
> ?
Sorry for the late reply, I was away from work for few days. Yes, It can
be like what you mentioned.
> ...
> 
> > +	if (kstrtobool(buf, &enable_vma_readahead))
> >  		return -EINVAL;
> 
> Ditto.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH v2 2/2] mm: Covert sysfs input to bool using kstrtobool()

[Jagdish Gediya]

On Thu, May 12, 2022 at 06:05:37PM -0700, Andrew Morton wrote:
> On Wed, 27 Apr 2022 18:17:29 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Tue, Apr 26, 2022 at 10:30:40PM +0530, Jagdish Gediya wrote:
> > > Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
> > > to false, "true" or "1" to true are currently handled through strncmp
> > > at multiple places. Use kstrtobool() to convert sysfs input to bool
> > > value.
> > 
> > ...
> > 
> > > +	if (kstrtobool(buf, &numa_demotion_enabled))
> > >  		return -EINVAL;
> > 
> > Hmm... The commit message doesn't explain what's wrong with the error codes
> > returned by kstrtobool(). Can't it be
> > 
> > 	ret = kstrtobool();
> > 	if (ret)
> > 		return ret;
> > 
> > ?
> 
> Jagdish fell asleep.
Sorry, I was away from work for few days.
> Yes, I agree.  It has no practical difference at present because
> kstrtobool() can only return 0 or -EINVAL.  I did this:
> 
> --- a/mm/migrate.c~mm-convert-sysfs-input-to-bool-using-kstrtobool-fix
> +++ a/mm/migrate.c
> @@ -2523,8 +2523,10 @@ static ssize_t numa_demotion_enabled_sto
>  					   struct kobj_attribute *attr,
>  					   const char *buf, size_t count)
>  {
> -	if (kstrtobool(buf, &numa_demotion_enabled))
> -		return -EINVAL;
> +	ssize_t ret;
> +
> +	ret = kstrtobool(buf, &numa_demotion_enabled);
> +		return ret;
>  
>  	return count;
>  }
> --- a/mm/swap_state.c~mm-convert-sysfs-input-to-bool-using-kstrtobool-fix
> +++ a/mm/swap_state.c
> @@ -874,8 +874,11 @@ static ssize_t vma_ra_enabled_store(stru
>  				      struct kobj_attribute *attr,
>  				      const char *buf, size_t count)
>  {
> -	if (kstrtobool(buf, &enable_vma_readahead))
> -		return -EINVAL;
> +	ssize_t ret;
> +
> +	ret = kstrtobool(buf, &enable_vma_readahead);
> +	if (ret)
> +		return ret;
>  
>  	return count;
>  }
> _
>