[libvirt] [PATCH] virCommand: Properly handle POLLHUP
It is a good practise to set revents to zero before doing any poll(). Moreover, we should check if event we waited for really occurred or if any of fds we were polling on didn't encountered hangup. --- src/util/command.c | 21 +++++++++++++++++---- 1 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index f5effdf..9b553f0 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1620,16 +1620,19 @@ virCommandProcessIO(virCommandPtr cmd) if (infd != -1) { fds[nfds].fd = infd; fds[nfds].events = POLLOUT; + fds[nfds].revents = 0; nfds++; } if (outfd != -1) { fds[nfds].fd = outfd; fds[nfds].events = POLLIN; + fds[nfds].revents = 0; nfds++; } if (errfd != -1) { fds[nfds].fd = errfd; fds[nfds].events = POLLIN; + fds[nfds].revents = 0; nfds++; } @@ -1645,8 +1648,8 @@ virCommandProcessIO(virCommandPtr cmd) } for (i = 0; i < nfds ; i++) { - if (fds[i].fd == errfd || - fds[i].fd == outfd) { + if (fds[i].revents & POLLIN && + (fds[i].fd == errfd || fds[i].fd == outfd)) { char data[1024]; char **buf; size_t *len; @@ -1684,7 +1687,7 @@ virCommandProcessIO(virCommandPtr cmd) memcpy(*buf + *len, data, done); *len += done; } - } else { + } else if (fds[i].revents & POLLOUT) { int done; /* Coverity 5.3.0 can't see that we only get here if @@ -1708,8 +1711,18 @@ virCommandProcessIO(virCommandPtr cmd) VIR_DEBUG("ignoring failed close on fd %d", tmpfd); } } + } else if (fds[i].revents & POLLHUP) { + if (fds[i].fd == errfd) { + VIR_DEBUG("hangup on stderr"); + errfd = -1; + } else if (fds[i].fd == outfd) { + VIR_DEBUG("hangup on stdout"); + outfd = -1; + } else { + VIR_DEBUG("hangup on stdin"); + infd = -1; + } } - } } -- 1.7.3.4
On 01/03/2012 10:58 AM, Michal Privoznik wrote:
It is a good practise to set revents to zero before doing any poll(). Moreover, we should check if event we waited for really occurred or if any of fds we were polling on didn't encountered hangup.
Looks like this also solves: https://bugzilla.redhat.com/show_bug.cgi?id=770788
--- src/util/command.c | 21 +++++++++++++++++---- 1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index f5effdf..9b553f0 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1620,16 +1620,19 @@ virCommandProcessIO(virCommandPtr cmd) if (infd != -1) { fds[nfds].fd = infd; fds[nfds].events = POLLOUT; + fds[nfds].revents = 0;
Oh my - you're right - we stack-allocated struct pollfd fds[3]; without initialization earlier on.
@@ -1708,8 +1711,18 @@ virCommandProcessIO(virCommandPtr cmd) VIR_DEBUG("ignoring failed close on fd %d", tmpfd); } } + } else if (fds[i].revents & POLLHUP) { + if (fds[i].fd == errfd) { + VIR_DEBUG("hangup on stderr"); + errfd = -1; + } else if (fds[i].fd == outfd) { + VIR_DEBUG("hangup on stdout"); + outfd = -1; + } else { + VIR_DEBUG("hangup on stdin"); + infd = -1; + }
Should we also be probing for POLLERR? (POLLNVAL shouldn't ever be present, because it would indicate a bigger bug in our code). ACK - what you have is a strict improvement, even if we further decide to check for POLLERR. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03.01.2012 20:00, Eric Blake wrote:
On 01/03/2012 10:58 AM, Michal Privoznik wrote:
It is a good practise to set revents to zero before doing any poll(). Moreover, we should check if event we waited for really occurred or if any of fds we were polling on didn't encountered hangup.
Looks like this also solves:
https://bugzilla.redhat.com/show_bug.cgi?id=770788
--- src/util/command.c | 21 +++++++++++++++++---- 1 files changed, 17 insertions(+), 4 deletions(-)
Should we also be probing for POLLERR? (POLLNVAL shouldn't ever be present, because it would indicate a bigger bug in our code).
ACK - what you have is a strict improvement, even if we further decide to check for POLLERR.
Thanks, pushed with this squashed in: diff --git a/src/util/command.c b/src/util/command.c index 9b553f0..bdaa88b 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1687,7 +1687,10 @@ virCommandProcessIO(virCommandPtr cmd) memcpy(*buf + *len, data, done); *len += done; } - } else if (fds[i].revents & POLLOUT) { + } + + if (fds[i].revents & POLLOUT && + fds[i].fd == infd) { int done; /* Coverity 5.3.0 can't see that we only get here if @@ -1711,7 +1714,9 @@ virCommandProcessIO(virCommandPtr cmd) VIR_DEBUG("ignoring failed close on fd %d", tmpfd); } } - } else if (fds[i].revents & POLLHUP) { + } + + if (fds[i].revents & (POLLHUP | POLLERR)) { if (fds[i].fd == errfd) { VIR_DEBUG("hangup on stderr"); errfd = -1;
On Tue, Jan 03, 2012 at 06:58:51PM +0100, Michal Privoznik wrote:
It is a good practise to set revents to zero before doing any poll(). Moreover, we should check if event we waited for really occurred or if any of fds we were polling on didn't encountered hangup. --- src/util/command.c | 21 +++++++++++++++++---- 1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index f5effdf..9b553f0 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1620,16 +1620,19 @@ virCommandProcessIO(virCommandPtr cmd) if (infd != -1) { fds[nfds].fd = infd; fds[nfds].events = POLLOUT; + fds[nfds].revents = 0; nfds++; } if (outfd != -1) { fds[nfds].fd = outfd; fds[nfds].events = POLLIN; + fds[nfds].revents = 0; nfds++; } if (errfd != -1) { fds[nfds].fd = errfd; fds[nfds].events = POLLIN; + fds[nfds].revents = 0; nfds++; }
ACK to this bit, clearly addressing a bug
@@ -1645,8 +1648,8 @@ virCommandProcessIO(virCommandPtr cmd) }
for (i = 0; i < nfds ; i++) { - if (fds[i].fd == errfd || - fds[i].fd == outfd) { + if (fds[i].revents & POLLIN && + (fds[i].fd == errfd || fds[i].fd == outfd)) { char data[1024]; char **buf; size_t *len; @@ -1684,7 +1687,7 @@ virCommandProcessIO(virCommandPtr cmd) memcpy(*buf + *len, data, done); *len += done; } - } else { + } else if (fds[i].revents & POLLOUT) { int done;
/* Coverity 5.3.0 can't see that we only get here if @@ -1708,8 +1711,18 @@ virCommandProcessIO(virCommandPtr cmd) VIR_DEBUG("ignoring failed close on fd %d", tmpfd); } } + } else if (fds[i].revents & POLLHUP) { + if (fds[i].fd == errfd) { + VIR_DEBUG("hangup on stderr"); + errfd = -1; + } else if (fds[i].fd == outfd) { + VIR_DEBUG("hangup on stdout"); + outfd = -1; + } else { + VIR_DEBUG("hangup on stdin"); + infd = -1; + } }
Was this part of the change actually solving anything ? When you have POLLIN set in the events, you'll never get a bare POLLHUP event back in revents. You'll always get revents set to POLLIN|POLLHUP. Thus since an earlier if() checks for POLLIN, I'm fairly sure this POLLHUP block you're adding won't ever run. If we want to explicitly check events for comprehensively, then I think we'd want to avoid the if/elseif/elseif style, and use separate if/if/if tests for each POLLXXX bit. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03.01.2012 20:09, Daniel P. Berrange wrote:
On Tue, Jan 03, 2012 at 06:58:51PM +0100, Michal Privoznik wrote:
It is a good practise to set revents to zero before doing any poll(). Moreover, we should check if event we waited for really occurred or if any of fds we were polling on didn't encountered hangup. --- src/util/command.c | 21 +++++++++++++++++---- 1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index f5effdf..9b553f0 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1620,16 +1620,19 @@ virCommandProcessIO(virCommandPtr cmd) if (infd != -1) { fds[nfds].fd = infd; fds[nfds].events = POLLOUT; + fds[nfds].revents = 0; nfds++; } if (outfd != -1) { fds[nfds].fd = outfd; fds[nfds].events = POLLIN; + fds[nfds].revents = 0; nfds++; } if (errfd != -1) { fds[nfds].fd = errfd; fds[nfds].events = POLLIN; + fds[nfds].revents = 0; nfds++; }
ACK to this bit, clearly addressing a bug
@@ -1645,8 +1648,8 @@ virCommandProcessIO(virCommandPtr cmd) }
for (i = 0; i < nfds ; i++) { - if (fds[i].fd == errfd || - fds[i].fd == outfd) { + if (fds[i].revents & POLLIN && + (fds[i].fd == errfd || fds[i].fd == outfd)) { char data[1024]; char **buf; size_t *len; @@ -1684,7 +1687,7 @@ virCommandProcessIO(virCommandPtr cmd) memcpy(*buf + *len, data, done); *len += done; } - } else { + } else if (fds[i].revents & POLLOUT) { int done;
/* Coverity 5.3.0 can't see that we only get here if @@ -1708,8 +1711,18 @@ virCommandProcessIO(virCommandPtr cmd) VIR_DEBUG("ignoring failed close on fd %d", tmpfd); } } + } else if (fds[i].revents & POLLHUP) { + if (fds[i].fd == errfd) { + VIR_DEBUG("hangup on stderr"); + errfd = -1; + } else if (fds[i].fd == outfd) { + VIR_DEBUG("hangup on stdout"); + outfd = -1; + } else { + VIR_DEBUG("hangup on stdin"); + infd = -1; + } }
Was this part of the change actually solving anything ? When you have POLLIN set in the events, you'll never get a bare POLLHUP event back in revents. You'll always get revents set to POLLIN|POLLHUP. Thus since an earlier if() checks for POLLIN, I'm fairly sure this POLLHUP block you're adding won't ever run. Yes it was. Simply reproducible test: remove that else branch and try running commandtest; You'll get this sequence:
fd: 4 ev: 1 rev: 0 fd: 6 ev: 1 rev: 1 fd: 4 ev: 1 rev: 16 fd: 6 ev: 1 rev: 16 diff --git a/src/util/command.c b/src/util/command.c index 1b62319..22b9d54 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1648,6 +1648,7 @@ virCommandProcessIO(virCommandPtr cmd) } for (i = 0; i < nfds ; i++) { + printf("fd: %d ev: %d rev: %d\n", fds[i].fd, fds[i].events, fds[i].revents); if (fds[i].revents & POLLIN && (fds[i].fd == errfd || fds[i].fd == outfd)) { char data[1024]; Although, I agree that POLLHUP semantics is not good documented, to say the least.
If we want to explicitly check events for comprehensively, then I think we'd want to avoid the if/elseif/elseif style, and use separate if/if/if tests for each POLLXXX bit.
Okay, I'll go this way.
Regards, Daniel
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik