[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virCommand: Properly handle POLLHUP



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]