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(a)gmail.com&gt; > 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.

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(a)gmail.com&gt; > > > 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.

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