From: Junio C Hamano <gitster@pobox•com>
To: Ilari Liusvaara <ilari.liusvaara@elisanet•fi>
Cc: git@vger•kernel.org
Subject: Re: [RFC PATCH 1/2] Report exec errors from run-command
Date: Thu, 24 Dec 2009 23:35:04 -0800 [thread overview]
Message-ID: <7vr5qjecbb.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1261676971-3285-2-git-send-email-ilari.liusvaara@elisanet.fi> (Ilari Liusvaara's message of "Thu\, 24 Dec 2009 19\:49\:30 +0200")
Ilari Liusvaara <ilari.liusvaara@elisanet•fi> writes:
> Previously run-command was unable to report errors happening in exec
> call. Change it to pass errno from failed exec to errno value at
> return.
>
> The errno value passing can be done by opening close-on-exec pipe and
> piping the error code through in case of failure. In case of success,
> close-on-exec closes the pipe on successful exec and parent process
> gets end of file on read.
Thanks; I think overall this is a good idea, even though I have no clue
if WIN32 side wants a similar support.
- At first reading, the "while (close(fd) < 0 && errno != EBADF);"
pattern was a bit of eyesore. It might be worth factoring that out to
a small static helper function that a smart compiler would
automatically inline (or mark it as a static inline).
- Is it guaranteed that a failed pipe(2) never touches its int fildes[2]
parameter, or the values are undefined when it fails? The approach
would save one extra variable, compared to an alternative approach to
keep an explicit variable to record a pipe failure, but It feels iffy
that the code relies on them being left as their initial -1 values.
- Should we worry about partial write as well (you seem to warn when you
get a partial read)? Is it worth using xread() and friends found in
wrapper.c instead of goto read/write_again?
- Shouldn't any of these warning() be die() instead?
- The two extra file descriptors this patch uses are allocated after all
the existing pipe flipping is done, and all the dup's done in the child
process are to use dup2() to set up the known file descriptors at low
numbers, so I don't think we have to worry about this patch changing
the behaviour of the process pair by changing the assignment of file
descriptors (we had a bug or two in the past that made subprocess
misbehave under some funky condition, e.g. run with fd#0 closed).
next prev parent reply other threads:[~2009-12-25 7:37 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-12-24 17:49 [RFC PATCH 0/2] Report remote helper exec failures Ilari Liusvaara
2009-12-24 17:49 ` [RFC PATCH 1/2] Report exec errors from run-command Ilari Liusvaara
2009-12-25 7:35 ` Junio C Hamano [this message]
2009-12-25 7:46 ` Junio C Hamano
2009-12-25 8:40 ` Junio C Hamano
2009-12-25 9:51 ` Ilari Liusvaara
2009-12-25 14:39 ` Johannes Sixt
2009-12-25 17:15 ` Ilari Liusvaara
2009-12-24 17:49 ` [RFC PATCH 2/2] Improve transport helper exec failure reporting Ilari Liusvaara
2009-12-25 7:44 ` Junio C Hamano
2009-12-25 9:32 ` Ilari Liusvaara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vr5qjecbb.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=ilari.liusvaara@elisanet$(echo .)fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox