Re: [PATCH] git-rev-list: give better diagnostic for failed write
- From
Linus Torvalds <torvalds@linux-foundation.org>
- Date
- Jun 25, 2007, 22:56 UTC
- Message-ID
- <alpine.LFD.0.98.0706251536240.8675@woody.linux-foundation.org>
- In-Reply-To
- <alpine.LFD.0.98.0706251505570.8675@woody.linux-foundation.org>
On Mon, 25 Jun 2007, Linus Torvalds wrote:
Show 8 quoted lines
>
> If you really really *must* get that ENOSPC error string output, create a
> helper function like
>
> void flush_or_die(FILE *f, const char *desc)
> {
> if (ferror(f))
> die("write failure on %s", desc)Actually, even this is really nasty, and it's a case where the current "git.c" code can also fail.
It's an example of where EPIPE can actually cause a write failure that you can never figure out what the reason for the error was, because the flush was caused by something earlier (write too much for the buffer or whatever), exactly because stdio throws the error away.
So after thinking about it some more, I would suggest just ignoring ferror entirely, and hoping that any errors are caught by the fflush().
I do hate stdio error checking. In my opinion, there really is only *one* correct way to use stdio error checking: ignore it. It doesn't work. The thing is fundamentally mis-designed for error handling.
So I think the right solution would literally be to either not do this broken error checking at all, or to rewrite the code that cares about errors to not use stdio.
There's also another issue: regular files really *are* different from pipes and sockets and other things. Not because of EPIPE, but because you want different buffering behaviour. For a regular file, we really don't even care about the line buffering, and we'd actually be better off (from a performance angle) without it.
But we don't have any sane way to save that kind of information (and we definitely do *not* want to do the "fstat()" thing on every flush). We could use the stdio buffering mode, but
- it's too weak (we want not line buffered or block buffered, we want basically "record buffered")
- I don't think there are any portable ways to read it (only set it, using "setbuf()" and friends).
Anyway, getting rid of stdio for writes we care about really *would* be a nice thing. But it's a lot of boring, nasty work.
So here's a patch that I think is acceptable. IT IS NOT PERFECT. Stdio simply cannot do a good job on errors, but it has a comment about the case where it just decides to ignore ferror() instead of doing what I suggest above.
I'm not saying this is great. But doing this right really does require avoiding stdio entirely.
Does this work for you?
(I pass the "desc" string thing in case there is some future use where we also want to use stdio, but we use it for something else than just regular stdout. Dunno).
Linus
--- builtin-rev-list.c | 2 +- cache.h | 2 ++ log-tree.c | 1 + write_or_die.c | 20 ++++++++++++++++++++ 4 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/builtin-rev-list.c b/builtin-rev-list.c index 813aadf..3980bf4 100644 --- a/builtin-rev-list.c +++ b/builtin-rev-list.c @@ -100,7 +100,7 @@ static void show_commit(struct commit *commit) printf("%s%c", buf, hdr_termination); free(buf); } - fflush(stdout); + flush_or_die(stdout, "stdout"); if (commit->parents) { free_commit_list(commit->parents); commit->parents = NULL; diff --git a/cache.h b/cache.h index ed83d92..aae9e2a 100644 --- a/cache.h +++ b/cache.h @@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME]; extern const char *git_commit_encoding; extern const char *git_log_output_encoding; +/* IO helper functions */ +extern void flush_or_die(FILE *, const char *); extern int copy_fd(int ifd, int ofd); extern int read_in_full(int fd, void *buf, size_t count); extern int write_in_full(int fd, const void *buf, size_t count); diff --git a/log-tree.c b/log-tree.c index 0cf21bc..061ecf7 100644 --- a/log-tree.c +++ b/log-tree.c @@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit) shown = 1; } opt->loginfo = NULL; + flush_or_die(stdout, "stdout"); return shown; } diff --git a/write_or_die.c b/write_or_die.c index 5c4bc85..84f411d 100644 --- a/write_or_die.c +++ b/write_or_die.c @@ -1,5 +1,25 @@ #include "cache.h" +/* + * Some cases use stdio, but want to flush after the write + * to get error handling (and to get better interactive + * behaviour - not buffering excessively). + * + * Of course, if the flush happened within the write itself, + * we've already lost the error code, and cannot report it any + * more. So we just ignore that case instead (and hope we get + * the right error code on the flush). + */ +void flush_or_die(FILE *f, const char *desc) +{ + if (fflush(f)) { + if (errno == EPIPE) + exit(0); + die("write failure on %s: %s", + desc, strerror(errno)); + } +} + int read_in_full(int fd, void *buf, size_t count) { char *p = buf;