[Qemu-devel] [PATCH for 2.6 0/2] qga: guest exec bugfixes

[Qemu-devel] [PATCH for 2.6 0/2] qga: guest exec bugfixes

[Denis V. Lunev]

Signed-off-by: Yuriy Pudgorodskiy <yur@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 1/2] qga: fix fd leak with guest-exec i/o channels

[Denis V. Lunev]

From: Yuriy Pudgorodskiy <yur@virtuozzo.com>

Signed-off-by: Yuriy Pudgorodskiy <yur@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands.c b/qga/commands.c
index e091ee1..9ad2f7d 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -446,6 +446,7 @@ GuestExec *qmp_guest_exec(const char *path,
         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_channel_set_close_on_unref(in_ch, true);
         g_io_add_watch(in_ch, G_IO_OUT, guest_exec_input_watch, &gei->in);
     }
 
@@ -461,6 +462,8 @@ GuestExec *qmp_guest_exec(const char *path,
         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_channel_set_close_on_unref(out_ch, true);
+        g_io_channel_set_close_on_unref(err_ch, true);
         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,
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/2] qga: fix fd leak with guest-exec i/o channels

[Michael Roth]

Quoting Denis V. Lunev (2016-04-06 00:43:30)
> From: Yuriy Pudgorodskiy <yur@virtuozzo.com>
> 
> Signed-off-by: Yuriy Pudgorodskiy <yur@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qga/commands.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/qga/commands.c b/qga/commands.c
> index e091ee1..9ad2f7d 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -446,6 +446,7 @@ GuestExec *qmp_guest_exec(const char *path,
>          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_channel_set_close_on_unref(in_ch, true);
>          g_io_add_watch(in_ch, G_IO_OUT, guest_exec_input_watch, &gei->in);
>      }
> 
> @@ -461,6 +462,8 @@ GuestExec *qmp_guest_exec(const char *path,
>          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_channel_set_close_on_unref(out_ch, true);
> +        g_io_channel_set_close_on_unref(err_ch, true);

I don't seem any harm in adding these for safety, but don't the handles
get closed via the g_io_channel_shutdown(ch, ...) calls we make prior to
unref in guest_exec_{output,input}_watch()? Or is there another unref
path I'm missing?

>          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,
> -- 
> 2.1.4
> 

Re: [Qemu-devel] [PATCH 1/2] qga: fix fd leak with guest-exec i/o channels

[Yuriy Pudgorodskiy]

On 4/7/2016 2:53 AM, Michael Roth wrote:
> Quoting Denis V. Lunev (2016-04-06 00:43:30)
>> From: Yuriy Pudgorodskiy <yur@virtuozzo.com>
>>
>> Signed-off-by: Yuriy Pudgorodskiy <yur@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>   qga/commands.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/qga/commands.c b/qga/commands.c
>> index e091ee1..9ad2f7d 100644
>> --- a/qga/commands.c
>> +++ b/qga/commands.c
>> @@ -446,6 +446,7 @@ GuestExec *qmp_guest_exec(const char *path,
>>           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_channel_set_close_on_unref(in_ch, true);
>>           g_io_add_watch(in_ch, G_IO_OUT, guest_exec_input_watch, &gei->in);
>>       }
>>
>> @@ -461,6 +462,8 @@ GuestExec *qmp_guest_exec(const char *path,
>>           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_channel_set_close_on_unref(out_ch, true);
>> +        g_io_channel_set_close_on_unref(err_ch, true);
> I don't seem any harm in adding these for safety, but don't the handles
> get closed via the g_io_channel_shutdown(ch, ...) calls we make prior to
> unref in guest_exec_{output,input}_watch()? Or is there another unref
> path I'm missing?
>

Well,  it may be fixed with explicit g_io_channel_shutdown(), and may be 
it is a
better fix style, but it needs to be fixed somehow. We missed in our 
code a call
to shutdown in guest_exec_output_watch():


diff --git a/qga/commands.c b/qga/commands.c
index 45688c8..9acb2f6 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -373,6 +373,7 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
      return true;

  close:
+    g_io_channel_shutdown(ch, true, NULL);
      g_io_channel_unref(ch);
      g_atomic_int_set(&p->closed, 1);
      return 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,
>> -- 
>> 2.1.4
>>
> .
>

Re: [Qemu-devel] [PATCH 1/2] qga: fix fd leak with guest-exec i/o channels

[Michael Roth]

Quoting Yuriy Pudgorodskiy (2016-04-07 06:22:13)
> On 4/7/2016 2:53 AM, Michael Roth wrote:
> > Quoting Denis V. Lunev (2016-04-06 00:43:30)
> >> From: Yuriy Pudgorodskiy <yur@virtuozzo.com>
> >>
> >> Signed-off-by: Yuriy Pudgorodskiy <yur@virtuozzo.com>
> >> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> ---
> >>   qga/commands.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/qga/commands.c b/qga/commands.c
> >> index e091ee1..9ad2f7d 100644
> >> --- a/qga/commands.c
> >> +++ b/qga/commands.c
> >> @@ -446,6 +446,7 @@ GuestExec *qmp_guest_exec(const char *path,
> >>           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_channel_set_close_on_unref(in_ch, true);
> >>           g_io_add_watch(in_ch, G_IO_OUT, guest_exec_input_watch, &gei->in);
> >>       }
> >>
> >> @@ -461,6 +462,8 @@ GuestExec *qmp_guest_exec(const char *path,
> >>           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_channel_set_close_on_unref(out_ch, true);
> >> +        g_io_channel_set_close_on_unref(err_ch, true);
> > I don't seem any harm in adding these for safety, but don't the handles
> > get closed via the g_io_channel_shutdown(ch, ...) calls we make prior to
> > unref in guest_exec_{output,input}_watch()? Or is there another unref
> > path I'm missing?
> >
> 
> Well,  it may be fixed with explicit g_io_channel_shutdown(), and may be 
> it is a
> better fix style, but it needs to be fixed somehow. We missed in our 
> code a call
> to shutdown in guest_exec_output_watch():
> 
> 
> diff --git a/qga/commands.c b/qga/commands.c
> index 45688c8..9acb2f6 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -373,6 +373,7 @@ static gboolean guest_exec_output_watch(GIOChannel *ch,
>       return true;
> 
>   close:
> +    g_io_channel_shutdown(ch, true, NULL);
>       g_io_channel_unref(ch);
>       g_atomic_int_set(&p->closed, 1);
>       return false;

When I scanned the code I could've sworn I saw it for both input/output, but
you're right, it's missing for output. I might squash in the above
hunk for symmetry and to avoid confusion about whether it needs to be
added later, but for now I've applied it as-is.

> 
> 
> 
> >>           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,
> >> -- 
> >> 2.1.4
> >>
> > .
> >
> 

[Qemu-devel] [PATCH 2/2] qga: Workaround for console redirection from non-interactive qemu-ga service

[Denis V. Lunev]

From: Yuri Pudgorodskiy <yur@virtuozzo.com>

mingw-glib uses helper process to assist gspawn() api. There are two
versions of helpers, one with main() and another with WinMain() startup
routines.

Whenever gspawn() detects consoleless environment (and qemu-ga is running
in such environment as Win32 service), it chooses helper with main()
instead of WinMain. It is done by name, e.g.
gspawn-win32-helper-console.exe vs gspawn-win32-helper.exe

Running console-aware application like any win32 console apps from main()
crt initalized process results in redirection of stdout to console created
in crt startup instead of parent-provided handle connected to subprocess
pipe. Thus, stdout/stderr redirection do not work correctly.

The patch makes WinMain()'s version of helper be used as the only helper
shipped with qemu-ga package. Using only win32 helper ensures console
is created before any redirection and fixes stdout/stderr redirection
issue.

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>
---
 qga/installer/qemu-ga.wxs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 7f92891..fa2260c 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -96,7 +96,7 @@
             <File Id="gspawn-win32-helper-console.exe" Name="gspawn-win32-helper-console.exe" Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="gspawn-helper" Guid="{CD67A5A3-2DB1-4DA1-A67A-8D71E797B466}">
-            <File Id="gspawn-win32-helper.exe" Name="gspawn-win32-helper.exe" Source="$(var.Mingw_bin)/gspawn-win32-helper.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="gspawn-win32-helper.exe" Name="gspawn-win32-helper.exe" Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <?endif?>
           <?if $(var.Arch) = "64"?>
@@ -104,7 +104,7 @@
             <File Id="gspawn-win64-helper-console.exe" Name="gspawn-win64-helper-console.exe" Source="$(var.Mingw_bin)/gspawn-win64-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="gspawn-helper" Guid="{D201AD22-1846-4E4F-B6E1-C7A908ED2457}">
-            <File Id="gspawn-win64-helper.exe" Name="gspawn-win64-helper.exe" Source="$(var.Mingw_bin)/gspawn-win64-helper.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="gspawn-win64-helper.exe" Name="gspawn-win64-helper.exe" Source="$(var.Mingw_bin)/gspawn-win64-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <?endif?>
           <Component Id="iconv" Guid="{35EE3558-D34B-4F0A-B8BD-430FF0775246}">
-- 
2.1.4

Re: [Qemu-devel] [PATCH for 2.6 0/2] qga: guest exec bugfixes

[Michael Roth]

Quoting Denis V. Lunev (2016-04-06 00:43:29)
> Signed-off-by: Yuriy Pudgorodskiy <yur@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> 

Thanks, applied to qga tree:

  https://github.com/mdroth/qemu/commits/qga

Can confirm it fixes guest-exec output on Windows 7 for me (was already
working for Windows 8.1, I guess the service handling is a bit different
in that case)