Re: [libvirt] [PATCH v4 2/2] libvirtd: fix crash on termination

[John Ferlan]

On 10/27/2017 02:32 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 27.10.2017 08:26, John Ferlan wrote:
>> From: Nikolay Shirokovskiy 
>>
>> 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 shutdown upon dispose which is triggered
>> by last daemon object unref at the end of main function. At the same
>> time qemu driver is shutdown 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.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/rpc/virnetdaemon.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>> index d970c47ad4..7cb3214166 100644
>> --- a/src/rpc/virnetdaemon.c
>> +++ b/src/rpc/virnetdaemon.c
>> @@ -882,6 +882,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
>>  virObjectLock(dmn);
>>  
>>  virHashForEach(dmn->servers, daemonServerClose, NULL);
>> +virHashRemoveAll(dmn->servers);
>> +dmn->servers = NULL;
> 
> Setting NULL is not good, we leak hash table memory. Originally I call 
> virHashFree instead of
> virHashRemoveAll thus this line. But calling virHashFree earlier then dispose 
> is dangerous
> I agee with you here, virHashRemoveAll is better.
> 

See and I left the assignment from your patch.  Still, right
virHashRemoveAll doesn't do the VIR_FREE(table->table) and
VIR_FREE(table)...  I'll remove the assignment.

John

>>  
>>  virObjectUnlock(dmn);
>>  }
>>

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

Re: [libvirt] [PATCH v4 2/2] libvirtd: fix crash on termination

[Nikolay Shirokovskiy]

On 27.10.2017 08:26, John Ferlan wrote:
> From: Nikolay Shirokovskiy 
> 
> 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 shutdown upon dispose which is triggered
> by last daemon object unref at the end of main function. At the same
> time qemu driver is shutdown 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.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/rpc/virnetdaemon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index d970c47ad4..7cb3214166 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -882,6 +882,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
>  virObjectLock(dmn);
>  
>  virHashForEach(dmn->servers, daemonServerClose, NULL);
> +virHashRemoveAll(dmn->servers);
> +dmn->servers = NULL;

Setting NULL is not good, we leak hash table memory. Originally I call 
virHashFree instead of
virHashRemoveAll thus this line. But calling virHashFree earlier then dispose 
is dangerous
I agee with you here, virHashRemoveAll is better.

>  
>  virObjectUnlock(dmn);
>  }
> 

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

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

[John Ferlan]

From: Nikolay Shirokovskiy 

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 shutdown upon dispose which is triggered
by last daemon object unref at the end of main function. At the same
time qemu driver is shutdown 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.

Signed-off-by: John Ferlan 
---
 src/rpc/virnetdaemon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index d970c47ad4..7cb3214166 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -882,6 +882,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
 virObjectLock(dmn);
 
 virHashForEach(dmn->servers, daemonServerClose, NULL);
+virHashRemoveAll(dmn->servers);
+dmn->servers = NULL;
 
 virObjectUnlock(dmn);
 }
-- 
2.13.6

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