Re: your mail
David Laight wrote: [Thu Jul 10 2008, 04:42:10PM EDT]
> On Thu, Jul 10, 2008 at 09:10:46AM -0500, agriffis@xxxxxxxxx wrote:
> > Clean up error handling in mutt_copy_header
> >
> > mutt_copy_header unnecessarily tests the result of each fputc/fputs (well,
> > most
> > of them anyway, it's not consistent). This obfuscates the code and hides
> > bugs.
> > Remove these extraneous checks since ferror/feof are checked at the bottom
> > of
> > the function, and get rid of all the early returns.
>
> You also want to add a fflush() before testing ferror().
Thanks! Reading fflush(3) I can see you're certainly right.
> Interestingly lint will complain if you don't test (or cast away) the
> result of fprintf(). Since fprintf() will only fail if the stdio
> buffer fills and the write() fails, almost all calls can never fail
> (unless bufferring is disabled). The write() call happens during
> fclose() - which probably can't return an appropriate error.
> So if you care about the result of the fprintf() calls, you need
> to do fflush(); ferror(); fclose().
> (Testing ferror() in a loop might be useful in order to abort early.)
Thanks, no loop here, but I'll keep that in mind. There are
other places in the code where ferror() is used without fflush()
but I'll leave that to another patch.
Updated patch with call to ferror() follows. The second and
third patches in the series continue to apply without
modification.
Aron
# HG changeset patch
# User Aron Griffis <agriffis@xxxxxxxxx>
# Date 1215733456 14400
# Branch HEAD
# Node ID 0f493412f651df08f7be8e1345f8b99395e0465e
# Parent b9ac445b035b0b25e3e137bd380d22a170fe7b0e
Clean up error handling in mutt_copy_header
mutt_copy_header unnecessarily tests the result of each fputc/fputs (well, most
of them anyway, it's not consistent). This obfuscates the code and hides bugs.
Remove these extraneous checks since fflush/ferror/feof are checked at the
bottom of the function, and get rid of all the early returns.
Signed-off-by: Aron Griffis <agriffis@xxxxxxxxx>
diff -r b9ac445b035b -r 0f493412f651 copy.c
--- a/copy.c Thu Jul 10 22:02:47 2008 +0200
+++ b/copy.c Thu Jul 10 19:44:16 2008 -0400
@@ -348,7 +348,7 @@
| (h->env->refs_changed ? CH_UPDATE_REFS : 0);
if (mutt_copy_hdr (in, out, h->offset, h->content->offset, flags, prefix) ==
-1)
- return (-1);
+ return -1;
if (flags & CH_TXTPLAIN)
{
@@ -360,10 +360,6 @@
rfc822_cat(buffer, sizeof(buffer), chsbuf, MimeSpecials);
fputs(buffer, out);
fputc('\n', out);
-
- if (ferror (out) != 0 || feof (out) != 0)
- return -1;
-
}
if (flags & CH_UPDATE)
@@ -373,24 +369,19 @@
if (h->env->irt_changed && h->env->in_reply_to)
{
LIST *listp = h->env->in_reply_to;
-
- if (fputs ("In-Reply-To: ", out) == EOF)
- return (-1);
-
+ fputs ("In-Reply-To: ", out);
for (; listp; listp = listp->next)
- if ((fputs (listp->data, out) == EOF) || (fputc (' ', out) == EOF))
- return (-1);
-
- if (fputc ('\n', out) == EOF)
- return (-1);
+ {
+ fputs (listp->data, out);
+ fputc (' ', out);
+ }
+ fputc ('\n', out);
}
if (h->env->refs_changed && h->env->references)
{
LIST *listp = h->env->references, *refs = NULL, *t;
-
- if (fputs ("References: ", out) == EOF)
- return (-1);
+ fputs ("References: ", out);
/* Mutt stores references in reverse order, thus we create
* a reordered refs list that we can put in the headers */
@@ -402,56 +393,36 @@
}
for (; refs; refs = refs->next)
- if ((fputs (refs->data, out) == EOF) || (fputc (' ', out) == EOF))
- return (-1);
+ {
+ fputs (refs->data, out);
+ fputc (' ', out);
+ }
/* clearing refs from memory */
for (t = refs; refs; refs = t->next, t = refs)
FREE (&refs);
- if (fputc ('\n', out) == EOF)
- return (-1);
+ fputc ('\n', out);
}
if (h->old || h->read)
{
- if (fputs ("Status: ", out) == EOF)
- return (-1);
-
+ fputs ("Status: ", out);
if (h->read)
- {
- if (fputs ("RO", out) == EOF)
- return (-1);
- }
+ fputs ("RO", out);
else if (h->old)
- {
- if (fputc ('O', out) == EOF)
- return (-1);
- }
-
- if (fputc ('\n', out) == EOF)
- return (-1);
+ fputc ('O', out);
+ fputc ('\n', out);
}
if (h->flagged || h->replied)
{
- if (fputs ("X-Status: ", out) == EOF)
- return (-1);
-
+ fputs ("X-Status: ", out);
if (h->replied)
- {
- if (fputc ('A', out) == EOF)
- return (-1);
- }
-
+ fputc ('A', out);
if (h->flagged)
- {
- if (fputc ('F', out) == EOF)
- return (-1);
- }
-
- if (fputc ('\n', out) == EOF)
- return (-1);
+ fputc ('F', out);
+ fputc ('\n', out);
}
}
}
@@ -468,14 +439,13 @@
{
if (flags & CH_PREFIX)
fputs(prefix, out);
- if (fputc ('\n', out) == EOF) /* add header terminator */
- return (-1);
+ fputc ('\n', out); /* add header terminator */
}
- if (ferror (out) || feof (out))
+ if (fflush (out) || ferror (out) || feof (out))
return -1;
- return (0);
+ return 0;
}
/* Count the number of lines and bytes to be deleted in this body*/