2002-11-16 06:14:21

by Olaf Dietsche

[permalink] [raw]
Subject: [PATCH] 2.5.47: strdup()

This *untested* patch adds strdup(). There are about five or six
different strdup() implementations in various parts of the kernel.

Regards, Olaf.

diff -urN a/include/linux/string.h b/include/linux/string.h
--- a/include/linux/string.h Sat Oct 5 18:42:33 2002
+++ b/include/linux/string.h Sat Nov 16 03:09:42 2002
@@ -15,7 +15,7 @@
extern char * strpbrk(const char *,const char *);
extern char * strsep(char **,const char *);
extern __kernel_size_t strspn(const char *,const char *);
-
+extern char *strdup(const char *);

/*
* Include machine specific inline routines
diff -urN a/kernel/ksyms.c b/kernel/ksyms.c
--- a/kernel/ksyms.c Tue Nov 12 17:56:02 2002
+++ b/kernel/ksyms.c Sat Nov 16 06:53:09 2002
@@ -576,6 +576,7 @@
EXPORT_SYMBOL(strnicmp);
EXPORT_SYMBOL(strspn);
EXPORT_SYMBOL(strsep);
+EXPORT_SYMBOL(strdup);

/* software interrupts */
EXPORT_SYMBOL(tasklet_init);
diff -urN a/lib/string.c b/lib/string.c
--- a/lib/string.c Sat Oct 5 18:42:33 2002
+++ b/lib/string.c Sat Nov 16 07:18:16 2002
@@ -22,6 +22,7 @@
#include <linux/types.h>
#include <linux/string.h>
#include <linux/ctype.h>
+#include <linux/slab.h>

#ifndef __HAVE_ARCH_STRNICMP
/**
@@ -479,6 +480,20 @@
s1++;
}
return NULL;
+}
+#endif
+
+#ifndef __HAVE_ARCH_STRDUP
+/**
+ * strdup - allocate memory and duplicate a string
+ */
+char *strdup(const char *s)
+{
+ char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
+ if (p)
+ strcpy(p, s);
+
+ return p;
}
#endif


2002-11-16 06:19:16

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Olaf Dietsche wrote:

> This *untested* patch adds strdup(). There are about five or six
> different strdup() implementations in various parts of the kernel.
>

> +#ifndef __HAVE_ARCH_STRDUP
> +/**
> + * strdup - allocate memory and duplicate a string
> + */
> +char *strdup(const char *s)
> +{
> + char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
> + if (p)
> + strcpy(p, s);
> +
> + return p;
> }



Comments:

* arch-specific strdup is unlikely

* IMO safer to create a strndup, and then update all strdup callers to
use strndup...


2002-11-16 06:25:44

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Olaf Dietsche wrote:
>
> +char *strdup(const char *s)
> +{
> + char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
> + if (p)
> + strcpy(p, s);
> +
> + return p;
> }

It's best to not assume GFP_KERNEL in there. Make the caller
pass in the required allocation mode.

2002-11-16 06:36:45

by William Lee Irwin III

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

On Sat, Nov 16, 2002 at 07:21:09AM +0100, Olaf Dietsche wrote:
+ char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);

I'm very concerned the implicit allocation will be abused and OOM
highmem boxen. Can you at least optionally highmem-allocate the buffers?


Thanks,
Bill

2002-11-16 08:42:10

by Peter Kjellerstedt

[permalink] [raw]
Subject: RE: [PATCH] 2.5.47: strdup()

> -----Original Message-----
> From: Olaf Dietsche
> [mailto:olaf.dietsche#[email protected]]
> Sent: Saturday, November 16, 2002 07:21
> To: [email protected]
> Cc: [email protected]
> Subject: [PATCH] 2.5.47: strdup()
>
> This *untested* patch adds strdup(). There are about five or six
> different strdup() implementations in various parts of the kernel.
>
> Regards, Olaf.

[snip]

> +#ifndef __HAVE_ARCH_STRDUP
> +/**
> + * strdup - allocate memory and duplicate a string
> + */
> +char *strdup(const char *s)
> +{
> + char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
> + if (p)
> + strcpy(p, s);
> +
> + return p;
> }
> #endif

You should make sure s != NULL before doing anything else.

//Peter

2002-11-16 15:36:15

by Olaf Dietsche

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Andrew Morton <[email protected]> writes:

> Olaf Dietsche wrote:
>>
>> +char *strdup(const char *s)
>> +{
>> + char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
>> + if (p)
>> + strcpy(p, s);
>> +
>> + return p;
>> }
>
> It's best to not assume GFP_KERNEL in there. Make the caller
> pass in the required allocation mode.

Ok, here is my next try, trying to include your and the other
suggestions. As before, it compiles, but is untested.

Regards, Olaf.

diff -urN a/include/linux/string.h b/include/linux/string.h
--- a/include/linux/string.h Sat Oct 5 18:42:33 2002
+++ b/include/linux/string.h Sat Nov 16 16:23:40 2002
@@ -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" {
@@ -15,7 +16,11 @@
extern char * strpbrk(const char *,const char *);
extern char * strsep(char **,const char *);
extern __kernel_size_t strspn(const char *,const char *);
+extern void *kmemdup(const void *, 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 Nov 12 17:56:02 2002
+++ b/kernel/ksyms.c Sat Nov 16 16:26:37 2002
@@ -576,6 +576,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 Sat Oct 5 18:42:33 2002
+++ b/lib/string.c Sat Nov 16 16:22:46 2002
@@ -22,6 +22,7 @@
#include <linux/types.h>
#include <linux/string.h>
#include <linux/ctype.h>
+#include <linux/slab.h>

#ifndef __HAVE_ARCH_STRNICMP
/**
@@ -479,6 +480,20 @@
s1++;
}
return NULL;
+}
+#endif
+
+#ifndef __HAVE_ARCH_KMEMDUP
+/**
+ * kmemdup - allocate memory and duplicate a string
+ */
+void *kmemdup(const void *s, size_t n, int flags)
+{
+ void *p = kmalloc(n, flags);
+ if (p)
+ memcpy(p, s, n);
+
+ return p;
}
#endif

2002-11-16 15:46:17

by Olaf Dietsche

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Peter Kjellerstedt <[email protected]> writes:

>> +char *strdup(const char *s)
>> +{
>> + char *p = kmalloc(strlen(s) + 1, GFP_KERNEL);
>> + if (p)
>> + strcpy(p, s);
>> +
>> + return p;
>> }
>> #endif
>
> You should make sure s != NULL before doing anything else.

Like strcpy(), it's the caller's responsibility to provide a valid
pointer. This shouldn't be a problem however, since access to NULL
triggers an Oops and thus is quickly detected.

Regards, Olaf.

2002-11-17 00:01:09

by Chris Wedgwood

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

On Sat, Nov 16, 2002 at 07:21:09AM +0100, Olaf Dietsche wrote:

> This *untested* patch adds strdup(). There are about five or six
> different strdup() implementations in various parts of the kernel.

How many users of these functions are there? I really don't like
certain functions which allocate memory in nebulous ways and almost
would prefer all users of this are fixed to specifically allocate and
str[n]cpy copy themselves making it clear who is allocating memory and
also who should free it.



2002-11-17 01:30:35

by Olaf Dietsche

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Chris Wedgwood <[email protected]> writes:

> On Sat, Nov 16, 2002 at 07:21:09AM +0100, Olaf Dietsche wrote:
>
>> This *untested* patch adds strdup(). There are about five or six
>> different strdup() implementations in various parts of the kernel.
>
> How many users of these functions are there? I really don't like
> certain functions which allocate memory in nebulous ways and almost
> would prefer all users of this are fixed to specifically allocate and
> str[n]cpy copy themselves making it clear who is allocating memory and
> also who should free it.

So you like duplicate code? Well, to each his own.

Regards, Olaf.

2002-11-17 04:33:41

by Chris Wedgwood

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

On Sun, Nov 17, 2002 at 02:37:27AM +0100, Olaf Dietsche wrote:

> So you like duplicate code? Well, to each his own.

Not at all.

I'm just not sure I like strdup being *easy* to use at it leads to
misuse. Admittedly this is a very poor argument for not having it.


--cw

2002-11-18 01:47:49

by Werner Almesberger

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Olaf Dietsche wrote:
> +static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }

Why hide what's really going on ? Better change the callers to use
kstrdup.

- Werner

--
_________________________________________________________________________
/ Werner Almesberger, Buenos Aires, Argentina [email protected] /
/_http://www.almesberger.net/____________________________________________/

2002-11-18 02:36:51

by Olaf Dietsche

[permalink] [raw]
Subject: Re: [PATCH] 2.5.47: strdup()

Werner Almesberger <[email protected]> writes:

> Olaf Dietsche wrote:
>> +static inline char *strdup(const char *s) { return kstrdup(s, GFP_KERNEL); }
>
> Why hide what's really going on ? Better change the callers to use
> kstrdup.

Convenience and laziness. And I haven't seen any other strdup() use,
which uses different allocation flags. But it's there for everybody to
use, if they wish.

Regards, Olaf.