[libvirt] PATCH: 8/25: Concurrent dispatch of RPC methods
Daniel P. Berrange
berrange at redhat.com
Fri Jan 16 15:25:57 UTC 2009
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 at redhat.com> 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 :|
More information about the libvir-list
mailing list