[PATCH] diff: fix textconv error zombies
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Mar 30, 2010, 17:36 UTC
- Message-ID
- <201003301936.03394.j6t@kdbg.org>
- In-Reply-To
- <20100330163004.GC17763@coredump.intra.peff.net>
To make the code simpler, run_textconv lumps all of its error checking into one conditional. However, the short-circuit means that an error in reading will prevent us from calling finish_command, leaving a zombie child. Clean up properly after errors.
Based-on-work-by: Jeff King <peff@peff.net> Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- Jeff,
> Yes, there are clever ways to make this shorter, but I think it is > clearer just written out.
Thanks for your work, but I'm worried that in your version the close() call is not before the finish_command (but that's really not _that_ important in this case). My version is perhaps not as easy to read, but has a slightly better diff-stat.
Oh, I removed the error messages after start_command and finish_command, because these two print one themselves; Junio, if you disagree with this change, ditch my version and take Jeff's.
-- Hannes
diff.c | 17 +++++++++++------ 1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c index dfdfa1a..b96faea 100644 --- a/diff.c +++ b/diff.c @@ -3874,6 +3874,7 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec, const char **arg = argv; struct child_process child; struct strbuf buf = STRBUF_INIT; + int err = 0; temp = prepare_temp_file(spec->path, spec); *arg++ = pgm; @@ -3884,16 +3885,20 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec, child.use_shell = 1; child.argv = argv; child.out = -1; - if (start_command(&child) != 0 || - strbuf_read(&buf, child.out, 0) < 0 || - finish_command(&child) != 0) { - close(child.out); - strbuf_release(&buf); + if (start_command(&child)) { remove_tempfile(); - error("error running textconv command '%s'", pgm); return NULL; } + + if (strbuf_read(&buf, child.out, 0) < 0) + err = error("error reading from textconv command '%s'", pgm); close(child.out); + + if (finish_command(&child) || err) { + strbuf_release(&buf); + remove_tempfile(); + return NULL; + } remove_tempfile(); return strbuf_detach(&buf, outsize);
-- 1.7.0.2.250.ge5578