[libvirt] [PATCH] qemu: fix not remove the pidfile when close a vm after restart libvirtd

[lhuang]

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 at 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