Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK
Stefan Beller <sbeller@google.com> wrote:
Show 7 quoted lines
> 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(-)
Show changes to wrapper.c +16 −24
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
