[PATCH] [TRIVIAL] kstrdup

[PATCH] [TRIVIAL] kstrdup

[Rusty Russell]

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.

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
--- linux-2.5-bk/drivers/md/dm-ioctl.c	2003-01-02 12:47:01.000000000 +1100
+++ working-2.5-bk-kstrdup/drivers/md/dm-ioctl.c	2003-01-06 15:47:26.000000000 +1100
@@ -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;
 
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
--- linux-2.5-bk/include/linux/string.h	2003-01-02 12:35:15.000000000 +1100
+++ working-2.5-bk-kstrdup/include/linux/string.h	2003-01-06 15:48:24.000000000 +1100
@@ -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
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
--- linux-2.5-bk/kernel/ksyms.c	2003-01-02 14:48:01.000000000 +1100
+++ working-2.5-bk-kstrdup/kernel/ksyms.c	2003-01-06 15:49:30.000000000 +1100
@@ -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);
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
--- linux-2.5-bk/lib/string.c	2003-01-02 12:35:15.000000000 +1100
+++ working-2.5-bk-kstrdup/lib/string.c	2003-01-06 15:48:57.000000000 +1100
@@ -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;
+}

Re: [PATCH] [TRIVIAL] kstrdup

[Jeff Garzik]

On Tue, Jan 14, 2003 at 12:55:40PM +1100, Rusty Russell wrote:
> +
> +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

Re: [PATCH] [TRIVIAL] kstrdup

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Tue, 14 Jan 2003 12:55:40 +1100, Rusty Russell said:
> Everyone loves reimplementing strdup. 

> +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()?


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] [TRIVIAL] kstrdup

[Jeff Garzik]

On Mon, Jan 13, 2003 at 10:28:14PM -0500, Valdis.Kletnieks@vt.edu 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.

Re: [PATCH] [TRIVIAL] kstrdup

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Mon, 13 Jan 2003 22:38:03 EST, Jeff Garzik said:
> On Mon, Jan 13, 2003 at 10:28:14PM -0500, Valdis.Kletnieks@vt.edu 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


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] [TRIVIAL] kstrdup

[Jeff Garzik]

On Mon, Jan 13, 2003 at 10:53:32PM -0500, Valdis.Kletnieks@vt.edu 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" :)


> 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

Re: [PATCH] [TRIVIAL] kstrdup

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Mon, 13 Jan 2003 23:15:14 EST, Jeff Garzik said:
> 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).


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] [TRIVIAL] kstrdup

[Olaf Dietsche]

Rusty Russell <rusty@rustcorp.com.au> writes:

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

diff -urN a/include/linux/string.h b/include/linux/string.h
--- a/include/linux/string.h	Mon Nov 18 10:33:57 2002
+++ b/include/linux/string.h	Tue Jan 14 12:37:29 2003
@@ -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
diff -urN a/kernel/ksyms.c b/kernel/ksyms.c
--- a/kernel/ksyms.c	Tue Jan 14 12:31:42 2003
+++ b/kernel/ksyms.c	Tue Jan 14 12:33:41 2003
@@ -586,6 +586,7 @@
 EXPORT_SYMBOL(strnicmp);
 EXPORT_SYMBOL(strspn);
 EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(kmemdup);
 
 /* software interrupts */
 EXPORT_SYMBOL(tasklet_init);
diff -urN a/lib/string.c b/lib/string.c
--- a/lib/string.c	Mon Nov 18 10:33:58 2002
+++ b/lib/string.c	Tue Jan 14 12:39:55 2003
@@ -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
 

Re: [PATCH] [TRIVIAL] kstrdup

[Rusty Russell]

In message <20030114031921.GD404@gtf.org> you write:
> 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.

Re: [PATCH] [TRIVIAL] kstrdup

[Rusty Russell]

In message <87bs2kkl50.fsf@goat.bogus.local> you write:
> +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.

Re: [PATCH] [TRIVIAL] kstrdup

[Pavel Machek]

Hi!

> 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
> +char *kstrdup(const char *s, int gfp)
> +{
> +	char *buf = kmalloc(strlen(s)+1, gfp);
> +	if (buf)
> +		strcpy(buf, s);
> +	return buf;
> +}



-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?

Re: [PATCH] [TRIVIAL] kstrdup

[Rusty Russell]

In message <20030119233750.GA674@elf.ucw.cz> you write:
> Hi!
> 
> > 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.

Re: [PATCH] [TRIVIAL] kstrdup

[Pavel Machek]

Hi!

> > > 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
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: [PATCH] [TRIVIAL] kstrdup

[Martin Mares]

Hi!

> 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
-- 
Martin `MJ' Mares   <mj@ucw.cz>   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

Re: [PATCH] [TRIVIAL] kstrdup

[Pavel Machek]

Hi!

> > 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
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: [PATCH] [TRIVIAL] kstrdup

[Rusty Russell]

In message <20030123140215.GA1229@atrey.karlin.mff.cuni.cz> you write:
> Hi!
> 
> > > > 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.