* [BK PATCH] One strdup() to rule them all
@ 2003-08-25 16:14 Dan Aloni
2003-08-25 16:25 ` Jakub Jelinek
2003-08-25 16:37 ` Jörn Engel
0 siblings, 2 replies; 17+ messages in thread
From: Dan Aloni @ 2003-08-25 16:14 UTC (permalink / raw)
To: Linux Kernel List
While working on the fix to network devices names and sysctl,
I fought to urge to create yet another strdup() implementation
This came up.
NOTE: I intentionally avoided changes to UML and ALSA. These were
the only two implementations left.
You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.
===================================================================
ChangeSet@1.1292, 2003-08-25 18:56:50+03:00, da-x@gmx.net
One strdup() to rule them all.
Unite 6 equal implementations of strdup() to just one.
drivers/md/dm-ioctl-v1.c | 14 ++++----------
drivers/md/dm-ioctl-v4.c | 14 ++++----------
drivers/parport/probe.c | 8 --------
fs/afs/super.c | 8 --------
fs/intermezzo/intermezzo_fs.h | 11 +----------
include/linux/string.h | 1 +
lib/string.c | 16 ++++++++++++++++
net/sunrpc/svcauth_unix.c | 9 +--------
8 files changed, 27 insertions(+), 54 deletions(-)
diff -Nru a/drivers/md/dm-ioctl-v1.c b/drivers/md/dm-ioctl-v1.c
--- a/drivers/md/dm-ioctl-v1.c Mon Aug 25 19:03:26 2003
+++ b/drivers/md/dm-ioctl-v1.c Mon Aug 25 19:03:26 2003
@@ -14,6 +14,7 @@
#include <linux/wait.h>
#include <linux/slab.h>
#include <linux/devfs_fs_kernel.h>
+#include <linux/string.h>
#include <asm/uaccess.h>
@@ -118,13 +119,6 @@
/*-----------------------------------------------------------------
* 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 +129,7 @@
if (!hc)
return NULL;
- hc->name = kstrdup(name);
+ hc->name = strdup(name);
if (!hc->name) {
kfree(hc);
return NULL;
@@ -145,7 +139,7 @@
hc->uuid = NULL;
else {
- hc->uuid = kstrdup(uuid);
+ hc->uuid = strdup(uuid);
if (!hc->uuid) {
kfree(hc->name);
kfree(hc);
@@ -264,7 +258,7 @@
/*
* duplicate new.
*/
- new_name = kstrdup(new);
+ new_name = strdup(new);
if (!new_name)
return -ENOMEM;
diff -Nru a/drivers/md/dm-ioctl-v4.c b/drivers/md/dm-ioctl-v4.c
--- a/drivers/md/dm-ioctl-v4.c Mon Aug 25 19:03:26 2003
+++ b/drivers/md/dm-ioctl-v4.c Mon Aug 25 19:03:26 2003
@@ -14,6 +14,7 @@
#include <linux/slab.h>
#include <linux/devfs_fs_kernel.h>
#include <linux/dm-ioctl.h>
+#include <linux/string.h>
#include <asm/uaccess.h>
@@ -119,13 +120,6 @@
/*-----------------------------------------------------------------
* 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)
@@ -136,7 +130,7 @@
if (!hc)
return NULL;
- hc->name = kstrdup(name);
+ hc->name = strdup(name);
if (!hc->name) {
kfree(hc);
return NULL;
@@ -146,7 +140,7 @@
hc->uuid = NULL;
else {
- hc->uuid = kstrdup(uuid);
+ hc->uuid = strdup(uuid);
if (!hc->uuid) {
kfree(hc->name);
kfree(hc);
@@ -268,7 +262,7 @@
/*
* duplicate new.
*/
- new_name = kstrdup(new);
+ new_name = strdup(new);
if (!new_name)
return -ENOMEM;
diff -Nru a/drivers/parport/probe.c b/drivers/parport/probe.c
--- a/drivers/parport/probe.c Mon Aug 25 19:03:26 2003
+++ b/drivers/parport/probe.c Mon Aug 25 19:03:26 2003
@@ -47,14 +47,6 @@
printk("\n");
}
-static char *strdup(char *str)
-{
- int n = strlen(str)+1;
- char *s = kmalloc(n, GFP_KERNEL);
- if (!s) return NULL;
- return strcpy(s, str);
-}
-
static void parse_data(struct parport *port, int device, char *str)
{
char *txt = kmalloc(strlen(str)+1, GFP_KERNEL);
diff -Nru a/fs/afs/super.c b/fs/afs/super.c
--- a/fs/afs/super.c Mon Aug 25 19:03:26 2003
+++ b/fs/afs/super.c Mon Aug 25 19:03:26 2003
@@ -29,14 +29,6 @@
#define AFS_FS_MAGIC 0x6B414653 /* 'kAFS' */
-static inline char *strdup(const char *s)
-{
- char *ns = kmalloc(strlen(s)+1,GFP_KERNEL);
- if (ns)
- strcpy(ns,s);
- return ns;
-}
-
static void afs_i_init_once(void *foo, kmem_cache_t *cachep, unsigned long flags);
static struct super_block *afs_get_sb(struct file_system_type *fs_type,
diff -Nru a/fs/intermezzo/intermezzo_fs.h b/fs/intermezzo/intermezzo_fs.h
--- a/fs/intermezzo/intermezzo_fs.h Mon Aug 25 19:03:26 2003
+++ b/fs/intermezzo/intermezzo_fs.h Mon Aug 25 19:03:26 2003
@@ -58,6 +58,7 @@
# include <linux/slab.h>
# include <linux/vmalloc.h>
# include <linux/smp_lock.h>
+# include <linux/string.h>
/* fixups for fs.h */
# ifndef fs_down
@@ -702,16 +703,6 @@
{
return (strlen(name) == dentry->d_name.len &&
memcmp(name, dentry->d_name.name, dentry->d_name.len) == 0);
-}
-
-static inline char *strdup(char *str)
-{
- char *tmp;
- tmp = kmalloc(strlen(str) + 1, GFP_KERNEL);
- if (tmp)
- memcpy(tmp, str, strlen(str) + 1);
-
- return tmp;
}
/* buffer MUST be at least the size of izo_ioctl_hdr */
diff -Nru a/include/linux/string.h b/include/linux/string.h
--- a/include/linux/string.h Mon Aug 25 19:03:26 2003
+++ b/include/linux/string.h Mon Aug 25 19:03:26 2003
@@ -16,6 +16,7 @@
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 char * strdup(const char *);
/*
* Include machine specific inline routines
diff -Nru a/lib/string.c b/lib/string.c
--- a/lib/string.c Mon Aug 25 19:03:26 2003
+++ b/lib/string.c Mon Aug 25 19:03:26 2003
@@ -582,3 +582,19 @@
}
#endif
+
+/**
+ * strdup - Allocate a copy of a string.
+ * @s: The string to copy. Must not be NULL.
+ *
+ * returns the address of the allocation, or NULL on
+ * error.
+ */
+char *strdup(const char *s)
+{
+ char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
+ if (rv)
+ strcpy(rv, s);
+ return rv;
+}
+EXPORT_SYMBOL(strdup);
diff -Nru a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
--- a/net/sunrpc/svcauth_unix.c Mon Aug 25 19:03:26 2003
+++ b/net/sunrpc/svcauth_unix.c Mon Aug 25 19:03:26 2003
@@ -7,6 +7,7 @@
#include <linux/err.h>
#include <linux/seq_file.h>
#include <linux/hash.h>
+#include <linux/string.h>
#define RPCDBG_FACILITY RPCDBG_AUTH
@@ -18,14 +19,6 @@
* AUTHNULL as for AUTHUNIX, and that is done here.
*/
-
-static char *strdup(char *s)
-{
- char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
- if (rv)
- strcpy(rv, s);
- return rv;
-}
struct unix_domain {
struct auth_domain h;
===================================================================
This BitKeeper patch contains the following changesets:
+
## Wrapped with gzip_uu ##
M'XL( $XS2C\ ]58VW+;R!%])KYBJOPBV28X=PRTD8O>M;-QK7:MTJZKDDI2
MJB$P,!'AP@P 2MH@_YX>@+1(FI0H>OD073@ V6STY73WF7F!/E7&G@UB/;SS
M7J"_E%5]-OB<W_F%J>'^JBSA?C0M<S.*]=WHQMC"9*,L+9J[(?7E,"OKV /!
M2UU'4S0WMCH;$)]]>:>^GYFSP=7['S]=O+WRO/-S],-4%Y_-KZ9&Y^=>7=JY
MSN)JK.MI5A9^;751Y:;6?E3F[1?1EF),X5>0@&$A6R(Q#]J(Q(1H3DR,*5>2
M>_.TT/=#&XT+73?65&52.S_^OGS*/]<U,JPH(Y0RREHAA9#>.T1\0D.*,!MA
M-:("$74FY)G KS [PQBY,(T7X4&O%!IB[WOTQSKQ@Q>ACX5!56WC9G9R"NJ1
M;3*#ZJG)D<XR'P3@[U.1U@9)9/[=Z RE^2PSN2EJ7:=E4:$R65/PKZ:J45D8
MW_L)<?=T[_(A$=[PF3^>AS7VWJ!X6MZ:+*O&UL1373MW5V*=5",-_U4S,]:/
M.N\)!J<I#QAMN:2"M00''*M ZB@(HB")UP*\18-+F2!",$Q:@25V5CP>_MBF
M#I6CF;:STM:CF2TG9F%.GPR.B6J9$%BU5&LJ)Y(G0N@DB*-U<QY1M6H7Y9C*
M)^W*TLD(,I06G]>,82$+6TD59BV?$*,2B@&0.H1/UHW9_/Z:!80)]:0%:1%E
M36SZ8E[JFF[:0IDD\$J2@ C*HI@FANAU4W8K6C4*AZ0+2V'2;#*.*N,W177K
MF[CQ=;,"&] (&2_L+!I5\T@W]?2Z*=*[!P2!=2RD@" NX-6HD :&X#!@2@<;
MECVJ:]4XSD+AL#1S36N?<*5%;6QN?O^]O$ZJE:A1S!CA+5:*XS8PTDBA9,R"
M0*B(?X7N!RW;%*X:* E<@8'Z9I:/RRK._-)^7HG:$IMY/(KS85I&=3:<DZ6C
M 5%$TH"'+2.*!FULI"30^Y*0Q-289#O.M^M:@9D0BCS7)KYAD\ ME3+D;<AX
M(O4DBL+(8)KH/6SB7]LD*:=A-V=V>>'&SG&B>*!:U2FFK@EQ%H3=$&+K$PAF
M#]DZ@3@:DN.,H"N3EW.#;KY,$5W$R)I"YP:6Q,!E9"HW6Y82,%IZ1'Q$0WO;
M_<&HN-R9B /&S@<B$?%>+$H1_6F]X[SQWD$840 +4R#WH5\&TVCXIK/[?&FK
MNSO]#N1X+]<M@TZP:=+X0=#=.4$J R?8+X/"W%YO*#2W(+83=OP V.U=* >J
M7<!.J!:0$)(.=N+_$W9]T>\#.WXLV-$>=F$/N_ IV/5RW?(H[ +2P:Y;'H7=
M.E5R8/OCR=D#6W?[ 7^)N.TT#; EF"*LE3B4HD.7W!-=& W5,<'5%$UEXE4
M]51R T#K;AT FW>,(M5E9R</<8DZ+B'Z-O4,0^<AD@5" -\.0MXE$IKL?IDD
M1\[D#&I<URM[I?5M4+?9<<1N([,[73ZD-X2/MP:(E5I6Z&ZZYW!P7.[IU;8L
M8C^_O\G,!':#X^2^\INT](MRO2T\Q4D#S$D(]0*,7C'2TQ6R+U\AQQX<FXAP
MTZ&CSE\7]VY'#X&!Q X':#<0 @RQP6L<86,C^?2!R#=M9CT=Y68<E86):@B2
M4^A/[*,ZH;4H BJ![#)0KIY%$H[=QE=Y9[?EWD$ -AP[I)$+O"CB[5O=IQ/W
M#7OMC:F[KUX((@4E),!!ZZHUZ'(7[%^GQRG3MW&,^D/$X6T:KW9N5X*)C@RD
MLSLIV,CF=D\/(G..[)L[>%R!HJFVZ.72"JB-JEZ\M^!5J\<L3Z?Y^8<ZGLXF
MQM9C8"2V\JNRL9%)@%29C8/++<<]!%#"%>.M9$2J9_9A>:P$?UA.X*UI=K7:
M'4YM9'?5O4-R*A1'1'K_\$8O7WI?$HJ&Z&V6E9$;"!I%Y>S>G8MJM'B2$QQ7
M9^BWJ5F\Y?853LQ'/[L#TZ*LT<2@7SY=7#AA)V]-W=BB<H>Q2,>Q-55WUMK=
M]H\"WO$:E;;[%@(. E\RUI;61W Y\GIT;0%<=>K]QQOTUW8.'/\F[S2>@&QF
MBI/J]!5YC7[\\^7U3^^O?GE_ 0 =I DZL?-3V$. 4#2[AYO7J'*?]'8B.__.
M^Z_W_J^7'Z]^N_[U;S]___'BI'^VP_?RD#Z:FNBF:O+S4$#343KT_@?PI/0:
$%Q@
--
Dan Aloni
da-x@gmx.net
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:14 [BK PATCH] One strdup() to rule them all Dan Aloni
@ 2003-08-25 16:25 ` Jakub Jelinek
2003-08-25 16:34 ` Dan Aloni
2003-08-25 17:05 ` Jeff Garzik
2003-08-25 16:37 ` Jörn Engel
1 sibling, 2 replies; 17+ messages in thread
From: Jakub Jelinek @ 2003-08-25 16:25 UTC (permalink / raw)
To: Dan Aloni; +Cc: Linux Kernel List
On Mon, Aug 25, 2003 at 07:14:35PM +0300, Dan Aloni wrote:
> diff -Nru a/lib/string.c b/lib/string.c
> --- a/lib/string.c Mon Aug 25 19:03:26 2003
> +++ b/lib/string.c Mon Aug 25 19:03:26 2003
> @@ -582,3 +582,19 @@
> }
>
> #endif
> +
> +/**
> + * strdup - Allocate a copy of a string.
> + * @s: The string to copy. Must not be NULL.
> + *
> + * returns the address of the allocation, or NULL on
> + * error.
> + */
> +char *strdup(const char *s)
> +{
> + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> + if (rv)
> + strcpy(rv, s);
> + return rv;
> +}
> +EXPORT_SYMBOL(strdup);
Better save strlen(s)+1 in a local size_t variable and use memcpy instead
of strcpy.
Jakub
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:25 ` Jakub Jelinek
@ 2003-08-25 16:34 ` Dan Aloni
2003-08-25 17:05 ` Jeff Garzik
1 sibling, 0 replies; 17+ messages in thread
From: Dan Aloni @ 2003-08-25 16:34 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Linux Kernel List
On Mon, Aug 25, 2003 at 12:25:32PM -0400, Jakub Jelinek wrote:
> On Mon, Aug 25, 2003 at 07:14:35PM +0300, Dan Aloni wrote:
> > diff -Nru a/lib/string.c b/lib/string.c
> > --- a/lib/string.c Mon Aug 25 19:03:26 2003
> > +++ b/lib/string.c Mon Aug 25 19:03:26 2003
> > @@ -582,3 +582,19 @@
> > }
> >
> > #endif
> > +
> > +/**
> > + * strdup - Allocate a copy of a string.
> > + * @s: The string to copy. Must not be NULL.
> > + *
> > + * returns the address of the allocation, or NULL on
> > + * error.
> > + */
> > +char *strdup(const char *s)
> > +{
> > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > + if (rv)
> > + strcpy(rv, s);
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(strdup);
>
> Better save strlen(s)+1 in a local size_t variable and use memcpy instead
> of strcpy.
Maybe, but no optimizations like this are needed for a function
such as strdup() which is called relatively rare.
--
Dan Aloni
da-x@gmx.net
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:25 ` Jakub Jelinek
2003-08-25 16:34 ` Dan Aloni
@ 2003-08-25 17:05 ` Jeff Garzik
2003-08-25 17:23 ` Dan Aloni
2003-08-25 17:49 ` Andries Brouwer
1 sibling, 2 replies; 17+ messages in thread
From: Jeff Garzik @ 2003-08-25 17:05 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Dan Aloni, Linux Kernel List
On Mon, Aug 25, 2003 at 12:25:32PM -0400, Jakub Jelinek wrote:
> On Mon, Aug 25, 2003 at 07:14:35PM +0300, Dan Aloni wrote:
> > diff -Nru a/lib/string.c b/lib/string.c
> > --- a/lib/string.c Mon Aug 25 19:03:26 2003
> > +++ b/lib/string.c Mon Aug 25 19:03:26 2003
> > @@ -582,3 +582,19 @@
> > }
> >
> > #endif
> > +
> > +/**
> > + * strdup - Allocate a copy of a string.
> > + * @s: The string to copy. Must not be NULL.
> > + *
> > + * returns the address of the allocation, or NULL on
> > + * error.
> > + */
> > +char *strdup(const char *s)
> > +{
> > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > + if (rv)
> > + strcpy(rv, s);
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(strdup);
>
> Better save strlen(s)+1 in a local size_t variable and use memcpy instead
> of strcpy.
Yep. When Rusty did his strdup cleanup, he followed my suggestion and
did just that.
Unfortunately Linus doesn't like the strdup cleanup, so I don't see this
patch going in either :)
Jeff
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 17:05 ` Jeff Garzik
@ 2003-08-25 17:23 ` Dan Aloni
2003-08-25 17:49 ` Andries Brouwer
1 sibling, 0 replies; 17+ messages in thread
From: Dan Aloni @ 2003-08-25 17:23 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Jakub Jelinek, Linux Kernel List
On Mon, Aug 25, 2003 at 01:05:30PM -0400, Jeff Garzik wrote:
> On Mon, Aug 25, 2003 at 12:25:32PM -0400, Jakub Jelinek wrote:
> > On Mon, Aug 25, 2003 at 07:14:35PM +0300, Dan Aloni wrote:
> > > diff -Nru a/lib/string.c b/lib/string.c
> > > --- a/lib/string.c Mon Aug 25 19:03:26 2003
> > > +++ b/lib/string.c Mon Aug 25 19:03:26 2003
> > > @@ -582,3 +582,19 @@
> > > }
> > >
> > > #endif
> > > +
> > > +/**
> > > + * strdup - Allocate a copy of a string.
> > > + * @s: The string to copy. Must not be NULL.
> > > + *
> > > + * returns the address of the allocation, or NULL on
> > > + * error.
> > > + */
> > > +char *strdup(const char *s)
> > > +{
> > > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > > + if (rv)
> > > + strcpy(rv, s);
> > > + return rv;
> > > +}
> > > +EXPORT_SYMBOL(strdup);
> >
> > Better save strlen(s)+1 in a local size_t variable and use memcpy instead
> > of strcpy.
>
> Yep. When Rusty did his strdup cleanup, he followed my suggestion and
> did just that.
>
> Unfortunately Linus doesn't like the strdup cleanup, so I don't see this
> patch going in either :)
Perhaps Linus would like to keep strdup()'s scattered across the kernel,
so their combined power would not be exploited by evil <insert silly LotR
humor here>. :) I'll end this thread here.
--
Dan Aloni
da-x@gmx.net
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 17:05 ` Jeff Garzik
2003-08-25 17:23 ` Dan Aloni
@ 2003-08-25 17:49 ` Andries Brouwer
2003-09-11 16:22 ` Jörn Engel
1 sibling, 1 reply; 17+ messages in thread
From: Andries Brouwer @ 2003-08-25 17:49 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Jakub Jelinek, Dan Aloni, Linux Kernel List
On Mon, Aug 25, 2003 at 01:05:30PM -0400, Jeff Garzik wrote:
> > > +char *strdup(const char *s)
> > > +{
> > > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > > + if (rv)
> > > + strcpy(rv, s);
> > > + return rv;
> > > +}
> Unfortunately Linus doesn't like the strdup cleanup, so I don't see this
> patch going in either :)
When seeing this my objection was: it introduces something with
a well-known name that uses GFP_KERNEL, so is not suitable everywhere -
an invitation to mistakes.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 17:49 ` Andries Brouwer
@ 2003-09-11 16:22 ` Jörn Engel
2003-09-12 1:57 ` Rusty Russell
0 siblings, 1 reply; 17+ messages in thread
From: Jörn Engel @ 2003-09-11 16:22 UTC (permalink / raw)
To: Andries Brouwer
Cc: Jeff Garzik, Jakub Jelinek, Dan Aloni, Linux Kernel List, Rusty Russell
On Mon, 25 August 2003 19:49:18 +0200, Andries Brouwer wrote:
> On Mon, Aug 25, 2003 at 01:05:30PM -0400, Jeff Garzik wrote:
>
> > > > +char *strdup(const char *s)
> > > > +{
> > > > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > > > + if (rv)
> > > > + strcpy(rv, s);
> > > > + return rv;
> > > > +}
>
> > Unfortunately Linus doesn't like the strdup cleanup, so I don't see this
> > patch going in either :)
>
> When seeing this my objection was: it introduces something with
> a well-known name that uses GFP_KERNEL, so is not suitable everywhere -
> an invitation to mistakes.
Andries, would you still object this function?
char *strdup(const char *s, int flags)
{
char *rv = kmalloc(strlen(s)+1, flags);
if (rv)
strcpy(rv, s);
return rv;
}
strdup(foo, GFP_KERNEL) should give most people enough clue to know
what they are doing.
Jörn
--
But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-09-11 16:22 ` Jörn Engel
@ 2003-09-12 1:57 ` Rusty Russell
2003-09-12 3:12 ` Jeff Garzik
0 siblings, 1 reply; 17+ messages in thread
From: Rusty Russell @ 2003-09-12 1:57 UTC (permalink / raw)
To: Jörn Engel, Andries Brouwer
Cc: Jeff Garzik, Jakub Jelinek, Dan Aloni, Linux Kernel List, torvalds
In message <20030911162223.GB3989@wohnheim.fh-wedel.de> you write:
> Andries, would you still object this function?
>
> char *strdup(const char *s, int flags)
> {
> char *rv = kmalloc(strlen(s)+1, flags);
> if (rv)
> strcpy(rv, s);
> return rv;
> }
No. We've been here. There are only around 50 users/potential users
of such a thing in the kernel, and seven implementations, but Linus
doesn't like it, so let's not waste more time on this please.
Rusty.
PS. kstrdrup is a better name since the args are different, a-la
kmalloc.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-09-12 1:57 ` Rusty Russell
@ 2003-09-12 3:12 ` Jeff Garzik
2003-09-12 4:16 ` Rusty Russell
0 siblings, 1 reply; 17+ messages in thread
From: Jeff Garzik @ 2003-09-12 3:12 UTC (permalink / raw)
To: Rusty Russell
Cc: Jörn Engel, Andries Brouwer, Jakub Jelinek, Dan Aloni,
Linux Kernel List, torvalds
Rusty Russell wrote:
> No. We've been here. There are only around 50 users/potential users
> of such a thing in the kernel, and seven implementations, but Linus
> doesn't like it, so let's not waste more time on this please.
Of course, if we DO waste time on it, your implementation Rusty kicks
ass and takes steroids :)
Jeff
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [BK PATCH] One strdup() to rule them all
2003-09-12 3:12 ` Jeff Garzik
@ 2003-09-12 4:16 ` Rusty Russell
2003-09-12 8:49 ` Bernd Petrovitsch
0 siblings, 1 reply; 17+ messages in thread
From: Rusty Russell @ 2003-09-12 4:16 UTC (permalink / raw)
To: Jeff Garzik
Cc: Jörn Engel, Andries Brouwer, Jakub Jelinek, Dan Aloni,
Linux Kernel List, torvalds
In message <3F6139AD.6070603@pobox.com> you write:
> Of course, if we DO waste time on it, your implementation Rusty kicks
> ass and takes steroids :)
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.0-test5-bk2/MAINTAINERS working-2.6.0-test5-bk2-modules_txt_kconfig1/MAINTAINERS
--- linux-2.6.0-test5-bk2/MAINTAINERS 2003-09-09 10:34:22.000000000 +1000
+++ working-2.6.0-test5-bk2-modules_txt_kconfig1/MAINTAINERS 2003-09-12 14:15:42.000000000 +1000
@@ -1102,6 +1102,13 @@ W: http://nfs.sourceforge.net/
W: http://www.cse.unsw.edu.au/~neilb/patches/linux-devel/
S: Maintained
+KSTRDUP
+P: Kstrdup Core Team
+M: kstrdup-core@ozlabs.org
+L: kstrdup@linux.kernel.org
+W: http://kstrdup.sourceforge.net/
+S: Supported
+
LANMEDIA WAN CARD DRIVER
P: Andrew Stanley-Jones
M: asj@lanmedia.com
Kill me now,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [BK PATCH] One strdup() to rule them all
2003-09-12 4:16 ` Rusty Russell
@ 2003-09-12 8:49 ` Bernd Petrovitsch
0 siblings, 0 replies; 17+ messages in thread
From: Bernd Petrovitsch @ 2003-09-12 8:49 UTC (permalink / raw)
To: Rusty Russell
Cc: Jeff Garzik, Jörn Engel, Andries Brouwer, Jakub Jelinek,
Dan Aloni, Linux Kernel List, torvalds
On Fre, 2003-09-12 at 06:16, Rusty Russell wrote:
> In message <3F6139AD.6070603@pobox.com> you write:
> > Of course, if we DO waste time on it, your implementation Rusty kicks
> > ass and takes steroids :)
>
> diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.0-test5-bk2/MAINTAINERS working-2.6.0-test5-bk2-modules_txt_kconfig1/MAINTAINERS
> --- linux-2.6.0-test5-bk2/MAINTAINERS 2003-09-09 10:34:22.000000000 +1000
> +++ working-2.6.0-test5-bk2-modules_txt_kconfig1/MAINTAINERS 2003-09-12 14:15:42.000000000 +1000
> @@ -1102,6 +1102,13 @@ W: http://nfs.sourceforge.net/
> W: http://www.cse.unsw.edu.au/~neilb/patches/linux-devel/
> S: Maintained
>
> +KSTRDUP
> +P: Kstrdup Core Team
> +M: kstrdup-core@ozlabs.org
> +L: kstrdup@linux.kernel.org
> +W: http://kstrdup.sourceforge.net/
> +S: Supported
> +
> LANMEDIA WAN CARD DRIVER
> P: Andrew Stanley-Jones
> M: asj@lanmedia.com
>
> Kill me now,
;-)
And kcalloc() [or whatever it should be called] falls probably in the
same category.
Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:14 [BK PATCH] One strdup() to rule them all Dan Aloni
2003-08-25 16:25 ` Jakub Jelinek
@ 2003-08-25 16:37 ` Jörn Engel
2003-08-25 16:55 ` Dan Aloni
2003-08-25 17:04 ` Jeff Garzik
1 sibling, 2 replies; 17+ messages in thread
From: Jörn Engel @ 2003-08-25 16:37 UTC (permalink / raw)
To: Dan Aloni; +Cc: Linux Kernel List
On Mon, 25 August 2003 19:14:35 +0300, Dan Aloni wrote:
>
> While working on the fix to network devices names and sysctl,
> I fought to urge to create yet another strdup() implementation
> This came up.
Nice one.
> +/**
> + * strdup - Allocate a copy of a string.
> + * @s: The string to copy. Must not be NULL.
> + *
> + * returns the address of the allocation, or NULL on
> + * error.
> + */
> +char *strdup(const char *s)
> +{
> + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> + if (rv)
> + strcpy(rv, s);
> + return rv;
> +}
My gut feeling is always afraid when something "must not be NULL",
someone will ignore this and Bad Things (tm) happen. Is strdup ever
used such performance critical code that the extra check would hurt?
Apart from that, well done.
Jörn
--
Eighty percent of success is showing up.
-- Woody Allen
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:37 ` Jörn Engel
@ 2003-08-25 16:55 ` Dan Aloni
2003-08-25 17:02 ` Jörn Engel
2003-08-25 17:04 ` Jeff Garzik
1 sibling, 1 reply; 17+ messages in thread
From: Dan Aloni @ 2003-08-25 16:55 UTC (permalink / raw)
To: J?rn Engel; +Cc: Linux Kernel List
On Mon, Aug 25, 2003 at 06:37:45PM +0200, J?rn Engel wrote:
> > +/**
> > + * strdup - Allocate a copy of a string.
> > + * @s: The string to copy. Must not be NULL.
> > + *
> > + * returns the address of the allocation, or NULL on
> > + * error.
> > + */
> > +char *strdup(const char *s)
> > +{
> > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > + if (rv)
> > + strcpy(rv, s);
> > + return rv;
> > +}
>
> My gut feeling is always afraid when something "must not be NULL",
> someone will ignore this and Bad Things (tm) happen. Is strdup ever
> used such performance critical code that the extra check would hurt?
There are two reasons while it shouldn't have a NULL check. One,
persistency: the other str*() functions don't do this sort of check.
Two, for general uses like that:
new_name = strdup(name);
if (!new_name)
goto allocation_failed;
With this check, NULL would be returning from strdup() either because
name == NULL or when the allocation fails. You cannot simply pass that
information through its return value and the caller would need to
check whether name == NULL by itself which should have been done anyway.
Passing NULL to strdup() is a bug, which would have NOT show as an
Oops if you add this check, and that is bad. Maybe it would be worth
to add a BUG_ON(s == NULL) instead, and perhaps also add this to the
other str*() functions, but that is a different patch.
--
Dan Aloni
da-x@gmx.net
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:55 ` Dan Aloni
@ 2003-08-25 17:02 ` Jörn Engel
0 siblings, 0 replies; 17+ messages in thread
From: Jörn Engel @ 2003-08-25 17:02 UTC (permalink / raw)
To: Dan Aloni; +Cc: Linux Kernel List
On Mon, 25 August 2003 19:55:12 +0300, Dan Aloni wrote:
> >
> > My gut feeling is always afraid when something "must not be NULL",
> > someone will ignore this and Bad Things (tm) happen. Is strdup ever
> > used such performance critical code that the extra check would hurt?
>
> There are two reasons while it shouldn't have a NULL check. One,
> persistency: the other str*() functions don't do this sort of check.
> Two, for general uses like that:
>
> new_name = strdup(name);
> if (!new_name)
> goto allocation_failed;
>
> With this check, NULL would be returning from strdup() either because
> name == NULL or when the allocation fails.
True, I missed that one.
> Passing NULL to strdup() is a bug, which would have NOT show as an
> Oops if you add this check, and that is bad. Maybe it would be worth
> to add a BUG_ON(s == NULL) instead, and perhaps also add this to the
> other str*() functions, but that is a different patch.
Ok. Will you write that patch then or do I have to put it onto my
list?
Jörn
--
When in doubt, use brute force.
-- Ken Thompson
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 16:37 ` Jörn Engel
2003-08-25 16:55 ` Dan Aloni
@ 2003-08-25 17:04 ` Jeff Garzik
2003-08-25 17:08 ` Jörn Engel
1 sibling, 1 reply; 17+ messages in thread
From: Jeff Garzik @ 2003-08-25 17:04 UTC (permalink / raw)
To: J?rn Engel; +Cc: Dan Aloni, Linux Kernel List
On Mon, Aug 25, 2003 at 06:37:45PM +0200, J?rn Engel wrote:
> On Mon, 25 August 2003 19:14:35 +0300, Dan Aloni wrote:
> >
> > While working on the fix to network devices names and sysctl,
> > I fought to urge to create yet another strdup() implementation
> > This came up.
>
> Nice one.
>
> > +/**
> > + * strdup - Allocate a copy of a string.
> > + * @s: The string to copy. Must not be NULL.
> > + *
> > + * returns the address of the allocation, or NULL on
> > + * error.
> > + */
> > +char *strdup(const char *s)
> > +{
> > + char *rv = kmalloc(strlen(s)+1, GFP_KERNEL);
> > + if (rv)
> > + strcpy(rv, s);
> > + return rv;
> > +}
>
> My gut feeling is always afraid when something "must not be NULL",
> someone will ignore this and Bad Things (tm) happen. Is strdup ever
> used such performance critical code that the extra check would hurt?
>
> Apart from that, well done.
Rusty created this patch, a long time ago. :)
Jeff
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 17:04 ` Jeff Garzik
@ 2003-08-25 17:08 ` Jörn Engel
2003-08-25 17:22 ` Jeff Garzik
0 siblings, 1 reply; 17+ messages in thread
From: Jörn Engel @ 2003-08-25 17:08 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Dan Aloni, Linux Kernel List
On Mon, 25 August 2003 13:04:41 -0400, Jeff Garzik wrote:
>
> Rusty created this patch, a long time ago. :)
But it never got merged? Was there some technical issue or was Rusty
just too lazy to keep bugging Linus?
Jörn
--
Correctness comes second.
Features come third.
Performance comes last.
Maintainability is needed for all of them.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [BK PATCH] One strdup() to rule them all
2003-08-25 17:08 ` Jörn Engel
@ 2003-08-25 17:22 ` Jeff Garzik
0 siblings, 0 replies; 17+ messages in thread
From: Jeff Garzik @ 2003-08-25 17:22 UTC (permalink / raw)
To: J?rn Engel; +Cc: Dan Aloni, Linux Kernel List
On Mon, Aug 25, 2003 at 07:08:25PM +0200, J?rn Engel wrote:
> On Mon, 25 August 2003 13:04:41 -0400, Jeff Garzik wrote:
> >
> > Rusty created this patch, a long time ago. :)
>
> But it never got merged? Was there some technical issue or was Rusty
> just too lazy to keep bugging Linus?
No, Rusty constantly submits it to Linus :)
Linus doesn't like consolidating strdup :)
Jeff
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2003-09-12 8:49 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-08-25 16:14 [BK PATCH] One strdup() to rule them all Dan Aloni
2003-08-25 16:25 ` Jakub Jelinek
2003-08-25 16:34 ` Dan Aloni
2003-08-25 17:05 ` Jeff Garzik
2003-08-25 17:23 ` Dan Aloni
2003-08-25 17:49 ` Andries Brouwer
2003-09-11 16:22 ` Jörn Engel
2003-09-12 1:57 ` Rusty Russell
2003-09-12 3:12 ` Jeff Garzik
2003-09-12 4:16 ` Rusty Russell
2003-09-12 8:49 ` Bernd Petrovitsch
2003-08-25 16:37 ` Jörn Engel
2003-08-25 16:55 ` Dan Aloni
2003-08-25 17:02 ` Jörn Engel
2003-08-25 17:04 ` Jeff Garzik
2003-08-25 17:08 ` Jörn Engel
2003-08-25 17:22 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).