Re: [libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available

2018-07-20 Thread Marc Hartmayer 
On Thu, Jul 19, 2018 at 04:51 PM +0200, John Ferlan  wrote:
> On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
>> @srv must be unlocked for the call virNetServerProcessMsg otherwise a
>> deadlock can occur.
>> 
>> Since the pointer 'srv->workers' will never be changed after
>> initialization and the thread pool has it's own locking we can release
>> the lock of 'srv' earlier. This also fixes the deadlock.
>> 
>> Signed-off-by: Marc Hartmayer 
>> Reviewed-by: Boris Fiuczynski 
>> Reviewed-by: Bjoern Walk 
>> ---
>>  src/rpc/virnetserver.c | 15 ++-
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> index 5c7f7dd08fe1..091e3ef23406 100644
>> --- a/src/rpc/virnetserver.c
>> +++ b/src/rpc/virnetserver.c
>> @@ -51,6 +51,7 @@ struct _virNetServer {
>>  
>>  char *name;
>>  
>> +/* Immutable pointer, self-locking APIs */
>>  virThreadPoolPtr workers;
>>  
>>  char *mdnsGroupName;
>> @@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void 
>> *opaque)
>>  VIR_FREE(job);
>>  }
>>  
>> -static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
>> -   virNetMessagePtr msg,
>> -   void *opaque)
>> +
>> +static void
>> +virNetServerDispatchNewMessage(virNetServerClientPtr client,
>> +   virNetMessagePtr msg,
>> +   void *opaque)
>>  {
>>  virNetServerPtr srv = opaque;
>>  virNetServerProgramPtr prog = NULL;
>> @@ -196,6 +199,9 @@ static void 
>> virNetServerDispatchNewMessage(virNetServerClientPtr client,
>>  break;
>>  }
>>  }
>
> Should we grab an object reference then to avoid the chance that
> something disposes srv right after it's unlocked? And then Unref instead
> of Unlock later on?
>
> Following virThreadPoolSendJob finds that it'll grab the srv->workers
> pool mutex and check the quit flag before adding a job to and of course
> as you point out virNetServerProcessMsg would eventually assume that the
> server is unlocked in virNetServerProgramDispatchCall before calling
> dispatcher->func.
>
> With a Ref/Unref, I'd feel better and thus consider this,

Yep, I’m fine with that.

>
> Reviewed-by: John Ferlan 

Thanks!

>
> John
>
>> +/* we can unlock @srv since @prog can only become invalid in case
>> + * of disposing @srv */
>> +virObjectUnlock(srv);
>>  
>>  if (srv->workers) {
>>  virNetServerJobPtr job;
>> @@ -223,15 +229,14 @@ static void 
>> virNetServerDispatchNewMessage(virNetServerClientPtr client,
>>  goto error;
>>  }
>>  
>> -virObjectUnlock(srv);
>>  return;
>>  
>>   error:
>>  virNetMessageFree(msg);
>>  virNetServerClientClose(client);
>> -virObjectUnlock(srv);
>>  }
>>  
>> +
>>  /**
>>   * virNetServerCheckLimits:
>>   * @srv: server to check limits on
>> 
>
-- 
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

Re: [libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available

2018-07-19 Thread John Ferlan 



On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> @srv must be unlocked for the call virNetServerProcessMsg otherwise a
> deadlock can occur.
> 
> Since the pointer 'srv->workers' will never be changed after
> initialization and the thread pool has it's own locking we can release
> the lock of 'srv' earlier. This also fixes the deadlock.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Bjoern Walk 
> ---
>  src/rpc/virnetserver.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 5c7f7dd08fe1..091e3ef23406 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -51,6 +51,7 @@ struct _virNetServer {
>  
>  char *name;
>  
> +/* Immutable pointer, self-locking APIs */
>  virThreadPoolPtr workers;
>  
>  char *mdnsGroupName;
> @@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void 
> *opaque)
>  VIR_FREE(job);
>  }
>  
> -static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
> -   virNetMessagePtr msg,
> -   void *opaque)
> +
> +static void
> +virNetServerDispatchNewMessage(virNetServerClientPtr client,
> +   virNetMessagePtr msg,
> +   void *opaque)
>  {
>  virNetServerPtr srv = opaque;
>  virNetServerProgramPtr prog = NULL;
> @@ -196,6 +199,9 @@ static void 
> virNetServerDispatchNewMessage(virNetServerClientPtr client,
>  break;
>  }
>  }

Should we grab an object reference then to avoid the chance that
something disposes srv right after it's unlocked? And then Unref instead
of Unlock later on?

Following virThreadPoolSendJob finds that it'll grab the srv->workers
pool mutex and check the quit flag before adding a job to and of course
as you point out virNetServerProcessMsg would eventually assume that the
server is unlocked in virNetServerProgramDispatchCall before calling
dispatcher->func.

With a Ref/Unref, I'd feel better and thus consider this,

Reviewed-by: John Ferlan 

John

> +/* we can unlock @srv since @prog can only become invalid in case
> + * of disposing @srv */
> +virObjectUnlock(srv);
>  
>  if (srv->workers) {
>  virNetServerJobPtr job;
> @@ -223,15 +229,14 @@ static void 
> virNetServerDispatchNewMessage(virNetServerClientPtr client,
>  goto error;
>  }
>  
> -virObjectUnlock(srv);
>  return;
>  
>   error:
>  virNetMessageFree(msg);
>  virNetServerClientClose(client);
> -virObjectUnlock(srv);
>  }
>  
> +
>  /**
>   * virNetServerCheckLimits:
>   * @srv: server to check limits on
> 

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

[libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available

2018-07-03 Thread Marc Hartmayer 
@srv must be unlocked for the call virNetServerProcessMsg otherwise a
deadlock can occur.

Since the pointer 'srv->workers' will never be changed after
initialization and the thread pool has it's own locking we can release
the lock of 'srv' earlier. This also fixes the deadlock.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 src/rpc/virnetserver.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5c7f7dd08fe1..091e3ef23406 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -51,6 +51,7 @@ struct _virNetServer {
 
 char *name;
 
+/* Immutable pointer, self-locking APIs */
 virThreadPoolPtr workers;
 
 char *mdnsGroupName;
@@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void 
*opaque)
 VIR_FREE(job);
 }
 
-static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
-   virNetMessagePtr msg,
-   void *opaque)
+
+static void
+virNetServerDispatchNewMessage(virNetServerClientPtr client,
+   virNetMessagePtr msg,
+   void *opaque)
 {
 virNetServerPtr srv = opaque;
 virNetServerProgramPtr prog = NULL;
@@ -196,6 +199,9 @@ static void 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
 break;
 }
 }
+/* we can unlock @srv since @prog can only become invalid in case
+ * of disposing @srv */
+virObjectUnlock(srv);
 
 if (srv->workers) {
 virNetServerJobPtr job;
@@ -223,15 +229,14 @@ static void 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
 goto error;
 }
 
-virObjectUnlock(srv);
 return;
 
  error:
 virNetMessageFree(msg);
 virNetServerClientClose(client);
-virObjectUnlock(srv);
 }
 
+
 /**
  * virNetServerCheckLimits:
  * @srv: server to check limits on
-- 
2.13.4

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