On Thu, Jul 19, 2018 at 04:51 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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 <mhartmay@xxxxxxxxxxxxx>
>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
>> ---
>> 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 <jferlan@xxxxxxxxxx>
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
