Re: [libvirt] PATCH: 8/25: Concurrent dispatch of RPC methods

Daniel P. Berrange wrote: > On Fri, Jan 16, 2009 at 12:49:06PM +0100, Jim Meyering wrote: >> "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: >>> This patch re-writes the code for dispatching RPC calls in the >>> remote driver to allow use from multiple threads. Only one thread >>> is allowed to send/recv on the socket at a time though. If another >>> thread comes along it will put itself on a queue and go to sleep. >>> The first thread may actually get around to transmitting the 2nd >>> thread's request while it is waiting for its own reply. It may >>> even get the 2nd threads reply, if its own RPC call is being really >>> slow. So when a thread wakes up from sleeping, it has to check >>> whether its own RPC call has already been processed. Likewise when >>> a thread owning the socket finishes with its own wor, it may have >>> to pass the buck to another thread. The upshot of this, is that >>> we have mutliple RPC calls executing in parallel, and requests+reply >>> are no longer guarenteed to be FIFO on the wire if talking to a new >>> enough server. >>> >>> This refactoring required use of a self-pipe/poll trick for sync >>> between threads, but fortunately gnulib now provides this on Windows >>> too, so there's no compatability problem there. <snip> > >>> + goto error; >>> + } >>> + xdr_destroy (&xdr); >>> + >>> + return rv; >>> + >>> +error: >>> + VIR_FREE(ret); >>> + return NULL; >> The above should free rv, not ret: >> >> VIR_FREE(rv); > > Doh, bad naming convention for arguments - we always use 'ret' for return > values. I should rename the argument to 'reply' since its what contains > the RPC reply, so we can use the normal convention of 'ret' for return > value. > I actually just tracked this down independently: the above bug was crashing virt-manager, where we incorrectly were passing 'None' to interfaceStats occasionally. So, good catch Jim :)