[RFC PATCH 0/3 V2] Introduce usr_strtobool (previously kstrtobool)

[RFC PATCH 0/3 V2] Introduce usr_strtobool (previously kstrtobool)

[Jonathan Cameron]

Hi all,

Here is a second pass at introducing a new function to unify
code that is attempting to get a boolean value from user input strings.

The first attempt (other than having some stupid bugs) was opposed by
Alexy Dobriyan on the basis that it did completely insufficient checking
on the string.  Given that under the original proposed name it was
associated with the other kstrto* functions it was reasonable to assume
if would be as strict as they are.  Hence the name change to remove
an implication of this.

The use cases are both the pair below and the numerous boolean
attributes in sysfs.  It's for these that I'm personally interested
in having such a function, but as Greg pointed out a good starting
point is to unify the places where this is already occuring.

The big questions to my mind are:

1) Is the usr_strtobool name a good choice?
2) Should we introduce other acceptable boolean inputs?
   Clearly there are issues in changing the list as it will at least
   in theory change the two api's effected by this series.

Thanks,

Jonathan

Jonathan Cameron (3):
  Add a usr_strtobool function matching semantics of existing in kernel
    equivalents
  debugfs: move to new usr_strtobool
  params.c: Use new usr_strtobool function to process boolean inputs

 fs/debugfs/file.c      |   20 ++++++--------------
 include/linux/string.h |    1 +
 kernel/params.c        |   14 ++++----------
 lib/string.c           |   29 +++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 24 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] Add a usr_strtobool function matching semantics of existing in kernel equivalents

[Jonathan Cameron]

This is a renamed and relocated fixed version of previous kstrtobool RFC

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 include/linux/string.h |    1 +
 lib/string.c           |   29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index a716ee2..1977cc0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -123,6 +123,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 int usr_strtobool(const char *s, bool *res);
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index f71bead..121bd65 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -535,6 +535,35 @@ bool sysfs_streq(const char *s1, const char *s2)
 }
 EXPORT_SYMBOL(sysfs_streq);
 
+/**
+ * usr_strtobool - convert common user inputs into boolean values
+ * @s: input string
+ * @res: result
+ *
+ * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
+ * Otherwise it will return -EINVAL.  Value pointed to by res is
+ * updated upon finding a match.
+ */
+int usr_strtobool(const char *s, bool *res)
+{
+	switch (s[0]) {
+	case 'y':
+	case 'Y':
+	case '1':
+		*res = true;
+		break;
+	case 'n':
+	case 'N':
+	case '0':
+		*res = false;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(usr_strtobool);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value
-- 
1.7.3.4

[PATCH 2/3] debugfs: move to new usr_strtobool

[Jonathan Cameron]

No functional chagnes requires that we eat errors from usr_strtobool.
If people want to not do this, then it should be at a later date.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 fs/debugfs/file.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 89d394d..1f0e3ef 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -428,26 +428,18 @@ static ssize_t write_file_bool(struct file *file, const char __user *user_buf,
 			       size_t count, loff_t *ppos)
 {
 	char buf[32];
-	int buf_size;
+	int buf_size, ret;
+	bool bv;
 	u32 *val = file->private_data;
 
 	buf_size = min(count, (sizeof(buf)-1));
 	if (copy_from_user(buf, user_buf, buf_size))
 		return -EFAULT;
 
-	switch (buf[0]) {
-	case 'y':
-	case 'Y':
-	case '1':
-		*val = 1;
-		break;
-	case 'n':
-	case 'N':
-	case '0':
-		*val = 0;
-		break;
-	}
-	
+	ret = usr_strtobool(buf, &bv);
+	if (!ret)
+		*val = bv;
+
 	return count;
 }
 
-- 
1.7.3.4

[PATCH 3/3] params.c: Use new usr_strtobool function to process boolean inputs

[Jonathan Cameron]

No functional changes.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 kernel/params.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 0da1411..49d15fe 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -297,21 +297,15 @@ EXPORT_SYMBOL(param_ops_charp);
 int param_set_bool(const char *val, const struct kernel_param *kp)
 {
 	bool v;
+	int ret;
 
 	/* No equals means "set"... */
 	if (!val) val = "1";
 
 	/* One of =[yYnN01] */
-	switch (val[0]) {
-	case 'y': case 'Y': case '1':
-		v = true;
-		break;
-	case 'n': case 'N': case '0':
-		v = false;
-		break;
-	default:
-		return -EINVAL;
-	}
+	ret = usr_strtobool(val, &v);
+	if (ret)
+		return ret;
 
 	if (kp->flags & KPARAM_ISBOOL)
 		*(bool *)kp->arg = v;
-- 
1.7.3.4

Re: [RFC PATCH 0/3 V2] Introduce usr_strtobool (previously kstrtobool)

[Jonathan Cameron]

If no one has any comments on this, who is likely to pick it up?
> Here is a second pass at introducing a new function to unify
> code that is attempting to get a boolean value from user input strings.
> 
> The first attempt (other than having some stupid bugs) was opposed by
> Alexy Dobriyan on the basis that it did completely insufficient checking
> on the string.  Given that under the original proposed name it was
> associated with the other kstrto* functions it was reasonable to assume
> if would be as strict as they are.  Hence the name change to remove
> an implication of this.
> 
> The use cases are both the pair below and the numerous boolean
> attributes in sysfs.  It's for these that I'm personally interested
> in having such a function, but as Greg pointed out a good starting
> point is to unify the places where this is already occuring.
> 
> The big questions to my mind are:
> 
> 1) Is the usr_strtobool name a good choice?
> 2) Should we introduce other acceptable boolean inputs?
>    Clearly there are issues in changing the list as it will at least
>    in theory change the two api's effected by this series.
> 
> Thanks,
> 
> Jonathan
> 
> Jonathan Cameron (3):
>   Add a usr_strtobool function matching semantics of existing in kernel
>     equivalents
>   debugfs: move to new usr_strtobool
>   params.c: Use new usr_strtobool function to process boolean inputs
> 
>  fs/debugfs/file.c      |   20 ++++++--------------
>  include/linux/string.h |    1 +
>  kernel/params.c        |   14 ++++----------
>  lib/string.c           |   29 +++++++++++++++++++++++++++++
>  4 files changed, 40 insertions(+), 24 deletions(-)
> 

Re: [PATCH 1/3] Add a usr_strtobool function matching semantics of existing in kernel equivalents

[Rusty Russell]

On Thu, 24 Mar 2011 13:23:43 +0000, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> This is a renamed and relocated fixed version of previous kstrtobool RFC

Please just call it strtobool, usr_ implies it's a user pointer.

And there's no need to introduce a new var in debugfs, just do:

	if (strtobool(buf, &bv) == 0)
		*val = bv;

And yes, I'll take it for the param stuff.

Thanks,
Rusty.

Re: [PATCH 1/3] Add a usr_strtobool function matching semantics of existing in kernel equivalents

[Jonathan Cameron]

On 04/19/11 02:36, Rusty Russell wrote:
> On Thu, 24 Mar 2011 13:23:43 +0000, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>> This is a renamed and relocated fixed version of previous kstrtobool RFC
> 
> Please just call it strtobool, usr_ implies it's a user pointer.
Will do. I guess that is still distinct enough from kstrto* to avoid
anyone expecting the semantics from this one.
> 
> And there's no need to introduce a new var in debugfs, just do:
> 
> 	if (strtobool(buf, &bv) == 0)
> 		*val = bv;
Excellent point.
> 
> And yes, I'll take it for the param stuff.
Thanks. New version coming shortly.
> 
> Thanks,
> Rusty.