This fix leaves a variable uninitialized, and depends on funny logic to later give it a value.<p>The general structure should be<p><pre><code> FILE *fp = stdin; /* sane default */
</code></pre>
then override with a new pointer from fopen if necessary.<p>In fact, looking at this more closely, note how fp is scoped to the body of the for loop. That's where it should be defined:<p><pre><code> for (firsttime = 1; ; firsttime = 0) {
FILE *fp = stdin;
/* logic flips it to fp = fopen(...) as necessary */
}
</code></pre>
[Note: the C compiler Joy was working with probably didn't support block scope declarations except at the top of the function. This comment applies to merging the fix using the environment of today.]<p>Note how the fclose at the bottom of the loop ends up closing stdin in the case that fp == stdin! It ensures that this only happens once, but you have to analyze it to see that this is true. I would write this as:<p><pre><code> if (fp != stdin) /* defensive! */
fclose(fp);
</code></pre>
Making it an infinite loop terminated by calling exit is unnecessary. There is already a continue in the loop, and a break in an inner loop; another break instead of the exit, and a "return status;" at the bottom wouldn't hurt. Better yet:<p><pre><code> /* Enter loop whenever there are args left to
process, or even if there are no args if it is
the first time through (to process stdin instead). */
for (firsttime = 1; firsttime || *argv; firsttime = 0) {
FILE *fp = stdin;
if (*argv) {
/* have args; do the fopen to replace stdin */
/* set status = 1 and continue if cannot fopen */
}
/* do head logic */
if (fp != stdin)
fclose(fp);
}
return status;
</code></pre>
C itself was only a couple of years old when that code was written originally, and the systems were small. Whereas I've been using it since 1989. Not criticizing the original!
The extremely odd for-loop structure feels to me like someone was trying very hard to avoid a goto, and in the process obfuscated the flow quite a bit more. The special case of head'ing stdin is similar to the classic "loop and a half problem" (which is really only a problem if you religiously abstain from goto use.) I'd consider this a more straightforward way to do it:<p><pre><code> FILE *f;
int i = 1;
...
if(argc < 2) {
f = stdin;
goto do_head;
}
...
while(i<argc) {
f = fopen(argv[i], "r");
/* check for failure to open */
...
do_head:
/* head'ing code goes here */
...
fclose(f);
i++;
}
</code></pre>
That being said, OpenBSD code is still far more concise and readable than GNU; compare coreutils' head.c here:<p><a href="http://code.metager.de/source/xref/gnu/coreutils/src/head.c" rel="nofollow">http://code.metager.de/source/xref/gnu/coreutils/src/head.c</a><p>GNU head has a bit more functionality, but I don't think the increase in complexity - nearly 10x more lines - is proportional to that.
This link to the commit log is slightly better:<p><a href="http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/head.c" rel="nofollow">http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/he...</a><p>You're one click away from the diff.
One of my coworkers found a data integrity causing bug that has been around for 11 years.<p>He and his team lead had no idea how such a bug missed 11 years of peer-QA through all edits on that module / activity.<p>It was as simple as converting to string and back and hitting an edge case on internationalization with commas vs periods.
Here is the diff:<p><a href="http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/head.c.diff?r1=1.17&r2=1.18&f=h" rel="nofollow">http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/he...</a>