Re: [libvirt] [PATCH] qemu: fix not remove the pidfile when close a vm after restart libvirtd
On 03/03/2015 06:57 PM, Michal Privoznik wrote:
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(a)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.
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