* [RFC PATCH 0/3] Introduce kstrtobool function
@ 2011-03-23 13:39 Jonathan Cameron
2011-03-23 13:39 ` [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents Jonathan Cameron
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Jonathan Cameron @ 2011-03-23 13:39 UTC (permalink / raw)
To: linux-kernel; +Cc: greg, rusty, adobriyan, 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
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 13:39 [RFC PATCH 0/3] Introduce kstrtobool function Jonathan Cameron @ 2011-03-23 13:39 ` Jonathan Cameron 2011-03-23 13:54 ` David Sterba 2011-03-23 15:30 ` Alexey Dobriyan 2011-03-23 13:39 ` [PATCH 2/3] debugfs: move to new kstrtobool function Jonathan Cameron ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 13:39 UTC (permalink / raw) To: linux-kernel; +Cc: greg, rusty, adobriyan, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 13:39 ` [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents Jonathan Cameron @ 2011-03-23 13:54 ` David Sterba 2011-03-23 14:00 ` Jonathan Cameron 2011-03-23 15:30 ` Alexey Dobriyan 1 sibling, 1 reply; 17+ messages in thread From: David Sterba @ 2011-03-23 13:54 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-kernel, greg, rusty, adobriyan 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/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 13:54 ` David Sterba @ 2011-03-23 14:00 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 14:00 UTC (permalink / raw) To: linux-kernel, greg, rusty, adobriyan 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/ > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 13:39 ` [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents Jonathan Cameron 2011-03-23 13:54 ` David Sterba @ 2011-03-23 15:30 ` Alexey Dobriyan 2011-03-23 16:01 ` Greg KH 1 sibling, 1 reply; 17+ messages in thread From: Alexey Dobriyan @ 2011-03-23 15:30 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-kernel, greg, rusty 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 15:30 ` Alexey Dobriyan @ 2011-03-23 16:01 ` Greg KH 2011-03-23 16:15 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: Greg KH @ 2011-03-23 16:01 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: Jonathan Cameron, linux-kernel, rusty 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 16:01 ` Greg KH @ 2011-03-23 16:15 ` Jonathan Cameron 2011-03-23 16:23 ` Greg KH 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 16:15 UTC (permalink / raw) To: Greg KH; +Cc: Alexey Dobriyan, linux-kernel, rusty 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 16:15 ` Jonathan Cameron @ 2011-03-23 16:23 ` Greg KH 2011-03-23 16:37 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: Greg KH @ 2011-03-23 16:23 UTC (permalink / raw) To: Jonathan Cameron; +Cc: Alexey Dobriyan, linux-kernel, rusty 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 16:23 ` Greg KH @ 2011-03-23 16:37 ` Jonathan Cameron 2011-03-23 18:44 ` Greg KH 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 16:37 UTC (permalink / raw) To: Greg KH; +Cc: Alexey Dobriyan, linux-kernel, rusty 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 16:37 ` Jonathan Cameron @ 2011-03-23 18:44 ` Greg KH 2011-03-23 19:11 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: Greg KH @ 2011-03-23 18:44 UTC (permalink / raw) To: Jonathan Cameron; +Cc: Alexey Dobriyan, linux-kernel, rusty 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents. 2011-03-23 18:44 ` Greg KH @ 2011-03-23 19:11 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 19:11 UTC (permalink / raw) To: Greg KH; +Cc: Alexey Dobriyan, linux-kernel, rusty 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] debugfs: move to new kstrtobool function 2011-03-23 13:39 [RFC PATCH 0/3] Introduce kstrtobool function Jonathan Cameron 2011-03-23 13:39 ` [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents Jonathan Cameron @ 2011-03-23 13:39 ` Jonathan Cameron 2011-03-23 20:20 ` Ryan Mallon 2011-03-23 13:39 ` [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs Jonathan Cameron 2011-03-24 0:34 ` [RFC PATCH 0/3] Introduce kstrtobool function Rusty Russell 3 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 13:39 UTC (permalink / raw) To: linux-kernel; +Cc: greg, rusty, adobriyan, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] debugfs: move to new kstrtobool function 2011-03-23 13:39 ` [PATCH 2/3] debugfs: move to new kstrtobool function Jonathan Cameron @ 2011-03-23 20:20 ` Ryan Mallon 2011-03-24 10:53 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: Ryan Mallon @ 2011-03-23 20:20 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-kernel, greg, rusty, adobriyan 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] debugfs: move to new kstrtobool function 2011-03-23 20:20 ` Ryan Mallon @ 2011-03-24 10:53 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2011-03-24 10:53 UTC (permalink / raw) To: Ryan Mallon; +Cc: linux-kernel, greg, rusty, adobriyan 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs. 2011-03-23 13:39 [RFC PATCH 0/3] Introduce kstrtobool function Jonathan Cameron 2011-03-23 13:39 ` [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents Jonathan Cameron 2011-03-23 13:39 ` [PATCH 2/3] debugfs: move to new kstrtobool function Jonathan Cameron @ 2011-03-23 13:39 ` Jonathan Cameron 2011-03-23 13:42 ` Jonathan Cameron 2011-03-24 0:34 ` [RFC PATCH 0/3] Introduce kstrtobool function Rusty Russell 3 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 13:39 UTC (permalink / raw) To: linux-kernel; +Cc: greg, rusty, adobriyan, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs. 2011-03-23 13:39 ` [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs Jonathan Cameron @ 2011-03-23 13:42 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2011-03-23 13:42 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-kernel, greg, rusty, adobriyan 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; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 0/3] Introduce kstrtobool function 2011-03-23 13:39 [RFC PATCH 0/3] Introduce kstrtobool function Jonathan Cameron ` (2 preceding siblings ...) 2011-03-23 13:39 ` [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs Jonathan Cameron @ 2011-03-24 0:34 ` Rusty Russell 3 siblings, 0 replies; 17+ messages in thread From: Rusty Russell @ 2011-03-24 0:34 UTC (permalink / raw) To: Jonathan Cameron, linux-kernel; +Cc: greg, adobriyan, Jonathan Cameron 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-03-24 10:52 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-23 13:39 [RFC PATCH 0/3] Introduce kstrtobool function Jonathan Cameron 2011-03-23 13:39 ` [PATCH 1/3] Add a kstrtobool function matching semantics of existing in kernel equivalents Jonathan Cameron 2011-03-23 13:54 ` David Sterba 2011-03-23 14:00 ` Jonathan Cameron 2011-03-23 15:30 ` Alexey Dobriyan 2011-03-23 16:01 ` Greg KH 2011-03-23 16:15 ` Jonathan Cameron 2011-03-23 16:23 ` Greg KH 2011-03-23 16:37 ` Jonathan Cameron 2011-03-23 18:44 ` Greg KH 2011-03-23 19:11 ` Jonathan Cameron 2011-03-23 13:39 ` [PATCH 2/3] debugfs: move to new kstrtobool function Jonathan Cameron 2011-03-23 20:20 ` Ryan Mallon 2011-03-24 10:53 ` Jonathan Cameron 2011-03-23 13:39 ` [PATCH 3/3] params.c: Use new kstrtobool implementation to processing boolean inputs Jonathan Cameron 2011-03-23 13:42 ` Jonathan Cameron 2011-03-24 0:34 ` [RFC PATCH 0/3] Introduce kstrtobool function Rusty Russell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).