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