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-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 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-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 @@
 if (!options || !*options)
return 0;
 
-for