Re: [PATCH] strtok -> strsep (The Easy Cases)

2001-05-04 Thread Ren=E9=20Scharfe

Rusty Russell <[EMAIL PROTECTED]> schrieb am 04.05.01:
> In message <01050413055100.00907@golmepha> you write:
> > Am Freitag,  4. Mai 2001 02:57 schrieb Rusty Russell:
> > > There are two cases where the substitution is problematic:
> > 
> > Yes, but...
> > 
> > The cases which my patch modifies are of a different kind:
> 
> The very first hunk of your patch is wrong.  I didn't check the
> others.  Note that the declaration of switches is:

Oops. *blush*

I think all other chunks are OK, but I'll check that thoroughly when I'll be home 
again next monday.

Thank you for taking the time to look at my patch.

René

(who bites the table out of shame)
__
Ferienklick.de - 225 Reisekataloge auf einen Blick!
Direkt zu Ihrem Traumurlaub: http://ferienklick.de/?PP=2-0-100-105-0

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



Re: [PATCH] strtok -> strsep (The Easy Cases)

2001-05-04 Thread Rusty Russell

In message <01050413055100.00907@golmepha> you write:
> Am Freitag,  4. Mai 2001 02:57 schrieb Rusty Russell:
> > There are two cases where the substitution is problematic:
> 
> Yes, but...
> 
> The cases which my patch modifies are of a different kind:

The very first hunk of your patch is wrong.  I didn't check the
others.  Note that the declaration of switches is:

char switches[len+1];

--- linux-2.4.4/arch/m68k/atari/config.cTue Nov 28 02:57:34 2000
+++ linux-2.4.4-rs/arch/m68k/atari/config.c Tue May  1 17:03:46 2001
@@ -206,13 +206,15 @@
 
 /* parse the options */
-for( p = strtok( switches, "," ); p; p = strtok( NULL, "," ) ) {
+while( p = strsep( &switches, "," ) ) {
+   if (!*p)
+   continue;
ovsc_shift = 0;
if (strncmp( p, "ov_", 3 ) == 0) {
p += 3;

Cheers,
Rusty.
--
Premature optmztion is rt of all evl. --DK
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



Re: [PATCH] strtok -> strsep (The Easy Cases)

2001-05-04 Thread Rene Scharfe

Am Freitag,  4. Mai 2001 02:57 schrieb Rusty Russell:
> In message <01050120580701.01713@golmepha> you write:
> > Hello,

Hi!

> >
> > the patch at the bottom does the bulk job of strtok replacement. It's a
> > very boring patch, containing easy cases, only. It became a bit big, too,
> > but I trust you can digest it nevertheless. It's made against kernel
> > version 2.4.4.
>
> There are two cases where the substitution is problematic:

Yes, but...

The cases which my patch modifies are of a different kind:

int parse_options(char *options)
{
char *p;

/* for (p = strtok(options, ","); p; p = strtok(NULL, ",")) { */
while (p = strsep(&options, ",")) {
/* ... */
}
return 0;
}

No temporary array, no kfree(). Our variable "options" is used for strtsep,
only. Notice btw. how we destoy the string content to which "options"
points with both strtok and strsep.

That said, it's possible I made a stupid mistake, of course. Or two. Do
you agree on the correctness of the code above? If not, forget the whole
thing and I'll try again later.


>
> Array:
>   char tmparray[500];
>   strcpy(tmparray, str);
>
>   /* for (p = strtok(tmparray, "n"); p; p = strtok(NULL, "n")) { */
>   while ((p = strsep(&tmparray, ","))) {
>
> This is clearly wrong, and invokes a compiler warning.  &tmparray ==
> tmparray (a cute C oddity I've never really liked).  You are blowing
> away the first few characters in tmparray, and your parser won't work
> properly.
>
> Dynamic:
>
>   char *tmp = strdup(str);
>
>   /* for (p = strtok(tmp, "n"); p; p = strtok(NULL, "n")) { */
>   while ((p = strsep(&tmp, ","))) {
>   ...
>   }
>
>   kfree(tmp);
>
> Here, tmp has changed in the strsep implementation, and kfree will do
> bad things.
>
> There is a real reason to avoid strtok, and that is SMP and multple
> threads calling it at once (that said, I don't know of a problem yet).
> But this patch is a step backwards.
>
> Rusty.
> --
> Premature optmztion is rt of all evl. --DK


René

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



Re: [PATCH] strtok -> strsep (The Easy Cases)

2001-05-03 Thread Rusty Russell

In message <01050120580701.01713@golmepha> you write:
> Hello,
> 
> the patch at the bottom does the bulk job of strtok replacement. It's a
> very boring patch, containing easy cases, only. It became a bit big, too,
> but I trust you can digest it nevertheless. It's made against kernel
> version 2.4.4.

There are two cases where the substitution is problematic:

Array:
char tmparray[500];
strcpy(tmparray, str);

/* for (p = strtok(tmparray, "n"); p; p = strtok(NULL, "n")) { */
while ((p = strsep(&tmparray, ","))) {

This is clearly wrong, and invokes a compiler warning.  &tmparray ==
tmparray (a cute C oddity I've never really liked).  You are blowing
away the first few characters in tmparray, and your parser won't work
properly.

Dynamic:

char *tmp = strdup(str);

/* for (p = strtok(tmp, "n"); p; p = strtok(NULL, "n")) { */
while ((p = strsep(&tmp, ","))) {
...
}

kfree(tmp);

Here, tmp has changed in the strsep implementation, and kfree will do
bad things.

There is a real reason to avoid strtok, and that is SMP and multple
threads calling it at once (that said, I don't know of a problem yet).
But this patch is a step backwards.

Rusty.
--
Premature optmztion is rt of all evl. --DK
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



[PATCH] strtok -> strsep (The Easy Cases)

2001-05-01 Thread Rene Scharfe

Hello,

the patch at the bottom does the bulk job of strtok replacement. It's a
very boring patch, containing easy cases, only. It became a bit big, too,
but I trust you can digest it nevertheless. It's made against kernel
version 2.4.4.

What is the benefit of getting rid of strtok? It is for cutting strings
into pieces and it's used in argument parsing code of most file
systems and framebuffers. The problem is: strtok is not reentrant and its
manpage tells you to stop using it. There is a replacement function:
strsep. strsep has the added benefit of returning empty tokens, too. We
don't need strtok, it's a bug - do you need any more reasons for replacing
it? In the longer run I want the kernel to be completely cleaned from
strtok - expect more patches to come.

Please apply this patch to the official version of the kernel.


René




diff -ur linux-2.4.4/arch/m68k/atari/config.c linux-2.4.4-rs/arch/m68k/atari/config.c
--- linux-2.4.4/arch/m68k/atari/config.cTue Nov 28 02:57:34 2000
+++ linux-2.4.4-rs/arch/m68k/atari/config.c Tue May  1 17:03:46 2001
@@ -206,13 +206,15 @@
 char *p;
 int ovsc_shift;
 
-/* copy string to local array, strtok works destructively... */
+/* copy string to local array, strsep works destructively... */
 strncpy( switches, str, len );
 switches[len] = 0;
 atari_switches = 0;
 
 /* parse the options */
-for( p = strtok( switches, "," ); p; p = strtok( NULL, "," ) ) {
+while( p = strsep( &switches, "," ) ) {
+   if (!*p)
+   continue;
ovsc_shift = 0;
if (strncmp( p, "ov_", 3 ) == 0) {
p += 3;
diff -ur linux-2.4.4/drivers/scsi/ibmmca.c linux-2.4.4-rs/drivers/scsi/ibmmca.c
--- linux-2.4.4/drivers/scsi/ibmmca.c   Sat Mar  3 03:38:38 2001
+++ linux-2.4.4-rs/drivers/scsi/ibmmca.cTue May  1 17:48:14 2001
@@ -1406,9 +1406,9 @@
io_base = 0;
id_base = 0;
if (str) {
-  token = strtok(str,",");
   j = 0;
-  while (token) {
+  while (token = strsep(&str,",")) {
+if (!*token) continue;
 if (!strcmp(token,"activity")) display_mode |= LED_ACTIVITY;
 if (!strcmp(token,"display")) display_mode |= LED_DISP;
 if (!strcmp(token,"adisplay")) display_mode |= LED_ADISP;
@@ -1424,7 +1424,6 @@
  scsi_id[id_base++] = simple_strtoul(token,NULL,0);
j++;
 }
-token = strtok(NULL,",");
   }
} else if (ints) {
   for (i = 0; i < IM_MAX_HOSTS && 2*i+2 < ints[0]; i++) {
diff -ur linux-2.4.4/drivers/video/acornfb.c linux-2.4.4-rs/drivers/video/acornfb.c
--- linux-2.4.4/drivers/video/acornfb.c Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/acornfb.c  Tue May  1 17:03:46 2001
@@ -1527,7 +1527,7 @@
 
acornfb_init_fbinfo();
 
-   for (opt = strtok(options, ","); opt; opt = strtok(NULL, ",")) {
+   while (opt = strsep(&options, ",")) {
if (!*opt)
continue;
 
diff -ur linux-2.4.4/drivers/video/amifb.c linux-2.4.4-rs/drivers/video/amifb.c
--- linux-2.4.4/drivers/video/amifb.c   Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/amifb.cTue May  1 17:03:46 2001
@@ -1195,7 +1195,9 @@
if (!options || !*options)
return 0;
 
-   for (this_opt = strtok(options, ","); this_opt; this_opt = strtok(NULL, ",")) {
+   while (this_opt = strsep(&options, ",")) {
+   if (!*this_opt)
+   continue;
if (!strcmp(this_opt, "inverse")) {
amifb_inverse = 1;
fb_invert_cmaps();
diff -ur linux-2.4.4/drivers/video/atafb.c linux-2.4.4-rs/drivers/video/atafb.c
--- linux-2.4.4/drivers/video/atafb.c   Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/atafb.cTue May  1 17:03:46 2001
@@ -2899,7 +2899,7 @@
 if (!options || !*options)
return 0;
  
-for(this_opt=strtok(options,","); this_opt; this_opt=strtok(NULL,",")) {
+while (this_opt = strsep(&options, ",")) {
if (!*this_opt) continue;
if ((temp=get_video_mode(this_opt)))
default_par=temp;
diff -ur linux-2.4.4/drivers/video/aty128fb.c linux-2.4.4-rs/drivers/video/aty128fb.c
--- linux-2.4.4/drivers/video/aty128fb.cFri Feb  9 20:30:23 2001
+++ linux-2.4.4-rs/drivers/video/aty128fb.c Tue May  1 17:03:46 2001
@@ -1614,8 +1614,9 @@
 if (!options || !*options)
return 0;
 
-for (this_opt = strtok(options, ","); this_opt;
-this_opt = strtok(NULL, ",")) {
+while (this_opt = strsep(&options, ",")) {
+   if (!*this_opt)
+   continue;
if (!strncmp(this_opt, "font:", 5)) {
char *p;
int i;
diff -ur linux-2.4.4/drivers/video/atyfb.c linux-2.4.4-rs/drivers/video/atyfb.c
--- linux-2.4.4/drivers/video/atyfb.c   Sat Apr 28 12:23:20 2001
+++ linux-2.4.4-rs/drivers/video/atyfb.cTue May  1 17:03:46 2001
@@ -4062,8 +4062,9 @@
 i