[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Close logfile fd after spawning qemu



On Thu, Sep 10, 2009 at 6:35 PM, Daniel Veillard<veillard redhat com> wrote:
> On Thu, Sep 10, 2009 at 10:15:58AM +0100, Mark McLoughlin wrote:
>> On Thu, 2009-09-10 at 11:04 +0200, Daniel Veillard wrote:
>> > On Thu, Sep 10, 2009 at 02:13:56PM +0900, Ryota Ozaki wrote:
>> > > Hi,
>> > >
>> > > This patch closes logfile fd after spawing qemu in qemudStartVMDaemon.
>> > > The fd seems to be closed in the error path, but not in the normal path.
>> > > The fd is passed to virExecDaemonize though, but looks not being closed
>> > > inside it. Eventually, the fd is never closed during libvirtd lifetime.
>> > >
>> > > Thanks,
>> > >   ozaki-r
>> > >
>> > > >From b3d3e0f24c5df5c7677e539bbc2598f2d7fbc3b8 Mon Sep 17 00:00:00 2001
>> > > From: Ryota Ozaki <ozaki ryota gmail com>
>> > > Date: Thu, 10 Sep 2009 12:53:56 +0900
>> > > Subject: [PATCH] Close logfile fd after spawning qemu
>> > >
>> > > ---
>> > >  src/qemu_driver.c |    1 +
>> > >  1 files changed, 1 insertions(+), 0 deletions(-)
>> > >
>> > > diff --git a/src/qemu_driver.c b/src/qemu_driver.c
>> > > index 8f16e72..f2182c4 100644
>> > > --- a/src/qemu_driver.c
>> > > +++ b/src/qemu_driver.c
>> > > @@ -2187,6 +2187,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
>> > >                             VIR_EXEC_NONBLOCK | VIR_EXEC_CLEAR_CAPS,
>> > >                             qemudSecurityHook, &hookData,
>> > >                             pidfile);
>> > > +    close(logfile);
>> > >      VIR_FREE(pidfile);
>> > >
>> > >      /* wait for qemu process to to show up */
>> >
>> >   Hum, it's not quite simple.
>>
>> Indeed. I only had a quick look and wasn't sure. We at least need to set
>> logfile to -1 after closing it to avoid trying to close it again further
>> down.
>
>  Okay, the fd is passed by address to virExecDaemonize, since it was
> properly initialized it should not be overwritten by __virExec, but as a
> safety I think this should be checked for -1 before close. The fd is not
> written anywhere, so we are pretty much sure it's a lost fd at this
> point and there is no way the application could use it beyond the
> function return. I'm just changing the patch slightly to
>
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index 5c2a8ec..d778a89 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -2239,6 +2239,9 @@ static int qemudStartVMDaemon(virConnectPtr conn,
>         /* No need for 'goto cleanup' now since qemudShutdownVMDaemon does enou
>     }
>
> +    if (logfile != -1)
> +        close(logfile);
> +
>     return ret;
>
>  cleanup:
>
>
>  i.e. check for -1 and close only at the non-cleanup return point.
> I just commited this.

I think it's ok. Thanks!
  ozaki-r

>
>  thanks !
>
> Daniel
>
> --
> Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library  http://libvirt.org/
>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]