[libvirt] [PATCH v2 1/7] virCommand: Introduce virCommandDoAsyncIO
Peter Krempa
pkrempa at redhat.com
Tue Jan 29 13:13:03 UTC 2013
On 01/28/13 17:39, Michal Privoznik wrote:
> Currently, if we want to feed stdin, or catch stdout or stderr of a
> virCommand we have to use virCommandRun(). When using virCommandRunAsync()
> we have to register FD handles by hand. This may lead to code duplication.
> Hence, introduce an internal API, which does this automatically within
> virCommandRunAsync(). The intended usage looks like this:
>
> virCommandPtr cmd = virCommandNew*(...);
> char *buf = NULL;
>
> ...
>
> virCommandSetOutputBuffer(cmd, &buf);
The docs for this func state:
* Capture the child's stdout to a string buffer. *outbuf is
* guaranteed to be allocated after successful virCommandRun or
* virCommandWait, and is best-effort allocated after failed
* virCommandRun; caller is responsible for freeing *outbuf.
* This requires the use of virCommandRun.
The last sentence isn't true after this patch. Same applies for the
error buffer setting func.
> virCommandDoAsyncIO(cmd);
>
> if (virCommandRunAsync(cmd, NULL) < 0)
> goto cleanup;
>
> ...
>
> if (virCommandWait(cmd, NULL) < 0)
> goto cleanup;
>
> /* @buf now contains @cmd's stdout */
> VIR_DEBUG("STDOUT: %s", NULLSTR(buf));
>
> ...
>
> cleanup:
> VIR_FREE(buf);
> virCommandFree(cmd);
>
> Note, that both stdout and stderr buffers may change until virCommandWait()
> returns.
> ---
> src/libvirt_private.syms | 1 +
> src/util/vircommand.c | 279 +++++++++++++++++++++++++++++++++++++++++++++--
> src/util/vircommand.h | 1 +
> 3 files changed, 273 insertions(+), 8 deletions(-)
>
[...]
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 8566d1a..117fc07 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
[...]
> @@ -2122,6 +2129,173 @@ virCommandHook(void *data)
> }
>
>
> +static void
> +virCommandHandleReadWrite(int watch, int fd, int events, void *opaque)
> +{
> + virCommandPtr cmd = (virCommandPtr) opaque;
> + char ***bufptr = NULL;
> + char buf[1024];
> + ssize_t nread;
> + size_t len = 0;
> + int *watchPtr = NULL;
> + bool eof = false;
> + int tmpfd, *fdptr = NULL, **fdptrptr = NULL;
> +
> + VIR_DEBUG("watch=%d fd=%d events=%d", watch, fd, events);
> + errno = 0;
> +
> + if (watch == cmd->inWatch) {
> + watchPtr = &cmd->inWatch;
> + fdptr = &cmd->infd;
> +
> + if (events & VIR_EVENT_HANDLE_WRITABLE) {
> + len = strlen(cmd->inbuf);
> +
> + while (true) {
> + nread = write(fd, cmd->inbuf + cmd->inbufOffset,
> + len - cmd->inbufOffset);
nread is a little awkward when used with write()
> + if (nread < 0) {
> + if (errno != EAGAIN && errno != EINTR) {
> + virReportSystemError(errno,
> + _("Unable to write command's "
> + "input to FD %d"),
> + fd);
> + eof = true;
> + }
> + break;
> + }
> +
> + if (nread == 0) {
> + eof = true;
> + break;
> + }
> +
> + cmd->inbufOffset += nread;
> + if (cmd->inbufOffset == len) {
> + tmpfd = cmd->infd;
> + if (VIR_CLOSE(cmd->infd) < 0)
> + VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
> + eof = true;
> + break;
> + }
> + }
> +
> + }
> + } else {
> + if (watch == cmd->outWatch) {
> + watchPtr = &cmd->outWatch;
> + bufptr = &cmd->outbuf;
> + fdptr = &cmd->outfd;
> + fdptrptr = &cmd->outfdptr;
> + } else {
> + watchPtr = &cmd->errWatch;
> + bufptr = &cmd->errbuf;
> + fdptr = &cmd->outfd;
> + fdptrptr = &cmd->outfdptr;
> + }
> +
> + if (bufptr && *bufptr && events & VIR_EVENT_HANDLE_READABLE) {
bufptr isn't NULL at this place.
even *bufptr shouldn't be NULL as the watch handle should be
unregistered on error but I'm not 100% sure about this.
> + if (**bufptr)
> + len = strlen(**bufptr);
> +
> + while (true) {
> + nread = read(fd, buf, sizeof(buf));
> + if (nread < 0) {
> + if (errno != EAGAIN && errno != EINTR) {
> + virReportSystemError(errno,
> + _("unable to read command's "
> + "output from FD %d"),
> + fd);
> + eof = true;
> + }
> + break;
> + }
> +
> + if (nread == 0) {
> + eof = true;
> + break;
> + }
> +
> + if (VIR_REALLOC_N(**bufptr, len + nread + 1) < 0) {
> + virReportOOMError();
> + break;
> + }
> +
> + memcpy(**bufptr + len, buf, nread);
> + (**bufptr)[len + nread] = '\0';
It's a bit awkward here as you can't guarantee you didn't read binary
data, so memcpy is better here compared to strcpy.
> + }
> +
> + }
> + }
> +
> + if (eof || (events & VIR_EVENT_HANDLE_HANGUP) ||
> + (events & VIR_EVENT_HANDLE_ERROR)) {
[1] ...
> + *watchPtr = -1;
> + /* Reset any capturing, in case caller runs
> + * this identical command again */
I don't think you are able to re-run the same command twice as [2] ...
> + tmpfd = *fdptr;
> + if (VIR_CLOSE(*fdptr) < 0)
> + VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
This is already logged by VIR_CLOSE.
> + if (bufptr)
> + *bufptr = NULL;
[2] ... here you clear the buffer address from the cmd structure. In
case someone re-runs it the pointers to the user's buffer won't be
available and the error reporting won't work.
> + if (fdptrptr)
> + *fdptrptr = NULL;
> + virEventRemoveHandle(watch);
This line should be moved before [1] to match other parts of this patch.
> + }
> +}
> +
> +
> +static int
> +virCommandRegisterEventLoop(virCommandPtr cmd)
> +{
> + int ret = -1;
> +
> + if (cmd->inbuf &&
> + (cmd->inWatch = virEventAddHandle(cmd->infd,
> + VIR_EVENT_HANDLE_WRITABLE |
> + VIR_EVENT_HANDLE_HANGUP |
> + VIR_EVENT_HANDLE_ERROR,
> + virCommandHandleReadWrite,
> + cmd, NULL)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to register infd %d in the event loop"),
> + cmd->infd);
> + goto cleanup;
> + }
> +
> + if (cmd->outbuf && cmd->outfdptr == &cmd->outfd &&
> + (cmd->outWatch = virEventAddHandle(cmd->outfd,
> + VIR_EVENT_HANDLE_READABLE |
> + VIR_EVENT_HANDLE_HANGUP |
> + VIR_EVENT_HANDLE_ERROR,
> + virCommandHandleReadWrite,
> + cmd, NULL)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to register outfd %d in the event loop"),
> + cmd->outfd);
> + goto cleanup;
> + }
> +
> + if (cmd->errbuf && cmd->errfdptr == &cmd->errfd &&
> + (cmd->errWatch = virEventAddHandle(cmd->errfd,
> + VIR_EVENT_HANDLE_READABLE |
> + VIR_EVENT_HANDLE_HANGUP |
> + VIR_EVENT_HANDLE_ERROR,
> + virCommandHandleReadWrite,
> + cmd, NULL)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to register errfd %d in the event loop"),
> + cmd->errfd);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + return ret;
> +}
I think you should unregister the handles registered if one of the
others failed. Otherwise they might be left registered.
An option would be to add the unregistration into the freeing function
of cmd.
> +
> +
> /**
> * virCommandRunAsync:
> * @cmd: command to start
[...]
> @@ -2272,6 +2471,24 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
> * guarantee that virProcessWait only fails due to failure to wait,
> * and repeat the exitstatus check code ourselves. */
> ret = virProcessWait(cmd->pid, exitstatus ? exitstatus : &status);
> +
> + if (cmd->inWatch != -1) {
> + virEventRemoveHandle(cmd->inWatch);
> + cmd->inWatch = -1;
> + }
> +
> + if (cmd->outWatch != -1) {
> + virEventRemoveHandle(cmd->outWatch);
> + virCommandHandleReadWrite(cmd->outWatch, cmd->outfd, events, cmd);
> + cmd->outWatch = -1;
> + }
> +
> + if (cmd->errWatch != -1) {
> + virEventRemoveHandle(cmd->errWatch);
> + virCommandHandleReadWrite(cmd->errWatch, cmd->errfd, events, cmd);
> + cmd->errWatch = -1;
> + }
Okay, this approach is better than the previous version.
> +
> if (ret == 0) {
> cmd->pid = -1;
> cmd->reap = false;
> @@ -2521,3 +2738,49 @@ virCommandFree(virCommandPtr cmd)
>
> VIR_FREE(cmd);
> }
Peter
More information about the libvir-list
mailing list