| Message ID | YvyGJiz+CFPcgpML@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c6d3cce6f3c4d1a8d9ebc556c38f1335afdfeb6c |
| Headers | show |
| Series | fix pipe_command() deadlock | expand |
Jeff King <peff@peff.net> writes: > When write() to a non-blocking pipe fails because the buffer is full, > POSIX says we should see EAGAIN. But our mingw_write() compat layer on > Windows actually returns ENOSPC for this case. This is probably > something we want to correct, but given that we don't plan to use > non-blocking descriptors in a lot of places, we can work around it by > just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(), > then this patch can be reverted. > > We don't actually use a non-blocking pipe yet, so this is still just > preparation. > > Helped-by: René Scharfe <l.s.r@web.de> > Signed-off-by: Jeff King <peff@peff.net> > --- > Ironically, this ENOSPC bug means that switching away from xwrite() in > the previous patch wasn't necessary (because it's not clever enough to > know that ENOSPC on a pipe means EAGAIN!). But I think handling both > shows the intent, and sets us up better for fixing mingw_write(). Yeah, I am impressed by the attention of small details by you two shown here to split the steps 4/6 and 5/6. If we consider that this step is a band-aid we'd be happier if we can remove, perhaps in-code comment to explain why we deal with ENOSPC here, instead of burying it only in the log message, would help remind people of the issue (but of course the patch is good with or without such a tweak, which is only relevant in the longer term). > run-command.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/run-command.c b/run-command.c > index e078c3046f..5fbaa8b5ac 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1377,7 +1377,8 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd) > io->u.out.len <= MAX_IO_SIZE ? > io->u.out.len : MAX_IO_SIZE); > if (len < 0) { > - if (errno != EINTR && errno != EAGAIN) { > + if (errno != EINTR && errno != EAGAIN && > + errno != ENOSPC) { > io->error = errno; > close(io->fd); > io->fd = -1;
On Wed, Aug 17, 2022 at 11:57:01AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > When write() to a non-blocking pipe fails because the buffer is full, > > POSIX says we should see EAGAIN. But our mingw_write() compat layer on > > Windows actually returns ENOSPC for this case. This is probably > > something we want to correct, but given that we don't plan to use > > non-blocking descriptors in a lot of places, we can work around it by > > just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(), > > then this patch can be reverted. > > > > We don't actually use a non-blocking pipe yet, so this is still just > > preparation. > > > > Helped-by: René Scharfe <l.s.r@web.de> > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > Ironically, this ENOSPC bug means that switching away from xwrite() in > > the previous patch wasn't necessary (because it's not clever enough to > > know that ENOSPC on a pipe means EAGAIN!). But I think handling both > > shows the intent, and sets us up better for fixing mingw_write(). > > Yeah, I am impressed by the attention of small details by you two > shown here to split the steps 4/6 and 5/6. If we consider that this > step is a band-aid we'd be happier if we can remove, perhaps in-code > comment to explain why we deal with ENOSPC here, instead of burying > it only in the log message, would help remind people of the issue > (but of course the patch is good with or without such a tweak, which > is only relevant in the longer term). Yeah, you may be right. I had originally written a quite long comment here (before I split things up so much), but found in the splitting that it made more sense to put most of the details into the commit message. But maybe it's worth squashing this in? diff --git a/run-command.c b/run-command.c index 5fbaa8b5ac..065883672b 100644 --- a/run-command.c +++ b/run-command.c @@ -1372,6 +1372,10 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd) * * Note that we lose xwrite()'s handling of MAX_IO_SIZE * and EINTR, so we have to implement those ourselves. + * + * We also check for ENOSPC to handle a quirk of + * mingw_write(), which uses that for a full pipe + * instead of EAGAIN. */ len = write(io->fd, io->u.out.buf, io->u.out.len <= MAX_IO_SIZE ? -Peff
diff --git a/run-command.c b/run-command.c index e078c3046f..5fbaa8b5ac 100644 --- a/run-command.c +++ b/run-command.c @@ -1377,7 +1377,8 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd) io->u.out.len <= MAX_IO_SIZE ? io->u.out.len : MAX_IO_SIZE); if (len < 0) { - if (errno != EINTR && errno != EAGAIN) { + if (errno != EINTR && errno != EAGAIN && + errno != ENOSPC) { io->error = errno; close(io->fd); io->fd = -1;
When write() to a non-blocking pipe fails because the buffer is full, POSIX says we should see EAGAIN. But our mingw_write() compat layer on Windows actually returns ENOSPC for this case. This is probably something we want to correct, but given that we don't plan to use non-blocking descriptors in a lot of places, we can work around it by just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(), then this patch can be reverted. We don't actually use a non-blocking pipe yet, so this is still just preparation. Helped-by: René Scharfe <l.s.r@web.de> Signed-off-by: Jeff King <peff@peff.net> --- Ironically, this ENOSPC bug means that switching away from xwrite() in the previous patch wasn't necessary (because it's not clever enough to know that ENOSPC on a pipe means EAGAIN!). But I think handling both shows the intent, and sets us up better for fixing mingw_write(). run-command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)