Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440030 > unrolled thread

Re: [PATCH v3 0/7] lib: string: add functions to case-convert strings

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/7] lib: string: add functions to case-convert strings Markus Mayer <markus.mayer@broadcom.com> - 2016-07-10 06:20 +0200
    Re: [PATCH v3 0/7] lib: string: add functions to case-convert strings Markus Mayer <mmayer@broadcom.com> - 2016-07-14 01:00 +0200
      Re: [PATCH v3 0/7] lib: string: add functions to case-convert strings Markus Mayer <mmayer@broadcom.com> - 2016-07-20 22:30 +0200

#1440030 — Re: [PATCH v3 0/7] lib: string: add functions to case-convert strings

On 9 July 2016 at 20:13, Chris Metcalf <cmetcalf@mellanox.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.

> 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.

Regards,
-Markus

[toc] | [next] | [standalone]

#1442929

On Sat, Jul 09, 2016 at 09:11:05PM -0700, Markus Mayer wrote:
> On 9 July 2016 at 20:13, Chris Metcalf <cmetcalf@mellanox.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? 

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);

[toc] | [prev] | [next] | [standalone]

#1447446

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@mellanox.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);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web