On Fri, Jan 16, 2009 at 10:18:32AM -0500, Cole Robinson wrote: > Daniel P. Berrange wrote: > > On Fri, Jan 16, 2009 at 12:49:06PM +0100, Jim Meyering wrote: > >> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :) That mistake should never have got anywhere near the RPC code! We are supposed to test all mandatory parameter for non-NULL in the libvirt.c file. I see that the 'path' arg in virDomainInterfaceStats is not being checked for non-NULL, so that's another bug to fix. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list