[libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination

[John Ferlan]

On 10/25/2017 04:24 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 25.10.2017 11:07, John Ferlan wrote:
>>
>>
>> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
>>> The problem is incorrect order of qemu driver shutdown and shutdown
>>> of netserver threads that serve client requests (thru qemu driver
>>> particularly).
>>>
>>> Net server threads are shutdowned upon dispose which is triggered
>>> by last daemon object unref at the end of main function. At the same
>>> time qemu driver is shutdowned earlier in virStateCleanup. As a result
>>> netserver threads see invalid driver object in the middle of request
>>> processing.
>>>
>>> Let's move shutting down netserver threads earlier to virNetDaemonClose.
>>>
>>> Note: order of last daemon unref and virStateCleanup
>>> is introduced in 85c3a182 for a valid reason.
>>> ---
>>>
>>> One can use next patch to trigger crash on termination. Call domstats function 
>>> and then send TERM to libvirtd.
>>>
>>> Patch to trigger crash
>>> #   diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> #   index cf5e4ad..39a57aa 100644
>>> #   --- a/src/qemu/qemu_driver.c
>>> #   +++ b/src/qemu/qemu_driver.c
>>> #   @@ -20144,6 +20144,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>>> #        domflags = 0;
>>> #        vm = vms[i];
>>> #
>>> #   +    sleep(5);
>>> #   +
>>> #        virObjectLock(vm);
>>> #
>>> #        if (HAVE_JOB(privflags) &&
>>>
>>>  src/rpc/virnetdaemon.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>>> index e3b9390..495bc4b 100644
>>> --- a/src/rpc/virnetdaemon.c
>>> +++ b/src/rpc/virnetdaemon.c
>>> @@ -880,6 +880,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
>>>      virObjectLock(dmn);
>>>  
>>>      virHashForEach(dmn->servers, daemonServerClose, NULL);
>>> +    virHashFree(dmn->servers);
>>
>> Should this be virHashRemoveAll?
> 
> Yep, it would be better.
> 
> I guess no need to resend the series...

I can change it locally before pushing - just wanted to check

John

> 
> Nikolay
> 
>>
>> Or I wonder if the virHashFree in virNetDaemonDispose.
>>
>> John
>>
>> (I'm at KVM Forum this week so responses will be delayed)
>>
>>> +    dmn->servers = NULL;
>>>  
>>>      virObjectUnlock(dmn);
>>>  }
>>>