1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v3 0/2] daemon: fix termination/reload issues
  4. View patch

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

Nikolay Shirokovskiy posted 2 patches 7 years, 6 months ago
There is a newer version of this series
[libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination
Posted by Nikolay Shirokovskiy 7 years, 6 months ago 
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);
+    dmn->servers = NULL;
 
     virObjectUnlock(dmn);
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination
Posted by John Ferlan 7 years, 6 months ago 

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?

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);
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination
Posted by Nikolay Shirokovskiy 7 years, 6 months ago 

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

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);
>>  }
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination
Posted by John Ferlan 7 years, 6 months ago 

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);
>>>  }
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.