Re: [libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
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 <jferlan(a)redhat.com>
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