Message-ID: <20160713225236.GA30571@lbrmn-mmayer.ric.broadcom.com>
Date:	Wed, 13 Jul 2016 15:52:38 -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 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? 

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