[PATCH] qemu: clear residual QMP caps processes during QEMU driver initialization

[Bihong Yu]

ping

On 2020/7/20 10:09, Bihong Yu wrote:
> 
> 
> On 2020/7/18 5:14, Daniel Henrique Barboza wrote:
>>
>>
>> On 7/17/20 8:10 AM, Bihong Yu wrote:
>>>> From c328ff62b11d58553fd2032a85fd3295e009b3d3 Mon Sep 17 00:00:00 2001
>>> From: Bihong Yu <yubihong at huawei.com>
>>> Date: Fri, 17 Jul 2020 16:55:12 +0800
>>> Subject: [PATCH] qemu: clear residual QMP caps processes during QEMU driver
>>>   initialization
>>>
>>> In some cases, the QMP capabilities processes possible residue. So we need to
>>> clear the residual QMP caps processes during starting libvirt.
>>
>> Which cases are you referring to? Do you have a reproducer for this behavior? I
>> tried it up starting and stopping libvirtd, fetching capabilities, starting vms
>> and so on, and wasn't able to see any 'qmp.pid' file hanging around.
> 
> Yes, I have a reproducer for this behavior. I refer case is that send kill signal
> to libvirtd when libvirtd is fetching capabilities before libvirtd calling
> qemuProcessQMPFree() and qemuProcessQMPStop().
> 
> When libvirtd restart, it can not find the temporary qmp directory and has no way
> to clear the residual QMP caps processes.
> 
> Thanks,
> 
> Bihong Yu
> 
>>
>> About the code, I looked it up and all calls to qemuProcessQMPNew() are being
>> cleaned up accordingly with a qemuProcessQMPFree() function, which calls
>> qemuProcessQMPStop(), and the Stop function cleans up both the pid file and
>> the uniqDir:
>>
>> ----- qemuProcessQMPStop(qemuProcessQMPPtr proc) -----
>>
>>     if (proc->pidfile)
>>         unlink(proc->pidfile);
>>
>>     if (proc->uniqDir)
>>         rmdir(proc->uniqDir);
>>
>> ------
>>
>> The proper fix here would be to understand in which conditions the 'qmp.pid' file
>> is being left behind and make the code go through the existing cleanup path,
>> fixing qemuProcessQMPFree and/or qemuProcessQMPStop if needed. What you're doing
>> here works, but it's fixing the symptom of a bug instead of the bug itself.
>>
>>
>> Thanks,
>>
>>
>> DHB
>>
>>
>>>
>>> Signed-off-by:Bihong Yu <yubihong at huawei.com>
>>> ---
>>>   src/qemu/qemu_driver.c  |  2 ++
>>>   src/qemu/qemu_process.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>   src/qemu/qemu_process.h |  2 ++
>>>   3 files changed, 44 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index d185666..d627fd7 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -889,6 +889,8 @@ qemuStateInitialize(bool privileged,
>>>           run_gid = cfg->group;
>>>       }
>>>
>>> +    qemuProcessQMPClear(cfg->libDir);
>>> +
>>>       qemu_driver->qemuCapsCache = virQEMUCapsCacheNew(cfg->libDir,
>>>                                                        cfg->cacheDir,
>>>                                                        run_uid,
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 70fc24b..d545e3e 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -8312,6 +8312,46 @@ static qemuMonitorCallbacks callbacks = {
>>>   };
>>>
>>>
>>> +/**
>>> + * qemuProcessQMPClear
>>> + *
>>> + * Try to kill residual QMP caps processes
>>> + */
>>> +void
>>> +qemuProcessQMPClear(const char *libDir)
>>> +{
>>> +    virErrorPtr orig_err;
>>> +    DIR *dirp = NULL;
>>> +    struct dirent *dp;
>>> +
>>> +    if (!virFileExists(libDir))
>>> +        return;
>>> +
>>> +    if (virDirOpen(&dirp, libDir) < 0)
>>> +        return;
>>> +
>>> +    virErrorPreserveLast(&orig_err);
>>> +    while (virDirRead(dirp, &dp, libDir) > 0) {
>>> +        g_autofree char *qmp_uniqDir = NULL;
>>> +        g_autofree char *qmp_pidfile = NULL;
>>> +
>>> +        if (!STRPREFIX(dp->d_name, "qmp-"))
>>> +            continue;
>>> +
>>> +        qmp_uniqDir = g_strdup_printf("%s/%s", libDir, dp->d_name);
>>> +        qmp_pidfile = g_strdup_printf("%s/%s", qmp_uniqDir, "qmp.pid");
>>> +
>>> +        ignore_value(virPidFileForceCleanupPath(qmp_pidfile));
>>> +
>>> +        if (qmp_uniqDir)
>>> +            rmdir(qmp_uniqDir);
>>> +    }
>>> +    virErrorRestore(&orig_err);
>>> +
>>> +    VIR_DIR_CLOSE(dirp);
>>> +}
>>> +
>>> +
>>>   static void
>>>   qemuProcessQMPStop(qemuProcessQMPPtr proc)
>>>   {
>>> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
>>> index 15e67b9..b039e6c 100644
>>> --- a/src/qemu/qemu_process.h
>>> +++ b/src/qemu/qemu_process.h
>>> @@ -233,6 +233,8 @@ qemuProcessQMPPtr qemuProcessQMPNew(const char *binary,
>>>                                       gid_t runGid,
>>>                                       bool forceTCG);
>>>
>>> +void qemuProcessQMPClear(const char *libDir);
>>> +
>>>   void qemuProcessQMPFree(qemuProcessQMPPtr proc);
>>>
>>>   int qemuProcessQMPStart(qemuProcessQMPPtr proc);
>>>
>> .