Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK

[Eric Wong <e@80x24.org>]

Stefan Beller <sbeller@google.com> wrote:
> On Mon, Jun 27, 2016 at 7:36 AM, Jeff King <peff@peff.net> wrote:
> > It's also true that our error rate will never be 0%. So some bugs will
> > always slip through, some review comments will be forgotten, etc. Eric
> > did find and fix the bug just now, so the "many eyes" theory did work
> > here eventually.
> 
> Eric, thanks for catching and fixing the bug!

No problem :)  I only noticed it because I was scanning emails
randomly and Duy and David's index-helper thread turned up.

> Quite a while ago, when I started doing code reviews professionally, I wondered
> if the code review procedure can be semi-automated, as automation helps keeping
> the error rate low. By that I mean having a check list which I can
> check off each point

Maybe a test case or even a small unit test would've helped.
I didn't notice the problem in xread until:

1) I copied the code into xwrite
2) s/POLLIN/POLLOUT/;
3) forced EAGAIN using a patched, home-baked HTTP server

The biggish comment before the poll() obscured the missing
"continue" for me.  I read xread() before and did not notice
the missing "continue".

Maybe the following optional patch on top of this series
improves readability:

----------8<--------
Subject: [PATCH 3/2] hoist out io_wait 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 <e@80x24.org>
---
 wrapper.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index d973f86..04bb952 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -227,6 +227,20 @@ int xopen(const char *path, int oflag, ...)
 	}
 }
 
+static void io_wait(int fd, short poll_events)
+{
+	struct pollfd pfd;
+
+	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);
+}
+
 /*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
@@ -243,18 +257,7 @@ ssize_t xread(int fd, void *buf, size_t len)
 			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);
+				io_wait(fd, POLLIN);
 				continue;
 			}
 		}
@@ -278,18 +281,7 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 			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);
+				io_wait(fd, POLLOUT);
 				continue;
 			}
 		}
-- 
EW