[libvirt] [PATCH] rpc: Fix client crash on connection close
A multi-threaded client with event loop may crash if one of its threads closes a connection while event loop is in the middle of sending keep-alive message (either request or response). The right place for it is inside virNetClientIOEventLoop() between poll() and virNetClientLock(). We should only close a connection directly if no-one is using it and defer the closing to the last user otherwise. So far we only did so if the close was initiated by keep-alive timeout. --- src/rpc/virnetclient.c | 18 ++++-------------- 1 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 167fbf6..c2b901d 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -108,8 +108,6 @@ struct _virNetClient { }; -static void virNetClientRequestClose(virNetClientPtr client); - static void virNetClientLock(virNetClientPtr client) { virMutexLock(&client->lock); @@ -253,7 +251,7 @@ virNetClientKeepAliveStart(virNetClientPtr client, static void virNetClientKeepAliveDeadCB(void *opaque) { - virNetClientRequestClose(opaque); + virNetClientClose(opaque); } static int @@ -512,19 +510,11 @@ virNetClientCloseLocked(virNetClientPtr client) void virNetClientClose(virNetClientPtr client) { - if (!client) - return; - - virNetClientLock(client); - virNetClientCloseLocked(client); - virNetClientUnlock(client); -} - -static void -virNetClientRequestClose(virNetClientPtr client) -{ VIR_DEBUG("client=%p", client); + if (!client) + return; + virNetClientLock(client); /* If there is a thread polling for data on the socket, set wantClose flag -- 1.7.8.5
On 03/02/2012 01:49 PM, Jiri Denemark wrote:
A multi-threaded client with event loop may crash if one of its threads closes a connection while event loop is in the middle of sending keep-alive message (either request or response). The right place for it is inside virNetClientIOEventLoop() between poll() and virNetClientLock(). We should only close a connection directly if no-one is using it and defer the closing to the last user otherwise. So far we only did so if the close was initiated by keep-alive timeout. --- src/rpc/virnetclient.c | 18 ++++-------------- 1 files changed, 4 insertions(+), 14 deletions(-)
@@ -512,19 +510,11 @@ virNetClientCloseLocked(virNetClientPtr client)
void virNetClientClose(virNetClientPtr client) { - if (!client) - return; - - virNetClientLock(client); - virNetClientCloseLocked(client); - virNetClientUnlock(client); -} - -static void -virNetClientRequestClose(virNetClientPtr client) -{ VIR_DEBUG("client=%p", client);
The diff that git picked is a bit confusing; but it looks like all you are doing is stating that virNetClientClose should do the same thing as virNetClientRequestClose did (which is safer); and now that they do the same, you don't need two names, so pick the shorter name. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Mar 02, 2012 at 16:12:16 -0700, Eric Blake wrote:
On 03/02/2012 01:49 PM, Jiri Denemark wrote:
A multi-threaded client with event loop may crash if one of its threads closes a connection while event loop is in the middle of sending keep-alive message (either request or response). The right place for it is inside virNetClientIOEventLoop() between poll() and virNetClientLock(). We should only close a connection directly if no-one is using it and defer the closing to the last user otherwise. So far we only did so if the close was initiated by keep-alive timeout. --- src/rpc/virnetclient.c | 18 ++++-------------- 1 files changed, 4 insertions(+), 14 deletions(-)
@@ -512,19 +510,11 @@ virNetClientCloseLocked(virNetClientPtr client)
void virNetClientClose(virNetClientPtr client) { - if (!client) - return; - - virNetClientLock(client); - virNetClientCloseLocked(client); - virNetClientUnlock(client); -} - -static void -virNetClientRequestClose(virNetClientPtr client) -{ VIR_DEBUG("client=%p", client);
The diff that git picked is a bit confusing;
And I explicitly chose --patience diff since it generated better diff, which represents exactly what I did :-)
but it looks like all you are doing is stating that virNetClientClose should do the same thing as virNetClientRequestClose did (which is safer); and now that they do the same, you don't need two names, so pick the shorter name.
Right.
ACK.
Thanks and pushed. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark