[libvirt] [PATCH] Close logfile fd after spawning qemu
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 */ -- 1.6.0.6
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. It's passed down to __virExec() where it is provided as the stdout/stderr to use, it's not closed there on the parent though. I guess it's safe to close it at that point at the end of qemudStartVMDaemon() since only the child can use it (up to the exec because it is been close on exec'ed). So looks fine to me but I would appreciate a second opinion :-) 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/
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. Cheers, Mark.
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. 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/
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/
On Thu, Sep 10, 2009 at 6:04 PM, Daniel Veillard<veillard@redhat.com> wrote:
Hum, it's not quite simple. It's passed down to __virExec() where it is provided as the stdout/stderr to use, it's not closed there on the parent though. I guess it's safe to close it at that point at the end of qemudStartVMDaemon() since only the child can use it (up to the exec because it is been close on exec'ed).
Hmm, I'm not sure the effect to the child though, it sounds safer. ozaki-r
So looks fine to me but I would appreciate a second opinion :-)
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/
participants (3)
-
Daniel Veillard -
Mark McLoughlin -
Ryota Ozaki