Re: [PATCH] strtok -> strsep (The Easy Cases)
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)
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)
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)
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)
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