Skip to contentSkip to conversation

kstrdup

PATCH In review Rusty Russell 6 participants 16 messages Jan 14, 2003 – Jan 29, 2003

Subsystems (3)
Library code Device mapper Generic string library
lore
Share & tools
lore

Rusty Russell

author
#0 lore raw

PATCH 1/1 kstrdup

Everyone loves reimplementing strdup. Give them a kstrdup (basically from drivers/md).

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: kstrdup
Author: Neil Brown and Rusty Russell
Status: Trivial

D: Everyone loves reimplementing strdup. Give them a kstrdup.

drivers/md/dm-ioctl.c +4 -11
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/drivers/md/dm-ioctl.c working-2.5-bk-kstrdup/drivers/md/dm-ioctl.c
@@ -14,6 +14,7 @@
 #include <linux/wait.h>
 #include <linux/blk.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 
 #include <asm/uaccess.h>
 

@@ -118,14 +119,6 @@ static struct hash_cell *__get_uuid_cell
 /*-----------------------------------------------------------------
  * Inserting, removing and renaming a device.
  *---------------------------------------------------------------*/
-static inline char *kstrdup(const char *str)
-{
-	char *r = kmalloc(strlen(str) + 1, GFP_KERNEL);
-	if (r)
-		strcpy(r, str);
-	return r;
-}
-
 static struct hash_cell *alloc_cell(const char *name, const char *uuid,
 				    struct mapped_device *md)
 {

@@ -135,7 +128,7 @@ static struct hash_cell *alloc_cell(cons
 	if (!hc)
 		return NULL;
 
-	hc->name = kstrdup(name);
+	hc->name = kstrdup(name, GFP_KERNEL);
 	if (!hc->name) {
 		kfree(hc);
 		return NULL;

@@ -145,7 +138,7 @@ static struct hash_cell *alloc_cell(cons
 		hc->uuid = NULL;
 
 	else {
-		hc->uuid = kstrdup(uuid);
+		hc->uuid = kstrdup(uuid, GFP_KERNEL);
 		if (!hc->uuid) {
 			kfree(hc->name);
 			kfree(hc);

@@ -266,7 +259,7 @@ int dm_hash_rename(const char *old, cons
 	/*
 	 * duplicate new.
 	 */
-	new_name = kstrdup(new);
+	new_name = kstrdup(new, GFP_KERNEL);
 	if (!new_name)
 		return -ENOMEM;
include/linux/string.h +2 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/include/linux/string.h working-2.5-bk-kstrdup/include/linux/string.h
@@ -78,6 +78,8 @@ extern int memcmp(const void *,const voi
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
 
+extern char *kstrdup(const char *s, int gfp);
+
 #ifdef __cplusplus
 }
 #endif
kernel/ksyms.c +1 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/kernel/ksyms.c working-2.5-bk-kstrdup/kernel/ksyms.c
@@ -577,6 +577,7 @@ EXPORT_SYMBOL(get_write_access);
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(kstrdup);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
lib/string.c +8 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/lib/string.c working-2.5-bk-kstrdup/lib/string.c
@@ -525,5 +525,12 @@ void *memchr(const void *s, int c, size_
 	}
 	return NULL;
 }
-
 #endif
+
+char *kstrdup(const char *s, int gfp)
+{
+	char *buf = kmalloc(strlen(s)+1, gfp);
+	if (buf)
+		strcpy(buf, s);
+	return buf;
+}
#1 lore raw

lib/string.c void *memchr(const void *s, int c, size_ · line 536

Quoted 8 lines · Rusty Russell
lib/string.c +8 -0
+
+char *kstrdup(const char *s, int gfp)
+{
+	char *buf = kmalloc(strlen(s)+1, gfp);
+	if (buf)
+		strcpy(buf, s);
+	return buf;
+}

Poo -- why not store the length in a temp, since the compiler does it behind the scenes anyway, and then memcpy instead of strcpy? (remember to store the +1 too, since you want to memcpy the null...)

Jeff

author
#2 lore raw

In message <[email protected]> you write:

Quoted 2 lines

Poo -- why not store the length in a temp, since the compiler does it behind the scenes anyway, and then memcpy instead of strcpy?

Apathy, mainly. There's a mild preference for using string functions on strings, but... Naah, mainly it's pure lack of caring.

Hope that helps,
Rusty.
PS. If your apathy is less than mine, please submit replacement patch
to trivial.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#3 lore raw
In reply to Rusty Russell

lib/string.c void *memchr(const void *s, int c, size_ · line 536

Quoted 9 lines · Rusty Russell

Everyone loves reimplementing strdup.

lib/string.c +7 -0
+char *kstrdup(const char *s, int gfp)
+{
+	char *buf = kmalloc(strlen(s)+1, gfp);
+	if (buf)
+		strcpy(buf, s);
+	return buf;
+}

Out of curiosity, who's job is it to avoid the race condition between when this function takes the strlen() and the other processor makes it a longer string before we return from kmalloc() and do the strcpy()?

#4 lore raw
Quoted 3 lines · On Mon, Jan 13, 2003 at 10:28:14PM -0500, [email protected] wrote:

Out of curiosity, who's job is it to avoid the race condition between when this function takes the strlen() and the other processor makes it a longer string before we return from kmalloc() and do the strcpy()?

The caller's.

#5 lore raw
Quoted 6 lines · Jeff Garzik
Quoted 3 lines · On Mon, Jan 13, 2003 at 10:28:14PM -0500, [email protected] wrote:

Out of curiosity, who's job is it to avoid the race condition between when this function takes the strlen() and the other processor makes it a longer string before we return from kmalloc() and do the strcpy()?

The caller's.

That's cool, long as everybody agrees on that - I've already filled my career quota of chasing down bugs due to non-threadsafe use of str*() functions. ;)

All the same, I'd probably feel better if it used strncpy() instead - there'd still be the possibility of copying now-stale data, but at least you'd not be able to walk off the end of the *new* array's allocated space....

/Valdis

#6 lore raw
Quoted 2 lines · On Mon, Jan 13, 2003 at 10:53:32PM -0500, [email protected] wrote:

That's cool, long as everybody agrees on that - I've already filled my career quota of chasing down bugs due to non-threadsafe use of str*() functions. ;)

Don't worry, it's pretty much a rule in Linux, IMO. Synchronization belongs _above_ simple data type primitives like strings or lists.

That's why answering your email was quick and easy ;-) The Linux answer is "don't do that" :)

Quoted 3 lines

All the same, I'd probably feel better if it used strncpy() instead - there'd still be the possibility of copying now-stale data, but at least you'd not be able to walk off the end of the *new* array's allocated space....

_Not_ doing things like this is a reason why Linux is so fast in certain areas :) Linus preaches over and over, "do what you need to do, and no more."

But having said that -- see my mail to Rusty about storing the strlen() result and then calling memcpy(). It [purposefully] does not address the fact that the string may become stale data, because it's the job of a higher level to ensure that. But it does make explicit a compiler temporary, and allows us to use the presumeably-faster memcpy().

Not that kstrdup() matters a whole lot, but anyway...

Jeff

#7 lore raw
Quoted 5 lines · Jeff Garzik

But having said that -- see my mail to Rusty about storing the strlen() result and then calling memcpy(). It [purposefully] does not address the fact that the string may become stale data, because it's the job of a higher level to ensure that. But it does make explicit a compiler temporary, and allows us to use the presumeably-faster memcpy().

5 second's thought and another shot of Cherry Coke and it suddenly dawns on me that memcpy() addresses my concerns as well as strncpy(), and as you noted is presumably faster as well (since it doesn't have to keep looking for a terminating null).

#8 lore raw
In reply to Rusty Russell
Quoted 1 line · Rusty Russell

Everyone loves reimplementing strdup.

Really? ;-) SCNR

Please consider this one or parts of it:
<http://groups.google.com/groups?selm=87y97t34hs.fsf%40goat.bogus.local>

Rediffed against 2.5.58 below.

Regards, Olaf.

include/linux/string.h +6 -0
diff -urN a/include/linux/string.h b/include/linux/string.h
@@ -7,6 +7,7 @@
 
 #include <linux/types.h>	/* for size_t */
 #include <linux/stddef.h>	/* for NULL */
+#include <linux/gfp.h>		/* for GFP_KERNEL */
 
 #ifdef __cplusplus
 extern "C" {

@@ -16,6 +17,11 @@
 extern char * strsep(char **,const char *);
 extern __kernel_size_t strspn(const char *,const char *);
 extern __kernel_size_t strcspn(const char *,const char *);
+extern void *kmemdup(const void *, __kernel_size_t, int);
+  
+static inline char *kstrdup(const char *s, int flags)
+	{ return kmemdup(s, strlen(s) + 1, flags); }
+static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }
 
 /*
  * Include machine specific inline routines
kernel/ksyms.c +1 -0
diff -urN a/kernel/ksyms.c b/kernel/ksyms.c
@@ -586,6 +586,7 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(kmemdup);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
lib/string.c +15 -0
diff -urN a/lib/string.c b/lib/string.c
@@ -22,6 +22,7 @@
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/slab.h>
 
 #ifndef __HAVE_ARCH_STRNICMP
 /**

@@ -502,6 +503,20 @@
 		s1++;
 	}
 	return NULL;
+}
+#endif
+
+#ifndef __HAVE_ARCH_KMEMDUP
+/**
+ * kmemdup - allocate memory and duplicate a string
+ */
+void *kmemdup(const void *s, __kernel_size_t n, int flags)
+{
+	void *p = kmalloc(n, flags);
+	if (p)
+		memcpy(p, s, n);
+
+	return p;
 }
 #endif
author
#9 lore raw

In message <[email protected]> you write:

include/linux/string.h line 24

Quoted 5 lines
include/linux/string.h +5 -0
+extern void *kmemdup(const void *, __kernel_size_t, int);
+  
+static inline char *kstrdup(const char *s, int flags)
+	{ return kmemdup(s, strlen(s) + 1, flags); }
+static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }

I disagree with this approach. (1) because strdup hides an allocation
assumption: it's too dangerous an interface, (2) because introducing a new interface is a much bigger deal than consolidating existing ones.

But really, I only keep the kstrdup patch around to irritate Linus.

Thanks!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#10 lore raw
In reply to Rusty Russell

Hi!

Quoted 2 lines

Everyone loves reimplementing strdup. Give them a kstrdup (basically from drivers/md).

I believe it would be better to call it strdup.

*Or* you might want kstrdup( "foo", GFP_ATOMIC ); But if you are hard-coding GFP_KERNEL, I believe there's no point in calling it *k*strdup.

Pavel

Quoted 7 lines
lib/string.c +7 -0
+char *kstrdup(const char *s, int gfp)
+{
+	char *buf = kmalloc(strlen(s)+1, gfp);
+	if (buf)
+		strcpy(buf, s);
+	return buf;
+}
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
author
#11 lore raw

In message <[email protected]> you write:

Quoted 6 lines

Hi!

Quoted 2 lines

Everyone loves reimplementing strdup. Give them a kstrdup (basically from drivers/md).

I believe it would be better to call it strdup.

No. Don't confuse people.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#12 lore raw

Hi!

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Everyone loves reimplementing strdup. Give them a kstrdup (basically from drivers/md).

I believe it would be better to call it strdup.

No. Don't confuse people.

Ehm?! What's confusing on strdup? Or you want to also introduce kmemcpy, kmemcmp, ksprintf etc?
Pavel

Signature · 2 lines
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.
#13 lore raw

Hi!

Quoted 2 lines

Ehm?! What's confusing on strdup? Or you want to also introduce kmemcpy, kmemcmp, ksprintf etc?

No, as long as they don't allocate any memory.

"kstrdup" makes it clear that the string is allocated by kmalloc() and should be freed by kfree().

Have a nice fortnight

Signature · 3 lines
Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
"In theory, practice and theory are the same, but in practice they are different." -- Larry McVoy
#14 lore raw

Hi!

Quoted 7 lines
Quoted 2 lines

Ehm?! What's confusing on strdup? Or you want to also introduce kmemcpy, kmemcmp, ksprintf etc?

No, as long as they don't allocate any memory.

"kstrdup" makes it clear that the string is allocated by kmalloc() and should be freed by kfree().

As we do not have any free(), there's no possibility for confusion.

As I said, if kstrdup took GFP_XXX argument, I'd agree, but as it hardcodes GFP_KERNEL...
Pavel

Signature · 2 lines
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.
author
#15 lore raw
In reply to Pavel Machek

In message <[email protected]> you write:

Quoted 11 lines

Hi!

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Everyone loves reimplementing strdup. Give them a kstrdup (basically from drivers/md).

I believe it would be better to call it strdup.

No. Don't confuse people.

Ehm?! What's confusing on strdup? Or you want to also introduce kmemcpy, kmemcmp, ksprintf etc?

It has an extra argument, like kmalloc.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.