On Thu, Sep 10, 2009 at 6:35 PM, Daniel Veillard<veillard@xxxxxxxxxx> 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@xxxxxxxxx> >> > > 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ > http://veillard.com/ | virtualization library http://libvirt.org/ > -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list