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

["Denis V. Lunev" <den@openvz.org>]

On 10/13/2015 07:05 AM, Michael Roth wrote:
> 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;
> +    }

not completely. we have tested your code and found that minor
change is required

Signed-off-by: Yuri Pudgorodskiy<yur@virtuozzo.com>
---
  qga/commands.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands.c b/qga/commands.c
index 1811ce6..deb041e 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -413,7 +413,7 @@ GuestExec *qmp_guest_exec(const char *path,
      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);
+        in_ch = g_io_channel_win32_new_fd(in_fd);
  #else
          in_ch = g_io_channel_unix_new(in_fd);
  #endif

I'll send a patch with this change and minor cosmetic
improvements soon (to make code shorter), but you can
take your version with this fix applied at your taste.

Den