Resend [PATCH] Make KOBJ_NAME_LEN match BUS_ID_SIZE

This looks suspiciously like strlcpy() from the *BSDs. Why not name it so?

The following patch is based on Samba's implementation (found in source/lib/replace.c). I just reformatted it somewhat, there are no functional changes. What do you think?

René

diff -ur linux-a/kernel/ksyms.c linux-b/kernel/ksyms.c
@@ -588,6 +588,7 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcpy);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);

diff -ur linux-a/lib/string.c linux-b/lib/string.c
@@ -527,3 +527,28 @@
 }
 
 #endif
+
+#ifndef __HAVE_ARCH_STRLCPY
+/**
+ * strlcpy - Copy a length-limited, %NUL-terminated string
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @bufsize: Size of the destination buffer
+ *
+ * Returns the length of @src, or 0 if @bufsize is 0. Unlike strncpy(),
+ * strlcpy() always NUL-terminates @dest and does no padding.
+ */
+size_t strlcpy(char *dest, const char *src, size_t bufsize)
+{
+	size_t len = strlen(src);
+	size_t ret = len;
+
+	if (bufsize == 0)
+		return 0;
+	if (len >= bufsize)
+		len = bufsize-1;
+	memcpy(dest, src, len);
+	dest[len] = '\0';   
+	return ret;
+}
+#endif

Looks good, you probably need a prototype in include/linux/string.h and a samba copyright boilerplate in lib/string.c.

What about strlcat?

Yes, Samba's and the BSDs' strlcpy() deviate in that point. It's a very unusual case to have a zero-sized buffer, though, so probably it doesn't matter much.

Anyway, I corrected this. Patch below contains a "BSD-compatible" version, and also a strlcat().

Ben, I think this one is better than your's because it's shorter and already GPL'd (there's not more license than C code :). Linus?

René

diff -ur linux-a/include/linux/string.h linux-b/include/linux/string.h
@@ -77,6 +77,12 @@
 #ifndef __HAVE_ARCH_MEMCHR
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
+#ifndef __HAVE_ARCH_STRLCPY
+__kernel_size_t strlcpy(char *, const char *, __kernel_size_t);
+#endif
+#ifndef __HAVE_ARCH_STRLCAT
+__kernel_size_t strlcat(char *, const char *, __kernel_size_t);
+#endif
 
 #ifdef __cplusplus
 }

diff -ur linux-a/kernel/ksyms.c linux-b/kernel/ksyms.c
@@ -588,6 +588,8 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcpy);
+EXPORT_SYMBOL(strlcat);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);

Mmpf. Thanks for noting. I'll be back in a minute with a fresh cup of tea..

René

You are right. How about that one?

René

diff -ur linux-a/include/linux/string.h linux-b/include/linux/string.h
@@ -77,6 +77,12 @@
 #ifndef __HAVE_ARCH_MEMCHR
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
+#ifndef __HAVE_ARCH_STRLCPY
+__kernel_size_t strlcpy(char *, const char *, __kernel_size_t);
+#endif
+#ifndef __HAVE_ARCH_STRLCAT
+__kernel_size_t strlcat(char *, const char *, __kernel_size_t);
+#endif
 
 #ifdef __cplusplus
 }

diff -ur linux-a/kernel/ksyms.c linux-b/kernel/ksyms.c
@@ -588,6 +588,8 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strlcpy);
+EXPORT_SYMBOL(strlcat);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);

In article <[email protected]>,

Ok, I did my own versions, since (a) I had already started and your patches wouldn't apply, and (b) I hate adding a zillion lines of extra copyright notices for a 5-line function and (c) I think your patch had EXPORT_SYMBOL wrong.

In particular, if any architecture ever decides to roll their own version of strlcpy/strlcat, the EXPORT_SYMBOL() in ksyms.c would be the wrong thing to do.

The current BK tree has my totally untested versions of these functions ("Famous last words: 'how hard can it be?'"), along with what I think is the proper way to export them.

Linus

Your API is compatible with *BSD but I'm wondering whether something slightly different might make error handling for callers easier:

Currently strlcpy returns the total length of the string it tried to create, whether it was too long or not. This means a check whether the complete string was copied is always something like

if (strlcpy(dest, src, sizeof(dest)) >= sizeof(dest))

      goto toolong;

If strlcpy would return 0 for successful copies this would simplify to

if (!strlcpy(dest, src, sizeof(dest)))

      goto toolong;

A patch is below.

I've tested only the compilation, any comments are welcome.

Another possible change to the semantics might be to copy/cat only if dest is big enough to make error handling especially in the case of strlcat easier (without additional information it's currently impossible to get the original contents of dest if only parts of src were copied).

cu
Adrian

@@ -102,20 +102,30 @@
  * @src: Where to copy the string from
  * @size: size of destination buffer
  *
- * Compatible with *BSD: the result is always a valid
+ * Similar to *BSD: the result is always a valid
  * NUL-terminated string that fits in the buffer (unless,
  * of course, the buffer size is zero). It does not pad
- * out the result like strncpy() does.
+ * out the result like strncpy() does. Unlike *BSD it
+ * returns 0 for a successful copy.
  */
 size_t strlcpy(char *dest, const char *src, size_t size)
 {
 	size_t ret = strlen(src);
+	size_t len;
+
+	if (!size)
+		return 0;
+
+	if (ret >= size)
+		len = size - 1;
+	else {
+		len = ret;
+		ret = 0;
+		}
+
+	memcpy(dest, src, len);
+	dest[len] = '\0';
 
-	if (size) {
-		size_t len = (ret >= size) ? size-1 : ret;
-		memcpy(dest, src, len);
-		dest[len] = '\0';
-	}
 	return ret;
 }
 EXPORT_SYMBOL(strlcpy);

@@ -175,23 +185,33 @@
  * @dest: The string to be appended to
  * @src: The string to append to it
  * @count: The size of the destination buffer.
+ *
+ * Returns 0 if successful, the number of bytes needed in dest
+ * otherwise.
  */
 size_t strlcat(char *dest, const char *src, size_t count)
 {
 	size_t dsize = strlen(dest);
 	size_t len = strlen(src);
-	size_t res = dsize + len;
+	size_t ret;
 
 	/* This would be a bug */
 	BUG_ON(dsize >= count);
 
 	dest += dsize;
 	count -= dsize;
+
 	if (len >= count)
-		len = count-1;
+		{
+		ret = dsize + len;
+		len = count - 1;
+		}
+	else
+		ret = 0;
+
 	memcpy(dest, src, len);
-	dest[len] = 0;
-	return res;
+	dest[len] = '\0';
+	return ret;
 }
 EXPORT_SYMBOL(strlcat);
 #endif

Part of the idea behind strlcpy was to explicity return the sizeof of the result. Read the papers on the function itself. The idea is that a lot of callers will usually do strlen() after calling strncpy.

Now, I'll be honest, I don't remember seeing one single usage of strncpy that was able to use the return of strlcpy. In fact, a few places that could have, shouldn't have since the value returned didn't translate to what the actual size of the string was. For example, you could not do:

	len = strlcpy(dest, myLongString, sizeof(dest));
	copy_to_user((void *)arg, dest, len);

If truncation occured, len would be the length it should have been, and not what it really was.

Also, 99% of the users didn't care about truncation either because it was known that it should never happen (e.g. netdevice names) or because truncation didn't cause any problems (e.g. informational strings). The use of strlcpy here was merely as a way to keep the kernel from choking on itself in the event of something really stupid happening (it's a lot easier to see a broken string than to trace overwriting random memory from a strcpy gone long).

So your return value is as useless as the current return value. But I think changing our strlcpy to mismatch *BSD would be even worse than what is there now.

What might be nice is a function that has two bits of info. a) Truncation, and b) The actual length of the resulting string. Maybe something like:

int strxcpy(char *dest, const char *src, size_t size, size_t *result)

{
	size_t src_size = strlen(src);
	size_t len = 0;

	if (size) {
		len = (src_size >= size) ? size - 1 : src_size;
		memcpy(dest, src, len);
		dest[len] = '\0';
		if (result)
			*result = len;
	}

	if (result)
		*result = len;

	return src_size >= size;
}

So the return value is zero for success and non-zero for truncation. In addition, if *result is non-NULL, the actual length of the result is set in that variable. The above example could then be:

	size_t len;
	strxcpy(dest, myLongString, sizeof(dest), &len);
        copy_to_user((void *)arg, dest, len);

IMO, this would only be for a few users, which is < 5% from what I can see. One place where I saw this might be useful is in the usb-sound driver where it creates an information string based on local info from the device firmware and from the USB structures, depending on whether the data was available in one place or both. It was complex, and the result size would have been very useful (instead I had to write it to use strlen in between strlcpy calls). Again, that's _one_ place out of hundreds.

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo       - http://www.deqo.com/

Well, judging by the fact that pretty much 99.9% of all users won't ever care about the return value. And coupled with the fact that the current behaviour is compatible with BSD (except for the BUG_ON() that I added, and I have no idea what BSD does for that case, since it pretty clearly _is_ a bug), I definitely prefer having standard return values than trying to make it "easier".

The BSD return values do actually make sense for nested operations.

Linus