Re: [PATCH 1/6] lib: string: add function strtolower()
On 1 July 2016 at 14:08, Rasmus Villemoeswrote: > A few suggestions: > > - Make the function take separate src and dst parameters, making it explicitly > allowed to pass the same value (but not other kinds of overlap, of > course). That way one can avoid "strcpy(dst, src); strtolower(dst);". > > - Drop the NULL check. If someone does "foo->bar = something; > strtolower(foo->bar); put foo in a global data structure...", the > dereference of foo->bar may happen much later. Doing the NULL deref > sooner means it's much easier to find and fix the bug. (Also, other > str* and mem* functions don't usually check for NULL). > > - While it's true that strcpy and memcpy by definition return dst, that's > mostly useless. If you want it to return anything, please make it > something that might be used - for example, having stpcpy semantics > (returning a pointer to dst's terminating \0) means a caller might avoid > a strlen call. > > - Maybe do strtoupper while you're at it. Quick grepping didn't find any > use for the copy-while-lowercasing, but copy-while-uppercasing can at > least be used in drivers/acpi/acpica/nsrepair2.c, > drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c, > drivers/power/power_supply_sysfs.c along with a bunch of inplace > uppercasing. > > > Rasmus Thanks for the suggestions to you and Jani. Based on the feedback I received, I am reworking the series now and will post v2 probably tomorrow. Regards, -Markus -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] lib: string: add function strtolower()
On Fri, Jul 01 2016, Markus Mayerwrote: > Add a function called strtolower() to convert strings to lower case > in-place, overwriting the original string. > > This seems to be a recurring requirement in the kernel that is > currently being solved by several duplicated implementations doing the > same thing. > > Signed-off-by: Markus Mayer > --- > include/linux/string.h | 1 + > lib/string.c | 14 ++ > 2 files changed, 15 insertions(+) > > diff --git a/include/linux/string.h b/include/linux/string.h > index 26b6f6a..aad605e 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); > #endif > void *memchr_inv(const void *s, int c, size_t n); > char *strreplace(char *s, char old, char new); > +char *strtolower(char *s); > > extern void kfree_const(const void *x); > > diff --git a/lib/string.c b/lib/string.c > index ed83562..6e3b560 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) > return s; > } > EXPORT_SYMBOL(strreplace); > + > +char *strtolower(char *s) > +{ > + char *p; > + > +if (unlikely(!s)) > +return NULL; > + > + for (p = s; *p; p++) > + *p = tolower(*p); > + > + return s; > +} > +EXPORT_SYMBOL(strtolower); A few suggestions: - Make the function take separate src and dst parameters, making it explicitly allowed to pass the same value (but not other kinds of overlap, of course). That way one can avoid "strcpy(dst, src); strtolower(dst);". - Drop the NULL check. If someone does "foo->bar = something; strtolower(foo->bar); put foo in a global data structure...", the dereference of foo->bar may happen much later. Doing the NULL deref sooner means it's much easier to find and fix the bug. (Also, other str* and mem* functions don't usually check for NULL). - While it's true that strcpy and memcpy by definition return dst, that's mostly useless. If you want it to return anything, please make it something that might be used - for example, having stpcpy semantics (returning a pointer to dst's terminating \0) means a caller might avoid a strlen call. - Maybe do strtoupper while you're at it. Quick grepping didn't find any use for the copy-while-lowercasing, but copy-while-uppercasing can at least be used in drivers/acpi/acpica/nsrepair2.c, drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c, drivers/power/power_supply_sysfs.c along with a bunch of inplace uppercasing. Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] lib: string: add function strtolower()
On Fri, 01 Jul 2016, Markus Mayerwrote: > On 1 July 2016 at 03:52, Jani Nikula wrote: >> On Fri, 01 Jul 2016, Markus Mayer wrote: >>> Add a function called strtolower() to convert strings to lower case >>> in-place, overwriting the original string. >>> >>> This seems to be a recurring requirement in the kernel that is >>> currently being solved by several duplicated implementations doing the >>> same thing. >>> >>> Signed-off-by: Markus Mayer >>> --- >>> include/linux/string.h | 1 + >>> lib/string.c | 14 ++ >>> 2 files changed, 15 insertions(+) >>> >>> diff --git a/include/linux/string.h b/include/linux/string.h >>> index 26b6f6a..aad605e 100644 >>> --- a/include/linux/string.h >>> +++ b/include/linux/string.h >>> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); >>> #endif >>> void *memchr_inv(const void *s, int c, size_t n); >>> char *strreplace(char *s, char old, char new); >>> +char *strtolower(char *s); >>> >>> extern void kfree_const(const void *x); >>> >>> diff --git a/lib/string.c b/lib/string.c >>> index ed83562..6e3b560 100644 >>> --- a/lib/string.c >>> +++ b/lib/string.c >>> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) >>> return s; >>> } >>> EXPORT_SYMBOL(strreplace); >>> + >> >> This needs a kernel-doc comment right here. > > Will add it. > >>> +char *strtolower(char *s) >>> +{ >>> + char *p; >>> + >>> +if (unlikely(!s)) >>> +return NULL; >> >> Using spaces for indentation? See scripts/checkpatch.pl. > > Not on purpose. Thanks for spotting it. > >>> + >>> + for (p = s; *p; p++) >>> + *p = tolower(*p); >>> + >>> + return s; >> >> Why does it return a value? Could be void? > > It could be void, but I thought that would make the function's use > less flexible. As is, the return value is there if anybody wants it, > but it can be ignored if it is not needed. Also, it seems customary > for string functions to be returning the string that was passed in. > > I'll change it to void if there are strong opinions leaning that way. > Personally, I like that it returns a char * better. I don't have strong opinions on this. Just a general aversion to returning something redundant. Avoids questions like, does it allocate a new string, should I use the return value instead of the string I passed in, should I check the return value or can I ignore it, should I check both the string I pass in and the return value for != NULL, etc. But I could be persuaded either way. BR, Jani. > >> BR, >> Jani. >> >>> +} >>> +EXPORT_SYMBOL(strtolower); >> >> -- >> Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] lib: string: add function strtolower()
On 1 July 2016 at 03:52, Jani Nikulawrote: > On Fri, 01 Jul 2016, Markus Mayer wrote: >> Add a function called strtolower() to convert strings to lower case >> in-place, overwriting the original string. >> >> This seems to be a recurring requirement in the kernel that is >> currently being solved by several duplicated implementations doing the >> same thing. >> >> Signed-off-by: Markus Mayer >> --- >> include/linux/string.h | 1 + >> lib/string.c | 14 ++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/include/linux/string.h b/include/linux/string.h >> index 26b6f6a..aad605e 100644 >> --- a/include/linux/string.h >> +++ b/include/linux/string.h >> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); >> #endif >> void *memchr_inv(const void *s, int c, size_t n); >> char *strreplace(char *s, char old, char new); >> +char *strtolower(char *s); >> >> extern void kfree_const(const void *x); >> >> diff --git a/lib/string.c b/lib/string.c >> index ed83562..6e3b560 100644 >> --- a/lib/string.c >> +++ b/lib/string.c >> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) >> return s; >> } >> EXPORT_SYMBOL(strreplace); >> + > > This needs a kernel-doc comment right here. Will add it. >> +char *strtolower(char *s) >> +{ >> + char *p; >> + >> +if (unlikely(!s)) >> +return NULL; > > Using spaces for indentation? See scripts/checkpatch.pl. Not on purpose. Thanks for spotting it. >> + >> + for (p = s; *p; p++) >> + *p = tolower(*p); >> + >> + return s; > > Why does it return a value? Could be void? It could be void, but I thought that would make the function's use less flexible. As is, the return value is there if anybody wants it, but it can be ignored if it is not needed. Also, it seems customary for string functions to be returning the string that was passed in. I'll change it to void if there are strong opinions leaning that way. Personally, I like that it returns a char * better. > BR, > Jani. > >> +} >> +EXPORT_SYMBOL(strtolower); > > -- > Jani Nikula, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] lib: string: add function strtolower()
On Fri, 01 Jul 2016, Markus Mayerwrote: > Add a function called strtolower() to convert strings to lower case > in-place, overwriting the original string. > > This seems to be a recurring requirement in the kernel that is > currently being solved by several duplicated implementations doing the > same thing. > > Signed-off-by: Markus Mayer > --- > include/linux/string.h | 1 + > lib/string.c | 14 ++ > 2 files changed, 15 insertions(+) > > diff --git a/include/linux/string.h b/include/linux/string.h > index 26b6f6a..aad605e 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); > #endif > void *memchr_inv(const void *s, int c, size_t n); > char *strreplace(char *s, char old, char new); > +char *strtolower(char *s); > > extern void kfree_const(const void *x); > > diff --git a/lib/string.c b/lib/string.c > index ed83562..6e3b560 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) > return s; > } > EXPORT_SYMBOL(strreplace); > + This needs a kernel-doc comment right here. > +char *strtolower(char *s) > +{ > + char *p; > + > +if (unlikely(!s)) > +return NULL; Using spaces for indentation? See scripts/checkpatch.pl. > + > + for (p = s; *p; p++) > + *p = tolower(*p); > + > + return s; Why does it return a value? Could be void? BR, Jani. > +} > +EXPORT_SYMBOL(strtolower); -- Jani Nikula, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] lib: string: add function strtolower()
Add a function called strtolower() to convert strings to lower case in-place, overwriting the original string. This seems to be a recurring requirement in the kernel that is currently being solved by several duplicated implementations doing the same thing. Signed-off-by: Markus Mayer--- include/linux/string.h | 1 + lib/string.c | 14 ++ 2 files changed, 15 insertions(+) diff --git a/include/linux/string.h b/include/linux/string.h index 26b6f6a..aad605e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); #endif void *memchr_inv(const void *s, int c, size_t n); char *strreplace(char *s, char old, char new); +char *strtolower(char *s); extern void kfree_const(const void *x); diff --git a/lib/string.c b/lib/string.c index ed83562..6e3b560 100644 --- a/lib/string.c +++ b/lib/string.c @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace); + +char *strtolower(char *s) +{ + char *p; + +if (unlikely(!s)) +return NULL; + + for (p = s; *p; p++) + *p = tolower(*p); + + return s; +} +EXPORT_SYMBOL(strtolower); -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html