[libvirt] [PATCH 3/3]: Read cmd stdout + stderr in virRun

[Daniel P. Berrange]

On Thu, Oct 30, 2008 at 02:06:35PM -0400, Cole Robinson wrote:
> The attached patch is my second cut at reading 
> stdout and stderr of the command virRun kicks
> off. There is no hard limit to the amount of
> data we read now, and we use a poll loop to
> avoid any possible full buffer issues.
> 
> If stdout or stderr had any content, we DEBUG
> it, and if the command appears to fail we
> return stderr in the error message. So now,
> trying to stop a logical pool with active
> volumes will return:
> 
> $ sudo virsh pool-destroy vgdata
> libvir: error : internal error '/sbin/vgchange -an vgdata' exited with non-zero status 5 and signal 0:   Can't deactivate volume group "vgdata" with 2 open logical volume(s)
> error: Failed to destroy pool vgdata


> +    fds[0].fd = outfd;
> +    fds[0].events = POLLIN;
> +    finished[0] = 0;
> +    fds[1].fd = errfd;
> +    fds[1].events = POLLIN;
> +    finished[1] = 0;
> +
> +    while(!(finished[0] && finished[1])) {
> +
> +        if (poll(fds, ARRAY_CARDINALITY(fds), -1) < 0) {
> +            if (errno == EAGAIN)
> +                continue;
> +            goto pollerr;
> +        }
> +
> +        for (i = 0; i < ARRAY_CARDINALITY(fds); ++i) {
> +            char data[1024], **buf;
> +            int got, size;
> +
> +            if (!(fds[i].revents))
> +                continue;
> +            else if (fds[i].revents & POLLHUP)
> +                finished[i] = 1;
> +
> +            if (!(fds[i].revents & POLLIN)) {
> +                if (fds[i].revents & POLLHUP)
> +                    continue;
> +
> +                ReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                            "%s", _("Unknown poll response."));
> +                goto error;
> +            }
> +
> +            got = read(fds[i].fd, data, sizeof(data));
> +
> +            if (got == 0) {
> +                finished[i] = 1;
> +                continue;
> +            }
> +            if (got < 0) {
> +                if (errno == EINTR)
> +                    continue;
> +                if (errno == EAGAIN)
> +                    break;
> +                goto pollerr;
> +            }
>  
> -    while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR);
> -    if (ret == -1) {
> +            buf = ((fds[i].fd == outfd) ? &outbuf : &errbuf);
> +            size = (*buf ? strlen(*buf) : 0);
> +            if (VIR_REALLOC_N(*buf, size+got+1) < 0) {
> +                ReportError(conn, VIR_ERR_NO_MEMORY, "%s", "realloc buf");
> +                goto error;
> +            }
> +            memmove(*buf+size, data, got);
> +            (*buf)[size+got] = '\0';
> +        }
> +        continue;
> +
> +    pollerr:
> +        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                    _("poll error: %s"), strerror(errno));
> +        goto error;
> +    }


I think it'd be nice to move the I/O processing loop out  of the
virRun() function and into a separate helper functiion along the
lines of 

   virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf)

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|