Re: [RFC PATCH 1/2] Report exec errors from run-command

From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Date: Dec 25, 2009, 09:51 UTC
Message-ID: <20091225095147.GB8319@Knoppix>
In-Reply-To: <7vr5qjecbb.fsf@alter.siamese.dyndns.org>

On Thu, Dec 24, 2009 at 11:35:04PM -0800, Junio C Hamano wrote:

> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> Thanks; I think overall this is a good idea, even though I have no clue
> if WIN32 side wants a similar support.

It would. But I have no clue about WIN32. But there are other people
on this list who have. :-)

>  - 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).

Done (with bit of reformatting to add space, line break and comment.

Show 5 quoted lines

>  - 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.

I added explicit set to -1 on failure case (I think failed pipe doesn't touch those, but you never know about what some oddball OS is going to do).

>  - 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?

That's hairy code. One really can't print any errors in write path, as those errors would go to who knows where due to redirections (I took the error warning out).

That partial read warning is more for detecting 'can't happen' situations since pipe should be large enough to atomically transfer integer.

>  - Shouldn't any of these warning() be die() instead?

If error reporting failures are fatal, all of them.

-Ilari