* [PATCH v1] kernel.h: Update comment about simple_strto<foo>() functions @ 2018-02-26 17:55 Andy Shevchenko 2018-02-26 22:31 ` Miguel Ojeda 0 siblings, 1 reply; 4+ messages in thread From: Andy Shevchenko @ 2018-02-26 17:55 UTC (permalink / raw) To: Andrew Morton, linux-kernel, Miguel Ojeda, Geert Uytterhoeven, Willy Tarreau Cc: Andy Shevchenko There were discussions in the past about use cases for simple_strto<foo>() functions and in some rare cases they have a benefit on kstrto<foo>() ones. Update a comment to reduce confusing about special use cases. Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- include/linux/kernel.h | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 207b0702fad1..07ad6f36b17d 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -326,8 +326,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res); * @res: Where to write the result of the conversion on success. * * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. - * Used as a replacement for the obsolete simple_strtoull. Return code must - * be checked. + * Used as a replacement for the simple_strtoull. Return code must be checked. */ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res) { @@ -355,8 +354,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign * @res: Where to write the result of the conversion on success. * * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. - * Used as a replacement for the obsolete simple_strtoull. Return code must - * be checked. + * Used as a replacement for the simple_strtoull. Return code must be checked. */ static inline int __must_check kstrtol(const char *s, unsigned int base, long *res) { @@ -432,7 +430,15 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t return kstrtoint_from_user(s, count, base, res); } -/* Obsolete, do not use. Use kstrto<foo> instead */ +/* + * Use kstrto<foo> instead. + * + * NOTE: The simple_strto<foo> do not check for overflow and depending + * on the input may give interesting results. + * + * Use these functions if and only if the code will need in place + * conversion and otherwise looks very ugly. Keep in mind above caveat. + */ extern unsigned long simple_strtoul(const char *,char **,unsigned int); extern long simple_strtol(const char *,char **,unsigned int); -- 2.16.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] kernel.h: Update comment about simple_strto<foo>() functions 2018-02-26 17:55 [PATCH v1] kernel.h: Update comment about simple_strto<foo>() functions Andy Shevchenko @ 2018-02-26 22:31 ` Miguel Ojeda 2018-02-27 11:40 ` Andy Shevchenko 0 siblings, 1 reply; 4+ messages in thread From: Miguel Ojeda @ 2018-02-26 22:31 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, linux-kernel, Geert Uytterhoeven, Willy Tarreau On Mon, Feb 26, 2018 at 6:55 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > There were discussions in the past about use cases for > simple_strto<foo>() functions and in some rare cases they have a benefit > on kstrto<foo>() ones. > > Update a comment to reduce confusing about special use cases. > > Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> I am not sure we should just go back to the old ones, though. Maybe it is better to create a new set of kstrto*_inplace() or some other name, safer than the old ones and following kstrto*()'s interface regarding returned errors, overflow checking, etc. There are two variations that can be useful: * A strict version taking a (start, end) range or (start, size) pair which contains the number to be converted. If there is any problem parsing it (e.g. invalid characters, extra characters, ...), fail. * A less strict version taking an extra end pointer (or size parameter) which is not allowed to be surpassed, and any non-digit character means successful stop. The old behavior (simple_*()) can still be achieved (almost) with the second version with an "infinite" end pointer if one really needs it. In any case, if you want to go forward with the old ones, we would also have to change the comments inside lib/vsprintf.c and possibly checkpatch :-) Cheers, Miguel > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/kernel.h | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 207b0702fad1..07ad6f36b17d 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -326,8 +326,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res); > * @res: Where to write the result of the conversion on success. > * > * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. > - * Used as a replacement for the obsolete simple_strtoull. Return code must > - * be checked. > + * Used as a replacement for the simple_strtoull. Return code must be checked. > */ > static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res) > { > @@ -355,8 +354,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign > * @res: Where to write the result of the conversion on success. > * > * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. > - * Used as a replacement for the obsolete simple_strtoull. Return code must > - * be checked. > + * Used as a replacement for the simple_strtoull. Return code must be checked. > */ > static inline int __must_check kstrtol(const char *s, unsigned int base, long *res) > { > @@ -432,7 +430,15 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t > return kstrtoint_from_user(s, count, base, res); > } > > -/* Obsolete, do not use. Use kstrto<foo> instead */ > +/* > + * Use kstrto<foo> instead. > + * > + * NOTE: The simple_strto<foo> do not check for overflow and depending > + * on the input may give interesting results. > + * > + * Use these functions if and only if the code will need in place > + * conversion and otherwise looks very ugly. Keep in mind above caveat. > + */ > > extern unsigned long simple_strtoul(const char *,char **,unsigned int); > extern long simple_strtol(const char *,char **,unsigned int); > -- > 2.16.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] kernel.h: Update comment about simple_strto<foo>() functions 2018-02-26 22:31 ` Miguel Ojeda @ 2018-02-27 11:40 ` Andy Shevchenko 2018-02-27 15:21 ` Miguel Ojeda 0 siblings, 1 reply; 4+ messages in thread From: Andy Shevchenko @ 2018-02-27 11:40 UTC (permalink / raw) To: Miguel Ojeda Cc: Andrew Morton, linux-kernel, Geert Uytterhoeven, Willy Tarreau On Mon, 2018-02-26 at 23:31 +0100, Miguel Ojeda wrote: > On Mon, Feb 26, 2018 at 6:55 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > There were discussions in the past about use cases for > > simple_strto<foo>() functions and in some rare cases they have a > > benefit > > on kstrto<foo>() ones. > > > > Update a comment to reduce confusing about special use cases. > > > > Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > > I am not sure we should just go back to the old ones, though. I didn't tell that we should. The niche of kstrto*() and simple_strto*() is different. > Maybe it > is better to create a new set of kstrto*_inplace() or some other name, > safer than the old ones and following kstrto*()'s interface regarding > returned errors, overflow checking, etc. There are two variations that > can be useful: > > * A strict version taking a (start, end) range or (start, size) pair > which contains the number to be converted. If there is any problem > parsing it (e.g. invalid characters, extra characters, ...), fail. > > * A less strict version taking an extra end pointer (or size > parameter) which is not allowed to be surpassed, and any non-digit > character means successful stop. Send a patch, we will discuss that for sure. > The old behavior (simple_*()) can still be achieved (almost) with the > second version with an "infinite" end pointer if one really needs it. > In any case, if you want to go forward with the old ones, we would > also have to change the comments inside lib/vsprintf.c and possibly > checkpatch :-) Feel free to amend. I actually didn't get your position here. You rather going to keep ugly code in your subsystem because of "official" comment than do it in more cleaner, but old fashion way. Btw, you can still weakly (based on power of base) detect an overflow by checking a returned pointer from simple_strto*(). -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] kernel.h: Update comment about simple_strto<foo>() functions 2018-02-27 11:40 ` Andy Shevchenko @ 2018-02-27 15:21 ` Miguel Ojeda 0 siblings, 0 replies; 4+ messages in thread From: Miguel Ojeda @ 2018-02-27 15:21 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, linux-kernel, Geert Uytterhoeven, Willy Tarreau On Tue, Feb 27, 2018 at 12:40 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, 2018-02-26 at 23:31 +0100, Miguel Ojeda wrote: >> On Mon, Feb 26, 2018 at 6:55 PM, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > There were discussions in the past about use cases for >> > simple_strto<foo>() functions and in some rare cases they have a >> > benefit >> > on kstrto<foo>() ones. >> > >> > Update a comment to reduce confusing about special use cases. >> > >> > Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> >> >> I am not sure we should just go back to the old ones, though. > > I didn't tell that we should. > The niche of kstrto*() and simple_strto*() is different. I meant that I am not sure that we should just re-allow the old ones (i.e. I didn't mean that you meant to remove the kstrto*() ones :-). > >> Maybe it >> is better to create a new set of kstrto*_inplace() or some other name, >> safer than the old ones and following kstrto*()'s interface regarding >> returned errors, overflow checking, etc. There are two variations that >> can be useful: >> >> * A strict version taking a (start, end) range or (start, size) pair >> which contains the number to be converted. If there is any problem >> parsing it (e.g. invalid characters, extra characters, ...), fail. >> >> * A less strict version taking an extra end pointer (or size >> parameter) which is not allowed to be surpassed, and any non-digit >> character means successful stop. > > Send a patch, we will discuss that for sure. > >> The old behavior (simple_*()) can still be achieved (almost) with the >> second version with an "infinite" end pointer if one really needs it. > >> In any case, if you want to go forward with the old ones, we would >> also have to change the comments inside lib/vsprintf.c and possibly >> checkpatch :-) > > Feel free to amend. > > I actually didn't get your position here. You rather going to keep ugly > code in your subsystem because of "official" comment than do it in more > cleaner, but old fashion way. I am not going to keep ugly code (why would you say so? It is not even accepted yet); but I am not adding calls to deprecated functions either. If those functions shouldn't be deprecated, fine; but *that* is what should be discussed/changed. Adding new calls to functions that were deprecated years ago (and currently are in HEAD) does not help anybody. > > Btw, you can still weakly (based on power of base) detect an overflow by > checking a returned pointer from simple_strto*(). Sure, but fixing that interface is precisely one of the reasons simple_strto*() were deprecated to begin with. Cheers, Miguel > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-27 15:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-26 17:55 [PATCH v1] kernel.h: Update comment about simple_strto<foo>() functions Andy Shevchenko 2018-02-26 22:31 ` Miguel Ojeda 2018-02-27 11:40 ` Andy Shevchenko 2018-02-27 15:21 ` Miguel Ojeda
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).