Re: [Qemu-devel] [PATCH 5/5] qga: guest-exec simple stdin/stdout/stderr redirection

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Denis V. Lunev (2015-10-07 05:32:21)
> From: Yuri Pudgorodskiy <yur@virtuozzo.com>
> 
> Implemented with base64-encoded strings in qga json protocol.
> Glib portable GIOChannel is used for data I/O.
> 
> Optinal stdin parameter of guest-exec command is now used as
> stdin content for spawned subprocess.
> 
> If capture-output bool flag is specified, guest-exec redirects out/err
> file descriptiors internally to pipes and collects subprocess
> output.
> 
> Guest-exe-status is modified to return this collected data to requestor
> in base64 encoding.
> 
> Signed-off-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>

For capture-output mode, win32 guests appear to spin forever on EOF and
the exec'd process never gets reaped as a result. The below patch
seems to fix this issue. If this seems reasonable I can squash it
into the patch directly, but you might want to check I didn't break one
of your !capture-output use-cases (I assume this is still mainly focused
on redirecting to virtio-serial for stdio).

Another issue I noticed is that glib relies on
gspawn-win{32,64}-helper[-console].exe for g_spawn*() interface, so guest
exec won't work for .msi distributables unless those are added. This can
probably be addressed during soft-freeze though.

diff --git a/qga/commands.c b/qga/commands.c
index 27c06c5..fbf8abd 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -318,7 +318,7 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
     struct GuestExecIOData *p = (struct GuestExecIOData *)p_;
     gsize bytes_read = 0;
 
-    if (cond == G_IO_HUP) {
+    if (cond == G_IO_HUP || cond == G_IO_ERR) {
         g_io_channel_unref(ch);
         g_atomic_int_set(&p->closed, 1);
         return FALSE;
@@ -330,10 +330,18 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
             t = g_try_realloc(p->data, p->size + GUEST_EXEC_IO_SIZE);
         }
         if (t == NULL) {
+            GIOStatus gstatus = 0;
             p->truncated = true;
             /* ignore truncated output */
             gchar buf[GUEST_EXEC_IO_SIZE];
-            g_io_channel_read_chars(ch, buf, sizeof(buf), &bytes_read, NULL);
+            gstatus = g_io_channel_read_chars(ch, buf, sizeof(buf),
+                                              &bytes_read, NULL);
+            if (gstatus == G_IO_STATUS_EOF || gstatus == G_IO_STATUS_ERROR) {
+                g_io_channel_unref(ch);
+                g_atomic_int_set(&p->closed, 1);
+                return false;
+            }
+
             return TRUE;
         }
         p->size += GUEST_EXEC_IO_SIZE;
@@ -342,8 +350,14 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
 
     /* Calling read API once.
      * On next available data our callback will be called again */
-    g_io_channel_read_chars(ch, (gchar *)p->data + p->length,
+    GIOStatus gstatus = 0;
+    gstatus = g_io_channel_read_chars(ch, (gchar *)p->data + p->length,
             p->size - p->length, &bytes_read, NULL);
+    if (gstatus == G_IO_STATUS_EOF || gstatus == G_IO_STATUS_ERROR) {
+        g_io_channel_unref(ch);
+        g_atomic_int_set(&p->closed, 1);
+        return false;
+    }

> ---
>  qga/commands.c       | 175 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  qga/qapi-schema.json |  11 +++-
>  2 files changed, 175 insertions(+), 11 deletions(-)
> 
> diff --git a/qga/commands.c b/qga/commands.c
> index b487324..740423f 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -15,6 +15,11 @@
>  #include "qga-qmp-commands.h"
>  #include "qapi/qmp/qerror.h"
> 
> +/* Maximum captured guest-exec out_data/err_data - 16MB */
> +#define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
> +/* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */
> +#define GUEST_EXEC_IO_SIZE (4*1024)
> +
>  /* Note: in some situations, like with the fsfreeze, logging may be
>   * temporarilly disabled. if it is necessary that a command be able
>   * to log for accounting purposes, check ga_logging_enabled() beforehand,
> @@ -71,10 +76,23 @@ struct GuestAgentInfo *qmp_guest_info(Error **errp)
>      return info;
>  }
> 
> +struct GuestExecIOData {
> +    guchar *data;
> +    gsize size;
> +    gsize length;
> +    gint closed;
> +    bool truncated;
> +    const char *name;
> +};
> +
>  struct GuestExecInfo {
>      GPid pid;
>      gint status;
> -    bool finished;
> +    bool has_output;
> +    gint finished;
> +    struct GuestExecIOData in;
> +    struct GuestExecIOData out;
> +    struct GuestExecIOData err;
>      QTAILQ_ENTRY(GuestExecInfo) next;
>  };
>  typedef struct GuestExecInfo GuestExecInfo;
> @@ -123,9 +141,18 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
>      }
> 
>      ges = g_new0(GuestExecStatus, 1);
> -    ges->exited = gei->finished;
> 
> -    if (gei->finished) {
> +    bool finished = g_atomic_int_get(&gei->finished);
> +
> +    /* need to wait till output channels are closed
> +     * to be sure we captured all output at this point */
> +    if (gei->has_output) {
> +        finished = finished && g_atomic_int_get(&gei->out.closed);
> +        finished = finished && g_atomic_int_get(&gei->err.closed);
> +    }
> +
> +    ges->exited = finished;
> +    if (finished) {
>          /* Glib has no portable way to parse exit status.
>           * On UNIX, we can get either exit code from normal termination
>           * or signal number.
> @@ -160,6 +187,20 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
>              ges->signal = WTERMSIG(gei->status);
>          }
>  #endif
> +        if (gei->out.length > 0) {
> +            ges->has_out_data = true;
> +            ges->out_data = g_base64_encode(gei->out.data, gei->out.length);
> +            g_free(gei->out.data);
> +            ges->has_out_truncated = gei->out.truncated;
> +        }
> +
> +        if (gei->err.length > 0) {
> +            ges->has_err_data = true;
> +            ges->err_data = g_base64_encode(gei->err.data, gei->err.length);
> +            g_free(gei->err.data);
> +            ges->has_err_truncated = gei->err.truncated;
> +        }
> +
>          QTAILQ_REMOVE(&guest_exec_state.processes, gei, next);
>          g_free(gei);
>      }
> @@ -230,6 +271,85 @@ static void guest_exec_task_setup(gpointer data)
>  #endif
>  }
> 
> +static gboolean guest_exec_input_watch(GIOChannel *ch,
> +        GIOCondition cond, gpointer p_)
> +{
> +    struct GuestExecIOData *p = (struct GuestExecIOData *)p_;
> +    gsize bytes_written = 0;
> +    GIOStatus status;
> +    GError *gerr = NULL;
> +
> +    /* nothing left to write */
> +    if (p->size == p->length) {
> +        goto done;
> +    }
> +
> +    status = g_io_channel_write_chars(ch, (gchar *)p->data + p->length,
> +            p->size - p->length, &bytes_written, &gerr);
> +
> +    /* can be not 0 even if not G_IO_STATUS_NORMAL */
> +    if (bytes_written != 0) {
> +        p->length += bytes_written;
> +    }
> +
> +    /* continue write, our callback will be called again */
> +    if (status == G_IO_STATUS_NORMAL || status == G_IO_STATUS_AGAIN) {
> +        return TRUE;
> +    }
> +
> +    if (gerr) {
> +        g_warning("qga: i/o error writing to inp_data channel: %s",
> +                gerr->message);
> +        g_error_free(gerr);
> +    }
> +
> +done:
> +    g_io_channel_shutdown(ch, true, NULL);
> +    g_io_channel_unref(ch);
> +    g_atomic_int_set(&p->closed, 1);
> +    g_free(p->data);
> +
> +    return FALSE;
> +}
> +
> +static gboolean guest_exec_output_watch(GIOChannel *ch,
> +        GIOCondition cond, gpointer p_)
> +{
> +    struct GuestExecIOData *p = (struct GuestExecIOData *)p_;
> +    gsize bytes_read = 0;
> +
> +    if (cond == G_IO_HUP) {
> +        g_io_channel_unref(ch);
> +        g_atomic_int_set(&p->closed, 1);
> +        return FALSE;
> +    }
> +
> +    if (p->size == p->length) {
> +        gpointer t = NULL;
> +        if (!p->truncated && p->size < GUEST_EXEC_MAX_OUTPUT) {
> +            t = g_try_realloc(p->data, p->size + GUEST_EXEC_IO_SIZE);
> +        }
> +        if (t == NULL) {
> +            p->truncated = true;
> +            /* ignore truncated output */
> +            gchar buf[GUEST_EXEC_IO_SIZE];
> +            g_io_channel_read_chars(ch, buf, sizeof(buf), &bytes_read, NULL);
> +            return TRUE;
> +        }
> +        p->size += GUEST_EXEC_IO_SIZE;
> +        p->data = t;
> +    }
> +
> +    /* Calling read API once.
> +     * On next available data our callback will be called again */
> +    g_io_channel_read_chars(ch, (gchar *)p->data + p->length,
> +            p->size - p->length, &bytes_read, NULL);
> +
> +    p->length += bytes_read;
> +
> +    return TRUE;
> +}
> +
>  GuestExec *qmp_guest_exec(const char *path,
>                         bool has_arg, strList *arg,
>                         bool has_env, strList *env,
> @@ -244,6 +364,10 @@ GuestExec *qmp_guest_exec(const char *path,
>      strList arglist;
>      gboolean ret;
>      GError *gerr = NULL;
> +    gint in_fd, out_fd, err_fd;
> +    GIOChannel *in_ch, *out_ch, *err_ch;
> +    GSpawnFlags flags;
> +    bool has_output = (has_capture_output && capture_output);
> 
>      arglist.value = (char *)path;
>      arglist.next = has_arg ? arg : NULL;
> @@ -251,11 +375,14 @@ GuestExec *qmp_guest_exec(const char *path,
>      argv = guest_exec_get_args(&arglist, true);
>      envp = guest_exec_get_args(has_env ? env : NULL, false);
> 
> -    ret = g_spawn_async_with_pipes(NULL, argv, envp,
> -            G_SPAWN_SEARCH_PATH |
> -            G_SPAWN_DO_NOT_REAP_CHILD |
> -            G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL,
> -            guest_exec_task_setup, NULL, &pid, NULL, NULL, NULL, &gerr);
> +    flags = G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD;
> +    if (!has_output) {
> +        flags |= G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
> +    }
> +
> +    ret = g_spawn_async_with_pipes(NULL, argv, envp, flags,
> +            guest_exec_task_setup, NULL, &pid, has_inp_data ? &in_fd : NULL,
> +            has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
>      if (!ret) {
>          error_setg(err, QERR_QGA_COMMAND_FAILED, gerr->message);
>          g_error_free(gerr);
> @@ -266,8 +393,40 @@ GuestExec *qmp_guest_exec(const char *path,
>      ge->pid = (int64_t)pid;
> 
>      gei = guest_exec_info_add(pid);
> +    gei->has_output = has_output;
>      g_child_watch_add(pid, guest_exec_child_watch, gei);
> 
> +    if (has_inp_data) {
> +        gei->in.data = g_base64_decode(inp_data, &gei->in.size);
> +#ifdef G_OS_WIN32
> +        in_ch = g_io_channel_win32_new_fd(in_ch);
> +#else
> +        in_ch = g_io_channel_unix_new(in_fd);
> +#endif
> +        g_io_channel_set_encoding(in_ch, NULL, NULL);
> +        g_io_channel_set_buffered(in_ch, false);
> +        g_io_channel_set_flags(in_ch, G_IO_FLAG_NONBLOCK, NULL);
> +        g_io_add_watch(in_ch, G_IO_OUT, guest_exec_input_watch, &gei->in);
> +    }
> +
> +    if (has_output) {
> +#ifdef G_OS_WIN32
> +        out_ch = g_io_channel_win32_new_fd(out_fd);
> +        err_ch = g_io_channel_win32_new_fd(err_fd);
> +#else
> +        out_ch = g_io_channel_unix_new(out_fd);
> +        err_ch = g_io_channel_unix_new(err_fd);
> +#endif
> +        g_io_channel_set_encoding(out_ch, NULL, NULL);
> +        g_io_channel_set_encoding(err_ch, NULL, NULL);
> +        g_io_channel_set_buffered(out_ch, false);
> +        g_io_channel_set_buffered(err_ch, false);
> +        g_io_add_watch(out_ch, G_IO_IN | G_IO_HUP,
> +                guest_exec_output_watch, &gei->out);
> +        g_io_add_watch(err_ch, G_IO_IN | G_IO_HUP,
> +                guest_exec_output_watch, &gei->err);
> +    }
> +
>  done:
>      g_free(argv);
>      g_free(envp);
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index da6b64f..6f5ea50 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -941,12 +941,17 @@
>  # @err-data: #optional base64-encoded stderr of the process
>  #       Note: @out-data and @err-data are present only
>  #       if 'capture-output' was specified for 'guest-exec'
> +# @out-truncated: #optional true if stdout was not fully captured
> +#       due to size limitation.
> +# @err-truncated: #optional true if stderr was not fully captured
> +#       due to size limitation.
>  #
>  # Since: 2.5
>  ##
>  { 'struct': 'GuestExecStatus',
>    'data': { 'exited': 'bool', '*exitcode': 'int', '*signal': 'int',
> -            '*out-data': 'str', '*err-data': 'str' }}
> +            '*out-data': 'str', '*err-data': 'str',
> +            '*out-truncated': 'bool', '*err-truncated': 'bool' }}
>  ##
>  # @guest-exec-status
>  #
> @@ -955,9 +960,9 @@
>  #
>  # @pid: pid returned from guest-exec
>  #
> -# Returns: @GuestExecStatus on success.
> +# Returns: GuestExecStatus on success.
>  #
> -# Since 2.3
> +# Since 2.5
>  ##
>  { 'command': 'guest-exec-status',
>    'data':    { 'pid': 'int' },
> -- 
> 2.1.4
>