Re: [PATCH 1/6] lib: string: add function strtolower()

From: Rasmus Villemoes
Date: Fri Jul 01 2016 - 17:09:41 EST


On Fri, Jul 01 2016, Markus Mayer <mmayer@xxxxxxxxxxxx> 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 <mmayer@xxxxxxxxxxxx>
> ---
> 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