On 08/02/2011 04:41 AM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 01:53:19PM -0600, Eric Blake wrote:
Steps to reproduce this problem (vm1 is not running):
for i in `seq 50`; do virsh managedsave vm1& done; killall virsh
Pre-patch, virNetServerClientClose could end up setting client->sock
to NULL prior to other cleanup functions trying to use client->sock.
This fixes things by checking for NULL in more places, and by deferring
the cleanup until after all queued messages have been served.
@@ -570,10 +577,7 @@ void virNetServerClientClose(virNetServerClientPtr client)
virNetTLSSessionFree(client->tls);
client->tls = NULL;
}
- if (client->sock) {
- virNetSocketFree(client->sock);
- client->sock = NULL;
- }
+ client->wantClose = true;
while (client->rx) {
virNetMessagePtr msg
@@ -586,6 +590,11 @@ void virNetServerClientClose(virNetServerClientPtr client)
virNetMessageFree(msg);
}
+ if (client->sock) {
+ virNetSocketFree(client->sock);
+ client->sock = NULL;
+ }
+
virNetServerClientUnlock(client);
}
I'm curious why you have these last 2 hunks of the patc ? The entire
of the virNetServerClientClose() is executed under the mutex, so AFAICT
moving these 4 lines to later in the function can have no effect. The
assignment to wantClose ought to not be needed at this point either,
since this flag is used by the server to decide to invoke the
virNetServerClientClose method.
I did it because virNetMessageFree calls a callback, and I didn't want
to audit whether that callback might temporarily drop the mutex or
reference client->sock.
That all said, these 2 hunks are at worst harmless, so ACK to the
patch.
Fair enough, so I pushed as-is.
--
Eric Blake ebl...@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
