From c22f6202052ca84c68df4fbb16e42c826d429558 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Mon, 27 Jun 2016 03:56:35 +0000 Subject: [PATCH 1/3] xread: retry after poll on EAGAIN/EWOULDBLOCK We should continue to loop after EAGAIN/EWOULDBLOCK as the intent of xread is to try until there is available data, EOF, or an unrecoverable error. Fixes: 1079c4be0b720 ("xread: poll on non blocking fds") Signed-off-by: Eric Wong Signed-off-by: Junio C Hamano --- wrapper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/wrapper.c b/wrapper.c index 1770efac8e..9b20eb9351 100644 --- a/wrapper.c +++ b/wrapper.c @@ -252,6 +252,7 @@ ssize_t xread(int fd, void *buf, size_t len) * call to read(2). */ poll(&pfd, 1, -1); + continue; } } return nr; From ef1cf0167a362cd504afdcc0eef4c3200ea6dfbb Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 26 Jun 2016 23:21:12 +0000 Subject: [PATCH 2/3] xwrite: poll on non-blocking FDs write(2) can hit the same EAGAIN/EWOULDBLOCK errors as read(2), so busy-looping on a non-blocking FD is a waste of resources. Currently, I do not know of a way for this happen: * the NonBlocking directive in systemd does not apply to stdin, stdout, or stderr. * xinetd provides no way to set the non-blocking flag at all But theoretically, it's possible a careless C10K HTTP server could use pipe2(..., O_NONBLOCK) to setup a pipe for git-http-backend with only the intent to use non-blocking reads; but accidentally leave non-blocking set on the write end passed as stdout to git-upload-pack. Followup-to: 1079c4be0b720 ("xread: poll on non blocking fds") Signed-off-by: Eric Wong Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- wrapper.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/wrapper.c b/wrapper.c index 9b20eb9351..f7ea6c43be 100644 --- a/wrapper.c +++ b/wrapper.c @@ -271,8 +271,26 @@ ssize_t xwrite(int fd, const void *buf, size_t len) len = MAX_IO_SIZE; while (1) { nr = write(fd, buf, len); - if ((nr < 0) && (errno == EAGAIN || errno == EINTR)) - continue; + if (nr < 0) { + if (errno == EINTR) + continue; + if (errno == EAGAIN || errno == EWOULDBLOCK) { + struct pollfd pfd; + pfd.events = POLLOUT; + pfd.fd = fd; + /* + * it is OK if this poll() failed; we + * want to leave this infinite loop + * only when write() returns with + * success, or an expected failure, + * which would be checked by the next + * call to write(2). + */ + poll(&pfd, 1, -1); + continue; + } + } + return nr; } } From d751dd11ae16fd4e2410d3a32c8e2d951fafc923 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 10 Jul 2016 08:20:46 +0000 Subject: [PATCH 3/3] hoist out handle_nonblock function for xread and xwrite At least for me, this improves the readability of xread and xwrite; hopefully allowing missing "continue" statements to be spotted more easily. Signed-off-by: Eric Wong Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- wrapper.c | 48 ++++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/wrapper.c b/wrapper.c index f7ea6c43be..a774ab7d80 100644 --- a/wrapper.c +++ b/wrapper.c @@ -224,6 +224,24 @@ int xopen(const char *path, int oflag, ...) } } +static int handle_nonblock(int fd, short poll_events, int err) +{ + struct pollfd pfd; + + if (err != EAGAIN && err != EWOULDBLOCK) + return 0; + + pfd.fd = fd; + pfd.events = poll_events; + + /* + * no need to check for errors, here; + * a subsequent read/write will detect unrecoverable errors + */ + poll(&pfd, 1, -1); + return 1; +} + /* * xread() is the same a read(), but it automatically restarts read() * operations with a recoverable error (EAGAIN and EINTR). xread() @@ -239,21 +257,8 @@ ssize_t xread(int fd, void *buf, size_t len) if (nr < 0) { if (errno == EINTR) continue; - if (errno == EAGAIN || errno == EWOULDBLOCK) { - struct pollfd pfd; - pfd.events = POLLIN; - pfd.fd = fd; - /* - * it is OK if this poll() failed; we - * want to leave this infinite loop - * only when read() returns with - * success, or an expected failure, - * which would be checked by the next - * call to read(2). - */ - poll(&pfd, 1, -1); + if (handle_nonblock(fd, POLLIN, errno)) continue; - } } return nr; } @@ -274,21 +279,8 @@ ssize_t xwrite(int fd, const void *buf, size_t len) if (nr < 0) { if (errno == EINTR) continue; - if (errno == EAGAIN || errno == EWOULDBLOCK) { - struct pollfd pfd; - pfd.events = POLLOUT; - pfd.fd = fd; - /* - * it is OK if this poll() failed; we - * want to leave this infinite loop - * only when write() returns with - * success, or an expected failure, - * which would be checked by the next - * call to write(2). - */ - poll(&pfd, 1, -1); + if (handle_nonblock(fd, POLLOUT, errno)) continue; - } } return nr;