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

Ilari Liusvaara [off-list ref] 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).

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

Ilari Liusvaara [off-list ref] 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.

 - 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