[RFC PATCH 0/3] Introduce kstrtobool function

[RFC PATCH 0/3] Introduce kstrtobool function

[Jonathan Cameron]

Dear all,

This is an initial stab at a simple utility function intended to pin
down what the kernel considers to be 'true' and 'false'.  My original
query was centered around processing of sysfs inputs but as Greg
pointed out there are a couple of other places in kernel where this
functionality would be useful.

Given my main targets are in staging:iio I will provide patches for them
if/when people are happy with what we have here.

I obviously need to add some documentation, but want to get some
quick opinions on whether this is a good idea.

If the concept is fine the obvious questions are:

1) Function naming - this is only tangentially similar to kstrto* so
   is that a sensible location
2) Other values people think should be true or false.

Thanks,

Jonathan

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

 fs/debugfs/file.c      |   19 ++++++-------------
 include/linux/kernel.h |    1 +
 kernel/params.c        |   15 ++++-----------
 lib/kstrtox.c          |   18 ++++++++++++++++++
 4 files changed, 29 insertions(+), 24 deletions(-)

-- 
1.7.3.4

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

[Jonathan Cameron]

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

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 00cec4d..0babad3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -247,6 +247,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
 int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
 int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
 int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
+int __must_check kstrtobool(const char *s, bool *res);
 
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);
 extern long simple_strtol(const char *,char **,unsigned int);
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 05672e8..84cf08f 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -225,4 +225,21 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
 	return 0;
 }
 EXPORT_SYMBOL(kstrtos8);
+
+int kstrtobool(const char *s, bool *res)
+{
+	switch (s[0]) {
+	case 'y':
+	case 'Y':
+	case '1':
+		*res = true;
+	case 'n':
+	case 'N':
+	case '0':
+		*res = false;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(kstrtobool);
-- 
1.7.3.4

[PATCH 2/3] debugfs: move to new kstrtobool function

[Jonathan Cameron]

No functional changes requires that we eat errors from
kstrtobool.  Note *val is still only updated if a valid
input is found.

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

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 89d394d..fed4485 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -429,25 +429,18 @@ static ssize_t write_file_bool(struct file *file, const char __user *user_buf,
 {
 	char buf[32];
 	int buf_size;
+	int 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 = kstrtobool(buf, &bv);
+	if (!ret)
+		*val = bv;
+
 	return count;
 }
 
-- 
1.7.3.4

[PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs.

[Jonathan Cameron]

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

diff --git a/kernel/params.c b/kernel/params.c
index 0da1411..37b4d83 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -297,3 +297,4 @@ 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 = kstrtobool(val, &v);
+	if (ret)
+		return ret;
 
 	if (kp->flags & KPARAM_ISBOOL)
 		*(bool *)kp->arg = v;
-- 
1.7.3.4

Re: [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs.

[Jonathan Cameron]

Oops. The patch title should probably make sense.

On 03/23/11 13:39, Jonathan Cameron wrote:
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  kernel/params.c |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index 0da1411..37b4d83 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -297,3 +297,4 @@ 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 = kstrtobool(val, &v);
> +	if (ret)
> +		return ret;
>  
>  	if (kp->flags & KPARAM_ISBOOL)
>  		*(bool *)kp->arg = v;

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

[David Sterba]

Hi,

you've missed a few breaks :)

On Wed, Mar 23, 2011 at 01:39:12PM +0000, Jonathan Cameron wrote:
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  include/linux/kernel.h |    1 +
>  lib/kstrtox.c          |   18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 00cec4d..0babad3 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -247,6 +247,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
>  int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
>  int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
>  int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
> +int __must_check kstrtobool(const char *s, bool *res);
>  
>  extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>  extern long simple_strtol(const char *,char **,unsigned int);
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 05672e8..84cf08f 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -225,4 +225,21 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
>  	return 0;
>  }
>  EXPORT_SYMBOL(kstrtos8);
> +
> +int kstrtobool(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(kstrtobool);
> -- 
> 1.7.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Jonathan Cameron]

On 03/23/11 13:54, David Sterba wrote:
> Hi,
> 
> you've missed a few breaks :)
Gah!  This really isn't going well. Lets just pretend I'm not an idiot and
those were there in the first place.  One day I'll remember to never
refactor (and not check) my code just before sending the emails out.

Thanks,
> 
> On Wed, Mar 23, 2011 at 01:39:12PM +0000, Jonathan Cameron wrote:
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>> ---
>>  include/linux/kernel.h |    1 +
>>  lib/kstrtox.c          |   18 ++++++++++++++++++
>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 00cec4d..0babad3 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -247,6 +247,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
>>  int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
>>  int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
>>  int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
>> +int __must_check kstrtobool(const char *s, bool *res);
>>  
>>  extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>>  extern long simple_strtol(const char *,char **,unsigned int);
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 05672e8..84cf08f 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -225,4 +225,21 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(kstrtos8);
>> +
>> +int kstrtobool(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(kstrtobool);
>> -- 
>> 1.7.3.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Alexey Dobriyan]

On Wed, Mar 23, 2011 at 3:39 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> +int kstrtobool(const char *s, bool *res)
> +{
> +       switch (s[0]) {
> +       case 'y':
> +       case 'Y':
> +       case '1':
> +               *res = true;
> +       case 'n':
> +       case 'N':
> +       case '0':
> +               *res = false;
> +       default:
> +               return -EINVAL;
> +       }
> +       return 0;
> +}

sigh... such simple thing and so many bugs

The only values such function should accept is 0 and 1.

Have you read the rest of kstrto*() code?
Where is newline check?

Anyway, I think it's better do not exist.

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

[Greg KH]

On Wed, Mar 23, 2011 at 05:30:11PM +0200, Alexey Dobriyan wrote:
> On Wed, Mar 23, 2011 at 3:39 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> > +int kstrtobool(const char *s, bool *res)
> > +{
> > +       switch (s[0]) {
> > +       case 'y':
> > +       case 'Y':
> > +       case '1':
> > +               *res = true;
> > +       case 'n':
> > +       case 'N':
> > +       case '0':
> > +               *res = false;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +       return 0;
> > +}
> 
> sigh... such simple thing and so many bugs
> 
> The only values such function should accept is 0 and 1.

Why?  That's not the way the existing kernel functions that use this
work.

> Have you read the rest of kstrto*() code?
> Where is newline check?
> 
> Anyway, I think it's better do not exist.

I think it is, as it's already duplicated in at least 2 different places
in the kernel, and probably more.  Once we get this implementation
working correctly, we don't need to rewrite it again.

thanks,

greg k-h

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

[Jonathan Cameron]

On 03/23/11 16:01, Greg KH wrote:
> On Wed, Mar 23, 2011 at 05:30:11PM +0200, Alexey Dobriyan wrote:
>> On Wed, Mar 23, 2011 at 3:39 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>>> +int kstrtobool(const char *s, bool *res)
>>> +{
>>> +       switch (s[0]) {
>>> +       case 'y':
>>> +       case 'Y':
>>> +       case '1':
>>> +               *res = true;
>>> +       case 'n':
>>> +       case 'N':
>>> +       case '0':
>>> +               *res = false;
>>> +       default:
>>> +               return -EINVAL;
>>> +       }
>>> +       return 0;
>>> +}
>>
>> sigh... such simple thing and so many bugs
Yeah, not by best work.
>>
>> The only values such function should accept is 0 and 1.
> 
> Why?  That's not the way the existing kernel functions that use this
> work.
> 
>> Have you read the rest of kstrto*() code?
>> Where is newline check?
There are plenty of nastier cases that get through than a newline
in the middle of the string (ybobsyouruncle -> 1 nyes->0 :)
>>
>> Anyway, I think it's better do not exist.
> 
> I think it is, as it's already duplicated in at least 2 different places
> in the kernel, and probably more.  Once we get this implementation
> working correctly, we don't need to rewrite it again.
Perhaps naming it like this is a bad idea.  It manages to imply that it
has the same level of strict checking which is seen in the other kstrto*
functions - which is self evidently not true!

The alternative is to try and pin down future interfaces to a narrower set
of 'true' and 'false' values.  We can't realistically change this pair,
(even to 'fix' them) but maybe we can ensure future versions only take 0 or 1?
That sort of simple convention would make life simpler!

Jonathan

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

[Greg KH]

On Wed, Mar 23, 2011 at 04:15:51PM +0000, Jonathan Cameron wrote:
> On 03/23/11 16:01, Greg KH wrote:
> > On Wed, Mar 23, 2011 at 05:30:11PM +0200, Alexey Dobriyan wrote:
> >> On Wed, Mar 23, 2011 at 3:39 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> >>> +int kstrtobool(const char *s, bool *res)
> >>> +{
> >>> +       switch (s[0]) {
> >>> +       case 'y':
> >>> +       case 'Y':
> >>> +       case '1':
> >>> +               *res = true;
> >>> +       case 'n':
> >>> +       case 'N':
> >>> +       case '0':
> >>> +               *res = false;
> >>> +       default:
> >>> +               return -EINVAL;
> >>> +       }
> >>> +       return 0;
> >>> +}
> >>
> >> sigh... such simple thing and so many bugs
> Yeah, not by best work.
> >>
> >> The only values such function should accept is 0 and 1.
> > 
> > Why?  That's not the way the existing kernel functions that use this
> > work.
> > 
> >> Have you read the rest of kstrto*() code?
> >> Where is newline check?
> There are plenty of nastier cases that get through than a newline
> in the middle of the string (ybobsyouruncle -> 1 nyes->0 :)
> >>
> >> Anyway, I think it's better do not exist.
> > 
> > I think it is, as it's already duplicated in at least 2 different places
> > in the kernel, and probably more.  Once we get this implementation
> > working correctly, we don't need to rewrite it again.
> Perhaps naming it like this is a bad idea.  It manages to imply that it
> has the same level of strict checking which is seen in the other kstrto*
> functions - which is self evidently not true!

Ok, perhaps the name might need to be changed a bit, but the idea is
still good to have.  Please try again.

thanks,

greg k-h

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

[Jonathan Cameron]

On 03/23/11 16:23, Greg KH wrote:
> On Wed, Mar 23, 2011 at 04:15:51PM +0000, Jonathan Cameron wrote:
>> On 03/23/11 16:01, Greg KH wrote:
>>> On Wed, Mar 23, 2011 at 05:30:11PM +0200, Alexey Dobriyan wrote:
>>>> On Wed, Mar 23, 2011 at 3:39 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>>>>> +int kstrtobool(const char *s, bool *res)
>>>>> +{
>>>>> +       switch (s[0]) {
>>>>> +       case 'y':
>>>>> +       case 'Y':
>>>>> +       case '1':
>>>>> +               *res = true;
>>>>> +       case 'n':
>>>>> +       case 'N':
>>>>> +       case '0':
>>>>> +               *res = false;
>>>>> +       default:
>>>>> +               return -EINVAL;
>>>>> +       }
>>>>> +       return 0;
>>>>> +}
>>>>
>>>> sigh... such simple thing and so many bugs
>> Yeah, not by best work.
>>>>
>>>> The only values such function should accept is 0 and 1.
>>>
>>> Why?  That's not the way the existing kernel functions that use this
>>> work.
>>>
>>>> Have you read the rest of kstrto*() code?
>>>> Where is newline check?
>> There are plenty of nastier cases that get through than a newline
>> in the middle of the string (ybobsyouruncle -> 1 nyes->0 :)
>>>>
>>>> Anyway, I think it's better do not exist.
>>>
>>> I think it is, as it's already duplicated in at least 2 different places
>>> in the kernel, and probably more.  Once we get this implementation
>>> working correctly, we don't need to rewrite it again.
>> Perhaps naming it like this is a bad idea.  It manages to imply that it
>> has the same level of strict checking which is seen in the other kstrto*
>> functions - which is self evidently not true!
> 
> Ok, perhaps the name might need to be changed a bit, but the idea is
> still good to have.  Please try again.
> 
Any thoughts on what naming would work?  Nothing immediately comes to mind
which is why I ended up pinching the kstrto* naming...

reallysloppy_strtobool?
guessintent_strtobool?

or the old classic underscore prefix to 'encourage' people to check what it does
before using it

__strtobool?

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

[Greg KH]

On Wed, Mar 23, 2011 at 04:37:40PM +0000, Jonathan Cameron wrote:
> > Ok, perhaps the name might need to be changed a bit, but the idea is
> > still good to have.  Please try again.
> > 
> Any thoughts on what naming would work?  Nothing immediately comes to mind
> which is why I ended up pinching the kstrto* naming...
> 
> reallysloppy_strtobool?
> guessintent_strtobool?

usr_strtobool?  I don't care :)

> or the old classic underscore prefix to 'encourage' people to check what it does
> before using it
> 
> __strtobool?

Ick, no, that implies that no one outside of this file should call it.
Or it should be called with locks held or some such "special" thing.
Don't do that...

thanks,

greg k-h

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

[Jonathan Cameron]

On 03/23/11 18:44, Greg KH wrote:
> On Wed, Mar 23, 2011 at 04:37:40PM +0000, Jonathan Cameron wrote:
>>> Ok, perhaps the name might need to be changed a bit, but the idea is
>>> still good to have.  Please try again.
>>>
>> Any thoughts on what naming would work?  Nothing immediately comes to mind
>> which is why I ended up pinching the kstrto* naming...
>>
>> reallysloppy_strtobool?
>> guessintent_strtobool?
> 
> usr_strtobool?  I don't care :)
That works for me.  Anything usr related is clearly sloppy ;)
> 
>> or the old classic underscore prefix to 'encourage' people to check what it does
>> before using it
>>
>> __strtobool?
> 
> Ick, no, that implies that no one outside of this file should call it.
> Or it should be called with locks held or some such "special" thing.
> Don't do that...
 I may have gotten a smidgen flippant ;)
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 2/3] debugfs: move to new kstrtobool function

[Ryan Mallon]

On 03/24/2011 02:39 AM, Jonathan Cameron wrote:
> No functional changes requires that we eat errors from
> kstrtobool.  Note *val is still only updated if a valid
> input is found.
> 
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  fs/debugfs/file.c |   19 ++++++-------------
>  1 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 89d394d..fed4485 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -429,25 +429,18 @@ static ssize_t write_file_bool(struct file *file, const char __user *user_buf,
>  {
>  	char buf[32];
>  	int buf_size;
> +	int 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 = kstrtobool(buf, &bv);
> +	if (!ret)
> +		*val = bv;
> +
>  	return count;

Shouldn't this be:

	ret = kstrtobool(buf, &bv);
	if (ret)
		return ret;

	*val = bv;
	return count;

?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [RFC PATCH 0/3] Introduce kstrtobool function

[Rusty Russell]

On Wed, 23 Mar 2011 13:39:11 +0000, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> Dear all,
> 
> This is an initial stab at a simple utility function intended to pin
> down what the kernel considers to be 'true' and 'false'.  My original
> query was centered around processing of sysfs inputs but as Greg
> pointed out there are a couple of other places in kernel where this
> functionality would be useful.

Seems reasonable.

Rusty.

Re: [PATCH 2/3] debugfs: move to new kstrtobool function

[Jonathan Cameron]

On 03/23/11 20:20, Ryan Mallon wrote:
> On 03/24/2011 02:39 AM, Jonathan Cameron wrote:
>> No functional changes requires that we eat errors from
>> kstrtobool.  Note *val is still only updated if a valid
>> input is found.
>>
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>> ---
>>  fs/debugfs/file.c |   19 ++++++-------------
>>  1 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 89d394d..fed4485 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -429,25 +429,18 @@ static ssize_t write_file_bool(struct file *file, const char __user *user_buf,
>>  {
>>  	char buf[32];
>>  	int buf_size;
>> +	int 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 = kstrtobool(buf, &bv);
>> +	if (!ret)
>> +		*val = bv;
>> +
>>  	return count;
> 
> Shouldn't this be:
> 
> 	ret = kstrtobool(buf, &bv);
> 	if (ret)
> 		return ret;
> 
> 	*val = bv;
> 	return count;
> 
That is indeed what one would normally expect to see.
However, I think we want to maintain what is already happening in
the function and previously it never returned an error for
an invalid value.

Now that's not to say I'd be against a 'fix' for that issue, but
it should be in a separate patch series as it has nothing to do
with the use of this new function.

Jonathan