2022-04-27 10:10:51

by Jagdish Gediya

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

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 <[email protected]>
---
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


2022-04-27 10:37:21

by Jagdish Gediya

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

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 <[email protected]>
---
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

2022-04-27 11:29:39

by Matthew Wilcox

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

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 <[email protected]>

Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>

2022-04-27 16:12:30

by Andy Shevchenko

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

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


2022-05-14 01:19:24

by Andrew Morton

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

On Wed, 27 Apr 2022 18:17:29 +0300 Andy Shevchenko <[email protected]> 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;
}
_


2022-05-14 03:38:10

by Alexey Dobriyan

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

On Thu, May 12, 2022 at 06:05:37PM -0700, Andrew Morton wrote:
> On Wed, 27 Apr 2022 18:17:29 +0300 Andy Shevchenko <[email protected]> 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

2022-05-16 19:39:35

by Jagdish Gediya

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

On Thu, May 12, 2022 at 06:05:37PM -0700, Andrew Morton wrote:
> On Wed, 27 Apr 2022 18:17:29 +0300 Andy Shevchenko <[email protected]> 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;
> }
> _
>

2022-05-16 22:48:30

by Jagdish Gediya

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

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