[libvirt] [PATCH] qemu: fix not remove the pidfile when close a vm after restart libvirtd
https://bugzilla.redhat.com/show_bug.cgi?id=1197600 when create a happy vm and then restart libvirtd, we will loss priv->pidfile, because we don't check if it is there is a pidfile. However we only use this pidfile when we start the vm, and won't use it after it start, so this is not a big deal. But it is strange when vm is offline but pidfile still exist, so remove vmname.pid in state dir (maybe /run/libvirt/qemu/)when priv->pidfile is NULL. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..46b93b3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -92,6 +92,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, { char ebuf[1024]; char *file = NULL; + char *pidfile = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; @@ -99,16 +100,19 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) < 0) goto cleanup; + if (virAsprintf(&pidfile, "%s/%s.pid", cfg->stateDir, vm->def->name) < 0) + goto cleanup; + if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_WARN("Failed to remove domain XML for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); VIR_FREE(file); - if (priv->pidfile && - unlink(priv->pidfile) < 0 && + if (unlink(priv->pidfile ? priv->pidfile : pidfile) < 0 && errno != ENOENT) VIR_WARN("Failed to remove PID file for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); + VIR_FREE(pidfile); ret = 0; cleanup: -- 1.8.3.1
On 02.03.2015 10:37, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1197600
when create a happy vm and then restart libvirtd, we will loss priv->pidfile, because we don't check if it is there is a pidfile. However we only use this pidfile when we start the vm, and won't use it after it start, so this is not a big deal.
But it is strange when vm is offline but pidfile still exist, so remove vmname.pid in state dir (maybe /run/libvirt/qemu/)when priv->pidfile is NULL.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..46b93b3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -92,6 +92,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, { char ebuf[1024]; char *file = NULL; + char *pidfile = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; @@ -99,16 +100,19 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) < 0) goto cleanup;
+ if (virAsprintf(&pidfile, "%s/%s.pid", cfg->stateDir, vm->def->name) < 0) + goto cleanup;
If this fails, @file is leaked. And there's a helper function to generate pid file path: virPidFileBuildPath(). I know it does exactly the same, but lets use that, so whenever we decide on changing the path, we need to change it at one place only, instead of digging through source code just to check if somebody has not used virAsprintf() directly.
+ if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_WARN("Failed to remove domain XML for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); VIR_FREE(file);
- if (priv->pidfile && - unlink(priv->pidfile) < 0 && + if (unlink(priv->pidfile ? priv->pidfile : pidfile) < 0 && errno != ENOENT) VIR_WARN("Failed to remove PID file for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); + VIR_FREE(pidfile);
ret = 0; cleanup:
While this works, I think we need a different approach: https://www.redhat.com/archives/libvir-list/2015-March/msg00047.html Michal
On 03/03/2015 06:57 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1197600
when create a happy vm and then restart libvirtd, we will loss priv->pidfile, because we don't check if it is there is a pidfile. However we only use this pidfile when we start the vm, and won't use it after it start, so this is not a big deal.
But it is strange when vm is offline but pidfile still exist, so remove vmname.pid in state dir (maybe /run/libvirt/qemu/)when priv->pidfile is NULL.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..46b93b3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -92,6 +92,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, { char ebuf[1024]; char *file = NULL; + char *pidfile = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; @@ -99,16 +100,19 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver, if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) < 0) goto cleanup;
+ if (virAsprintf(&pidfile, "%s/%s.pid", cfg->stateDir, vm->def->name) < 0) + goto cleanup; If this fails, @file is leaked. And there's a helper function to generate pid file path: virPidFileBuildPath(). I know it does exactly
On 02.03.2015 10:37, Luyao Huang wrote: the same, but lets use that, so whenever we decide on changing the path, we need to change it at one place only, instead of digging through source code just to check if somebody has not used virAsprintf() directly.
Yes, i forgot check if &file will be leaked, thanks for pointing out that. About the virPidFileBuildPath(), i should say i missed this function from the code :) and use virPidFileBuildPath() is better than virAsprintf() in every sense.
+ if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_WARN("Failed to remove domain XML for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); VIR_FREE(file);
- if (priv->pidfile && - unlink(priv->pidfile) < 0 && + if (unlink(priv->pidfile ? priv->pidfile : pidfile) < 0 && errno != ENOENT) VIR_WARN("Failed to remove PID file for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); + VIR_FREE(pidfile);
ret = 0; cleanup:
While this works, I think we need a different approach:
https://www.redhat.com/archives/libvir-list/2015-March/msg00047.html
Good approach :)
Michal
Luyao
participants (3)
-
lhuang -
Luyao Huang
-
Michal Privoznik