[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] qga: fix fd leak with guest-exec i/o channe
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qga: fix fd leak with guest-exec i/o channels |
Date: |
Thu, 07 Apr 2016 09:26:28 -0500 |
User-agent: |
alot/0.3.6 |
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 <address@hidden>
> >>
> >> Signed-off-by: Yuriy Pudgorodskiy <address@hidden>
> >> Signed-off-by: Denis V. Lunev <address@hidden>
> >> CC: Michael Roth <address@hidden>
> >> ---
> >> 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
> >>
> > .
> >
>