Message-ID: <20160720202816.GA18480@lbrmn-mmayer.ric.broadcom.com>
Date:	Wed, 20 Jul 2016 13:28:16 -0700
From:	Markus Mayer <mmayer@...adcom.com>
To:	Chris Metcalf <cmetcalf@...lanox.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Kees Cook <keescook@...omium.org>,
	dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
	linux-acpi@...r.kernel.org, speakup@...ux-speakup.org,
	devel@...verdev.osuosl.org, linux-scsi@...r.kernel.org,
	target-devel@...r.kernel.org, linux-pm@...r.kernel.org,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 0/7] lib: string: add functions to case-convert strings

On Wed, Jul 13, 2016 at 03:52:38PM -0700, Markus Mayer wrote:
> On Sat, Jul 09, 2016 at 09:11:05PM -0700, Markus Mayer wrote:
>> On 9 July 2016 at 20:13, Chris Metcalf <cmetcalf@...lanox.com> wrote:
>>> On 7/8/2016 6:43 PM, Markus Mayer wrote:
>>>>
>>>> This series introduces a family of generic string case conversion
>>>> functions. This kind of functionality is needed in several places in
>>>> the kernel. Right now, everybody seems to be implementing their own
>>>> copy of this functionality.
>>>>
>>>> Based on the discussion of the previous version of this series[1] and
>>>> the use cases found in the kernel, it does look like having several
>>>> flavours of case conversion functions is beneficial. The use cases fall
>>>> into three categories:
>>>>      - copying a string and converting the case while specifying a
>>>>        maximum length to mimic strlcpy()
>>>>      - copying a string and converting the case without specifying a
>>>>        length to mimic strcpy()
>>>>      - converting the case of a string in-place (i.e. modifying the
>>>>        string that was passed in)
>>>>
>>>> Consequently, I am proposing these new functions:
>>>>      void strlcpytoupper(char *dst, const char *src, size_t len);
>>>>      void strlcpytolower(char *dst, const char *src, size_t len);
>>>>      void strcpytoupper(char *dst, const char *src);
>>>>      void strcpytolower(char *dst, const char *src);
>>>>      void strtoupper(char *s);
>>>>      void strtolower(char *s);
>>>
>>>
>>> You may want to read the article here:
>>>
>>> https://lwn.net/Articles/659214/
>> 
>> I'll read that. Thanks.
> 
> It doesn't look like there is going to be the danger of "mass changes". 
> So far, I have two ACKs (one where the semantics doesn't change,
> because it's using strtolower()) and the other in a driver in staging.
> 
> But I understand the concern and will keep an eye out if there are
> other ACKs.
>  
>>> and follow up some of the discussion threads on LKML about the best
>>> semantics to advertise for the strlcpy/strscpy variants.  It might
>>> be helpful to return some kind of overflow/truncation error from
>>> your copy functions so people can error-check the result.
>> 
>> I am inclined to agree. However, everybody has been telling me that
>> these functions should be void. Originally they weren't.
> 
> What about something like this?  It might also work to keep the four
> static inline functions as "void" (since they won't ever return E2BIG
> anyway) and just have strlcpyto* return an integer (since that's where
> the buffer could be too small).
> 
> Rasmus, what's your take? 

Ping. Any thoughts on this proposal? Does it make sense for me to sent out a
new revision of the patch set incorporating these changes -- at least for
the strlcpyto* functions?

Thanks,
-Markus
 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index ae82d13..6cc85dc 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -116,8 +116,8 @@ 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);
> -extern void strlcpytoupper(char *dst, const char *src, size_t len);
> -extern void strlcpytolower(char *dst, const char *src, size_t len);
> +extern int strlcpytoupper(char *dst, const char *src, size_t len);
> +extern int strlcpytolower(char *dst, const char *src, size_t len);
>  
>  extern void kfree_const(const void *x);
>  
> @@ -175,38 +175,46 @@ static inline const char *kbasename(const char *path)
>   * strcpytoupper - Copy string and convert to uppercase.
>   * @dst: The buffer to store the result.
>   * @src: The string to convert to uppercase.
> + *
> + * Returns the number of characters copied.
>   */
> -static inline void strcpytoupper(char *dst, const char *src)
> +static inline int strcpytoupper(char *dst, const char *src)
>  {
> -	strlcpytoupper(dst, src, ~(size_t)0);
> +	return strlcpytoupper(dst, src, ~(size_t)0);
>  }
>  
>  /**
>   * strcpytolower - Copy string and convert to lowercase.
>   * @dst: The buffer to store the result.
>   * @src: The string to convert to lowercase.
> + *
> + * Returns the number of characters copied.
>   */
> -static inline void strcpytolower(char *dst, const char *src)
> +static inline int strcpytolower(char *dst, const char *src)
>  {
> -	strlcpytolower(dst, src, ~(size_t)0);
> +	return strlcpytolower(dst, src, ~(size_t)0);
>  }
>  
>  /**
>   * strtoupper - Convert string to uppercase.
>   * @s: The string to operate on.
> + *
> + * Returns the number of characters copied.
>   */
> -static inline void strtoupper(char *s)
> +static inline int strtoupper(char *s)
>  {
> -	strlcpytoupper(s, s, ~(size_t)0);
> +	return strlcpytoupper(s, s, ~(size_t)0);
>  }
>  
>  /**
>   * strtolower - Convert string to lowercase.
>   * @s: The string to operate on.
> + *
> + * Returns the number of characters copied.
>   */
> -static inline void strtolower(char *s)
> +static inline int strtolower(char *s)
>  {
> -	strlcpytolower(s, s, ~(size_t)0);
> +	return strlcpytolower(s, s, ~(size_t)0);
>  }
>  
>  #endif /* _LINUX_STRING_H_ */
> diff --git a/lib/string.c b/lib/string.c
> index 7903e10..d36d5fb2 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -958,17 +958,21 @@ EXPORT_SYMBOL(strreplace);
>   * @dst: The buffer to store the result.
>   * @src: The string to convert to uppercase.
>   * @len: Maximum string length. May be SIZE_MAX to set no limit.
> + *
> + * Returns the number of characters copied or -E2BIG if @dst wasn't big enough.
>   */
> -void strlcpytoupper(char *dst, const char *src, size_t len)
> +int strlcpytoupper(char *dst, const char *src, size_t len)
>  {
>  	size_t i;
>  
>  	if (!len)
> -		return;
> +		return -E2BIG;
>  
>  	for (i = 0; i < len && src[i]; ++i)
>  		dst[i] = toupper(src[i]);
>  	dst[i < len ? i : i - 1] = '\0';
> +
> +	return (i < len) ? i : -E2BIG;
>  }
>  EXPORT_SYMBOL(strlcpytoupper);
>  
> @@ -977,16 +981,20 @@ EXPORT_SYMBOL(strlcpytoupper);
>   * @dst: The buffer to store the result.
>   * @src: The string to convert to lowercase.
>   * @len: Maximum string length. May be SIZE_MAX to set no limit.
> + *
> + * Returns the number of characters copied or -E2BIG if @dst wasn't big enough.
>   */
> -void strlcpytolower(char *dst, const char *src, size_t len)
> +int strlcpytolower(char *dst, const char *src, size_t len)
>  {
>  	size_t i;
>  
>  	if (!len)
> -		return;
> +		return -E2BIG;
>  
>  	for (i = 0; i < len && src[i]; ++i)
>  		dst[i] = tolower(src[i]);
>  	dst[i < len ? i : i - 1] = '\0';
> +
> +	return (i < len) ? i : -E2BIG;
>  }
>  EXPORT_SYMBOL(strlcpytolower);

Powered by blists - more mailing lists