[libvirt] virCommandProcessIO: hangs on poll()

[Eric Blake]

On 04/19/2013 01:32 AM, wangxiaojun wrote:
> recent qemu (git resp) with param like:           
> #>qemu-kvm -S -no-user-config  -nodefaults -nographic  -M none -qmp
> monarg -pidfile pidfile
> 
> will just hangs forever.  no any output or errors .
> and when libvirt Try to get caps via QMP qemuCaps. it run above command.
> so virCommandProcessIO  call poll() will be hangs forever.
> 
> i patch the libvirt (git resp) with :
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 50712b0..915edaa 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2377,7 +2377,6 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>      virCommandClearCaps(cmd);
>      virCommandSetGID(cmd, runGid);
>      virCommandSetUID(cmd, runUid);
> -
>      if (virCommandRun(cmd, &status) < 0)

This hunk is unrelated.

>          goto cleanup;
>  
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index ac56a63..84738f9 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -1824,6 +1824,7 @@ virCommandProcessIO(virCommandPtr cmd)
>          int i;
>          struct pollfd fds[3];
>          int nfds = 0;
> +        int pfds = 0;
>  
>          if (cmd->inpipe != -1) {
>              fds[nfds].fd = cmd->inpipe;
> @@ -1846,8 +1847,8 @@ virCommandProcessIO(virCommandPtr cmd)
>  
>          if (nfds == 0)
>              break;
> -
> -        if (poll(fds, nfds, -1) < 0) {
> +        pfds = poll(fds, nfds, -1);
> +        if ( pfds< 0) {
>              if (errno == EAGAIN || errno == EINTR)
>                  continue;
>              virReportSystemError(errno, "%s",
> @@ -1855,10 +1856,22 @@ virCommandProcessIO(virCommandPtr cmd)
>              goto cleanup;
>          }
>  
> -        for (i = 0; i < nfds ; i++) {
> -            if (fds[i].revents & (POLLIN | POLLHUP | POLLERR) &&
> +        for (i = 0; i < pfds ; i++) {

We are guaranteed that pfds <= nfds; but it should not hurt to loop over
nfds instead of pfds.  Furthermore, there is no guarantee that when
'pfds < nfds' that the fds that were changed were the first pfds
consecutive entries, so looping over nfds actually feels better here.


> +            if (fds[i].revents & (POLLHUP | POLLRDHUP | POLLERR |
> POLLNVAL))

Hmm, you are adding POLLNVAL as a possible flag to look for.

> +            {
> +                if (fds[i].fd == errfd) {
> +                    VIR_DEBUG("hangup on stderr");
> +                } else if (fds[i].fd == outfd) {
> +                    VIR_DEBUG("hangup on stdout");
> +                } else {
> +                    VIR_DEBUG("hangup on stdin");
> +                }
> +                errfd = -1;
> +            }

Why are you blindly changing errfd, even if you checked that it was some
other fd that you gave a message about?

> +
> +            if (fds[i].revents & (POLLIN | POLLHUP | POLLRDHUP |
> POLLERR | POLLNVAL) &&
>                  (fds[i].fd == errfd || fds[i].fd == outfd)) {
> -                char data[1024];
> +                char data[1024*2];

Why do we need the larger buffer?

>                  char **buf;
>                  size_t *len;
>                  int done;
> 
> 
> 
> So everythings is OK.  
> i use gentoo OS.   qemu git source and libvirt git source.

You may be right that our use of poll() isn't entirely correct (it
wouldn't be the first time we've botched it, since that interface is
notoriously tricky), but if we fix it, I'd like to fix it right, and
audit our other uses of poll to make sure they are also right.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130422/35a88ff3/attachment-0001.sig>