On 06/28/2011 11:01 AM, Daniel P. Berrange wrote:
> The client stream object can be used independantly of the
s/independantly/independently/
> virNetClientPtr object, so must have full locking of its
> own and not rely on any caller.
>
> * src/remote/remote_driver.c: Remove locking around stream
> callback
> * src/rpc/virnetclientstream.c: Add locking to all APIs
> and callbacks
> ---
> src/remote/remote_driver.c | 3 -
> src/rpc/virnetclientstream.c | 112 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 89 insertions(+), 26 deletions(-)
>
> @@ -390,20 +435,23 @@ int virNetClientStreamEventAddCallback(virNetClientStreamPtr st,
> void *opaque,
> virFreeCallback ff)
> {
> + int ret = -1;
> +
> + virMutexLock(&st->lock);
> if (st->cb) {
> virNetError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("multiple stream callbacks not supported"));
> - return 1;
> + goto cleanup;
> }
>
> - virNetClientStreamRef(st);
> + st->refs++;
> if ((st->cbTimer =
> virEventAddTimeout(-1,
> virNetClientStreamEventTimer,
> st,
> virNetClientStreamEventTimerFree)) < 0) {
> - virNetClientStreamFree(st);
> - return -1;
> + st->refs--;
> + goto cleanup;
> }
This increments st->refs on success,
> int virNetClientStreamEventRemoveCallback(virNetClientStreamPtr st)
> {
> + int ret = -1;
> +
> + virMutexUnlock(&st->lock);
> if (!st->cb) {
> virNetError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("no stream callback registered"));
> - return -1;
> + goto cleanup;
> }
>
> if (!st->cbDispatch &&
but nothing here ever decrements it.
That looks like the only flaw; so ACK if you fix things to guarantee no
leaked st on a paired callback add/remove.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list