[PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Jesper Juhl]

Convert strtok() use to strsep() in usr/gen_init_cpio.c

I've compile tested this patch and it compiles fine.
I build a 2.6.13-rc6-mm2 kernel with the patch applied without problems, and
the resulting kernel boots and runs just fine (using it right now).
But despite this basic testing it would still be nice if someone would 
double-check that I haven't made some silly mistake that would break some 
other setup than mine.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 gen_init_cpio.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)

--- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c	2005-06-17 21:48:29.000000000 +0200
+++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c	2005-08-24 18:58:21.000000000 +0200
@@ -438,7 +438,7 @@ struct file_handler file_handler_table[]
 int main (int argc, char *argv[])
 {
 	FILE *cpio_list;
-	char line[LINE_SIZE];
+	char *line, *ln;
 	char *args, *type;
 	int ec = 0;
 	int line_nr = 0;
@@ -455,7 +455,14 @@ int main (int argc, char *argv[])
 		exit(1);
 	}
 
-	while (fgets(line, LINE_SIZE, cpio_list)) {
+	ln = malloc(LINE_SIZE);
+	if (!ln) {
+		fprintf(stderr,
+			"ERROR: unable to allocate %d bytes of buffer\n", LINE_SIZE);
+		exit(1);
+	}
+
+	while (line = ln, fgets(line, LINE_SIZE, cpio_list)) {
 		int type_idx;
 		size_t slen = strlen(line);
 
@@ -466,14 +473,15 @@ int main (int argc, char *argv[])
 			continue;
 		}
 
-		if (! (type = strtok(line, " \t"))) {
+		if (! (type = strsep(&line, " \t"))) {
 			fprintf(stderr,
 				"ERROR: incorrect format, could not locate file type line %d: '%s'\n",
-				line_nr, line);
+				line_nr, ln);
 			ec = -1;
+			continue;
 		}
 
-		if ('\n' == *type) {
+		if (!*type || '\n' == *type) {
 			/* a blank line */
 			continue;
 		}
@@ -483,16 +491,20 @@ int main (int argc, char *argv[])
 			continue;
 		}
 
-		if (! (args = strtok(NULL, "\n"))) {
+		if (! (args = strsep(&line, "\n"))) {
 			fprintf(stderr,
 				"ERROR: incorrect format, newline required line %d: '%s'\n",
-				line_nr, line);
+				line_nr, ln);
 			ec = -1;
+			continue;
 		}
+		
+		if (!*args)
+			continue;
 
 		for (type_idx = 0; file_handler_table[type_idx].type; type_idx++) {
 			int rc;
-			if (! strcmp(line, file_handler_table[type_idx].type)) {
+			if (! strcmp(type, file_handler_table[type_idx].type)) {
 				if ((rc = file_handler_table[type_idx].handler(args))) {
 					ec = rc;
 					fprintf(stderr, " line %d\n", line_nr);
@@ -503,9 +515,10 @@ int main (int argc, char *argv[])
 
 		if (NULL == file_handler_table[type_idx].type) {
 			fprintf(stderr, "unknown file type line %d: '%s'\n",
-				line_nr, line);
+				line_nr, ln);
 		}
 	}
+	free(ln);
 	cpio_trailer();
 
 	exit(ec);

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Brian Gerst]

Jesper Juhl wrote:
> Convert strtok() use to strsep() in usr/gen_init_cpio.c
> 
> I've compile tested this patch and it compiles fine.
> I build a 2.6.13-rc6-mm2 kernel with the patch applied without problems, and
> the resulting kernel boots and runs just fine (using it right now).
> But despite this basic testing it would still be nice if someone would 
> double-check that I haven't made some silly mistake that would break some 
> other setup than mine.
> 
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
> 
>  gen_init_cpio.c |   31 ++++++++++++++++++++++---------
>  1 files changed, 22 insertions(+), 9 deletions(-)
> 
> --- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c	2005-06-17 21:48:29.000000000 +0200
> +++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c	2005-08-24 18:58:21.000000000 +0200
> @@ -438,7 +438,7 @@ struct file_handler file_handler_table[]
>  int main (int argc, char *argv[])
>  {
>  	FILE *cpio_list;
> -	char line[LINE_SIZE];
> +	char *line, *ln;
>  	char *args, *type;
>  	int ec = 0;
>  	int line_nr = 0;
> @@ -455,7 +455,14 @@ int main (int argc, char *argv[])
>  		exit(1);
>  	}
>  
> -	while (fgets(line, LINE_SIZE, cpio_list)) {
> +	ln = malloc(LINE_SIZE);

Why change to malloc()?  This is a userspace program.  It doesn't have 
the kernel stack constraints.

--
				Brian Gerst

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Jeff Garzik]

Brian Gerst wrote:
> Jesper Juhl wrote:
> 
>> Convert strtok() use to strsep() in usr/gen_init_cpio.c
>>
>> I've compile tested this patch and it compiles fine.
>> I build a 2.6.13-rc6-mm2 kernel with the patch applied without 
>> problems, and
>> the resulting kernel boots and runs just fine (using it right now).
>> But despite this basic testing it would still be nice if someone would 
>> double-check that I haven't made some silly mistake that would break 
>> some other setup than mine.
>>
>>
>> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
>> ---
>>
>>  gen_init_cpio.c |   31 ++++++++++++++++++++++---------
>>  1 files changed, 22 insertions(+), 9 deletions(-)
>>
>> --- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c    2005-06-17 
>> 21:48:29.000000000 +0200
>> +++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c    2005-08-24 
>> 18:58:21.000000000 +0200
>> @@ -438,7 +438,7 @@ struct file_handler file_handler_table[]
>>  int main (int argc, char *argv[])
>>  {
>>      FILE *cpio_list;
>> -    char line[LINE_SIZE];
>> +    char *line, *ln;
>>      char *args, *type;
>>      int ec = 0;
>>      int line_nr = 0;
>> @@ -455,7 +455,14 @@ int main (int argc, char *argv[])
>>          exit(1);
>>      }
>>  
>> -    while (fgets(line, LINE_SIZE, cpio_list)) {
>> +    ln = malloc(LINE_SIZE);
> 
> 
> Why change to malloc()?  This is a userspace program.  It doesn't have 
> the kernel stack constraints.

Good catch, agreed.

I prefer the code as-is, with LINE_SIZE stack allocations.

	Jeff

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Jesper Juhl]

On 8/24/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> Brian Gerst wrote:
> > Jesper Juhl wrote:
> >
> >> Convert strtok() use to strsep() in usr/gen_init_cpio.c
> >>
> >> I've compile tested this patch and it compiles fine.
> >> I build a 2.6.13-rc6-mm2 kernel with the patch applied without
> >> problems, and
> >> the resulting kernel boots and runs just fine (using it right now).
> >> But despite this basic testing it would still be nice if someone would
> >> double-check that I haven't made some silly mistake that would break
> >> some other setup than mine.
> >>
> >>
> >> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> >> ---
> >>
> >>  gen_init_cpio.c |   31 ++++++++++++++++++++++---------
> >>  1 files changed, 22 insertions(+), 9 deletions(-)
> >>
> >> --- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c    2005-06-17
> >> 21:48:29.000000000 +0200
> >> +++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c    2005-08-24
> >> 18:58:21.000000000 +0200
> >> @@ -438,7 +438,7 @@ struct file_handler file_handler_table[]
> >>  int main (int argc, char *argv[])
> >>  {
> >>      FILE *cpio_list;
> >> -    char line[LINE_SIZE];
> >> +    char *line, *ln;
> >>      char *args, *type;
> >>      int ec = 0;
> >>      int line_nr = 0;
> >> @@ -455,7 +455,14 @@ int main (int argc, char *argv[])
> >>          exit(1);
> >>      }
> >>
> >> -    while (fgets(line, LINE_SIZE, cpio_list)) {
> >> +    ln = malloc(LINE_SIZE);
> >
> >
> > Why change to malloc()?  This is a userspace program.  It doesn't have
> > the kernel stack constraints.
> 
> Good catch, agreed.
> 
> I prefer the code as-is, with LINE_SIZE stack allocations.
> 
The reason I did it like that was that strsep takes offense at
strsep(&line, ...) when line is allocated on the stack. So I just
changed it around to being malloc()'ed and things were good.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Brian Gerst]

Jesper Juhl wrote:
> On 8/24/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>>Brian Gerst wrote:
>>
>>>Jesper Juhl wrote:
>>>
>>>
>>>>Convert strtok() use to strsep() in usr/gen_init_cpio.c
>>>>
>>>>I've compile tested this patch and it compiles fine.
>>>>I build a 2.6.13-rc6-mm2 kernel with the patch applied without
>>>>problems, and
>>>>the resulting kernel boots and runs just fine (using it right now).
>>>>But despite this basic testing it would still be nice if someone would
>>>>double-check that I haven't made some silly mistake that would break
>>>>some other setup than mine.
>>>>
>>>>
>>>>Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
>>>>---
>>>>
>>>> gen_init_cpio.c |   31 ++++++++++++++++++++++---------
>>>> 1 files changed, 22 insertions(+), 9 deletions(-)
>>>>
>>>>--- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c    2005-06-17
>>>>21:48:29.000000000 +0200
>>>>+++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c    2005-08-24
>>>>18:58:21.000000000 +0200
>>>>@@ -438,7 +438,7 @@ struct file_handler file_handler_table[]
>>>> int main (int argc, char *argv[])
>>>> {
>>>>     FILE *cpio_list;
>>>>-    char line[LINE_SIZE];
>>>>+    char *line, *ln;
>>>>     char *args, *type;
>>>>     int ec = 0;
>>>>     int line_nr = 0;
>>>>@@ -455,7 +455,14 @@ int main (int argc, char *argv[])
>>>>         exit(1);
>>>>     }
>>>>
>>>>-    while (fgets(line, LINE_SIZE, cpio_list)) {
>>>>+    ln = malloc(LINE_SIZE);
>>>
>>>
>>>Why change to malloc()?  This is a userspace program.  It doesn't have
>>>the kernel stack constraints.
>>
>>Good catch, agreed.
>>
>>I prefer the code as-is, with LINE_SIZE stack allocations.
>>
> 
> The reason I did it like that was that strsep takes offense at
> strsep(&line, ...) when line is allocated on the stack. So I just
> changed it around to being malloc()'ed and things were good.
> 

Do this instead:
	char ln[LINE_SIZE], *line;

--
				Brian Gerst

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Horst von Brand]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
> Convert strtok() use to strsep() in usr/gen_init_cpio.c

This is userland code...

No, I'm not looking it over carfully, just a fast look over.

> I've compile tested this patch and it compiles fine.

You should be able ti test it then.

> I build a 2.6.13-rc6-mm2 kernel with the patch applied without problems, and
> the resulting kernel boots and runs just fine (using it right now).
> But despite this basic testing it would still be nice if someone would 
> double-check that I haven't made some silly mistake that would break some 
> other setup than mine.
> 
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
> 
>  gen_init_cpio.c |   31 ++++++++++++++++++++++---------
>  1 files changed, 22 insertions(+), 9 deletions(-)
> 
> --- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c	2005-06-17 21:48:29.000000000 +0200
> +++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c	2005-08-24 18:58:21.000000000 +0200
> @@ -438,7 +438,7 @@ struct file_handler file_handler_table[]
>  int main (int argc, char *argv[])
>  {
>  	FILE *cpio_list;
> -	char line[LINE_SIZE];
> +	char *line, *ln;

No need for this, there is no particular stack frugality requirement with
user code.

>  	char *args, *type;
>  	int ec = 0;
>  	int line_nr = 0;
> @@ -455,7 +455,14 @@ int main (int argc, char *argv[])
>  		exit(1);
>  	}
>  
> -	while (fgets(line, LINE_SIZE, cpio_list)) {
> +	ln = malloc(LINE_SIZE);

Ditto.

[...]

> -		if ('\n' == *type) {
> +		if (!*type || '\n' == *type) {

Redundant. If *type == '\n', it is certainly != 0.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Jesper Juhl]

On Wednesday 24 August 2005 22:39, Brian Gerst wrote:
> 
> Do this instead:
> 	char ln[LINE_SIZE], *line;
> 
Right, now why didn't I think of that :)

Jeff: Does the patch below agree with you more?


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 usr/gen_init_cpio.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)
 
 --- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c	2005-06-17 21:48:29.000000000 +0200
+++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c	2005-08-24 23:10:36.000000000 +0200
@@ -438,7 +438,8 @@ struct file_handler file_handler_table[]
 int main (int argc, char *argv[])
 {
 	FILE *cpio_list;
-	char line[LINE_SIZE];
+	char ln[LINE_SIZE];
+	char *line;
 	char *args, *type;
 	int ec = 0;
 	int line_nr = 0;
@@ -455,7 +456,7 @@ int main (int argc, char *argv[])
 		exit(1);
 	}
 
-	while (fgets(line, LINE_SIZE, cpio_list)) {
+	while (line = ln, fgets(line, LINE_SIZE, cpio_list)) {
 		int type_idx;
 		size_t slen = strlen(line);
 
@@ -466,11 +467,12 @@ int main (int argc, char *argv[])
 			continue;
 		}
 
-		if (! (type = strtok(line, " \t"))) {
+		if (! (type = strsep(&line, " \t"))) {
 			fprintf(stderr,
 				"ERROR: incorrect format, could not locate file type line %d: '%s'\n",
-				line_nr, line);
+				line_nr, ln);
 			ec = -1;
+			continue;
 		}
 
 		if ('\n' == *type) {
@@ -483,16 +485,20 @@ int main (int argc, char *argv[])
 			continue;
 		}
 
-		if (! (args = strtok(NULL, "\n"))) {
+		if (! (args = strsep(&line, "\n"))) {
 			fprintf(stderr,
 				"ERROR: incorrect format, newline required line %d: '%s'\n",
-				line_nr, line);
+				line_nr, ln);
 			ec = -1;
+			continue;
 		}
+		
+		if (!*args)
+			continue;
 
 		for (type_idx = 0; file_handler_table[type_idx].type; type_idx++) {
 			int rc;
-			if (! strcmp(line, file_handler_table[type_idx].type)) {
+			if (! strcmp(type, file_handler_table[type_idx].type)) {
 				if ((rc = file_handler_table[type_idx].handler(args))) {
 					ec = rc;
 					fprintf(stderr, " line %d\n", line_nr);
@@ -503,7 +509,7 @@ int main (int argc, char *argv[])
 
 		if (NULL == file_handler_table[type_idx].type) {
 			fprintf(stderr, "unknown file type line %d: '%s'\n",
-				line_nr, line);
+				line_nr, ln);
 		}
 	}
 	cpio_trailer();

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Jesper Juhl]

On 8/24/05, Horst von Brand <vonbrand@inf.utfsm.cl> wrote:
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
[snip]
> 
> > -             if ('\n' == *type) {
> > +             if (!*type || '\n' == *type) {
> 
> Redundant. If *type == '\n', it is certainly != 0.

Hmm, I added that since if we get past the if above it, then "type"
might be !=NULL, but *type might be, but you are right, it's still
redundant since there's a check below that'll catch it if so.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Paul Jackson]

Horst wrote:
> > -		if ('\n' == *type) {
> > +		if (!*type || '\n' == *type) {
> 
> Redundant. If *type == '\n', it is certainly != 0.

No - I don't think redundant, at least not this change in isolation.
Perhaps redundant in light of subsequent code lines, as Jesper notes in
his followup.

But it is confusing to read - poor and inconsistent choice of code
phrasing.

If the patch had read as:
    -		if (*type == '\n') {
    +		if (*type == '\n' || *type == '\0') {

then it would be clearer to the reader in my view.  A check for newline
is changed to a check for newline or nul-byte.

(Yes - I recognize that one is not given the freedom to change the
-old- lines in a patch for the sake of clarity ;).

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Sam Ravnborg]

On Wed, Aug 24, 2005 at 09:08:53PM +0200, Jesper Juhl wrote:
> Convert strtok() use to strsep() in usr/gen_init_cpio.c
> 
> I've compile tested this patch and it compiles fine.
> I build a 2.6.13-rc6-mm2 kernel with the patch applied without problems, and
> the resulting kernel boots and runs just fine (using it right now).
> But despite this basic testing it would still be nice if someone would 
> double-check that I haven't made some silly mistake that would break some 
> other setup than mine.
This is userland - I see no point in uglyfying the code.

	Sam

Re: [PATCH 3/3] exterminate strtok - usr/gen_init_cpio.c

[Horst von Brand]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On Wednesday 24 August 2005 22:39, Brian Gerst wrote:
> > 
> > Do this instead:
> > 	char ln[LINE_SIZE], *line;
> > 
> Right, now why didn't I think of that :)
> 
> Jeff: Does the patch below agree with you more?
> 
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
> 
>  usr/gen_init_cpio.c |   22 ++++++++++++++--------
>  1 files changed, 14 insertions(+), 8 deletions(-)
>  
>  --- linux-2.6.13-rc6-mm2-orig/usr/gen_init_cpio.c	2005-06-17 21:48:29.000000000 +0200
> +++ linux-2.6.13-rc6-mm2/usr/gen_init_cpio.c	2005-08-24 23:10:36.000000000 +0200
> @@ -438,7 +438,8 @@ struct file_handler file_handler_table[]
>  int main (int argc, char *argv[])
>  {
>  	FILE *cpio_list;
> -	char line[LINE_SIZE];
> +	char ln[LINE_SIZE];
> +	char *line;
>  	char *args, *type;
>  	int ec = 0;
>  	int line_nr = 0;
> @@ -455,7 +456,7 @@ int main (int argc, char *argv[])
>  		exit(1);
>  	}
>  
> -	while (fgets(line, LINE_SIZE, cpio_list)) {
> +	while (line = ln, fgets(line, LINE_SIZE, cpio_list)) {
>  		int type_idx;
>  		size_t slen = strlen(line);

The ',' in the while() isn't exactly readable... first order of bussiness
inside:

	while (fgets(line, LINE_SIZE, cpio_list)) {
	while (fgets(ln, LINE_SIZE, cpio_list)) {
		int type_idx;
		size_t slen = strlen(ln);

		line = ln;

Or just use the fact that fgets(3) returns the buffer if no error:

	while(line = fgets(ln, LINE_SIZE, cpio_list)) {

(yes, gcc will complain... wrap in () to shut it up)
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513