[libvirt] [PATCH 4/5] Change virConnectPtr into virObjectLocklable

[Peter Krempa]

On Mon, Apr 13, 2015 at 16:33:51 +0200, Martin Kletzander wrote:
> It already had a virMutex inside, so this is just a cleanup.

It also probably fixes a potential deadlock too.

> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  src/datatypes.c     | 12 ++----------
>  src/datatypes.h     | 12 +++++-------
>  src/libvirt-host.c  | 18 +++++++++---------
>  src/util/virerror.c | 18 +++++++++---------
>  4 files changed, 25 insertions(+), 35 deletions(-)
> 

...

> diff --git a/src/datatypes.h b/src/datatypes.h
> index 4973b07..9e19c55 100644
> --- a/src/datatypes.h
> +++ b/src/datatypes.h
> @@ -1,7 +1,7 @@
>  /*
>   * datatypes.h: management of structs for public data types
>   *
> - * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2015 Red Hat, Inc.

Yay, it's 2015 already? We are going to get hoverboards this year!

>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -328,7 +328,7 @@ struct _virConnectCloseCallbackData {
>   * Internal structure associated to a connection
>   */
>  struct _virConnect {
> -    virObject object;
> +    virObjectLockable object;
>      /* All the variables from here, until the 'lock' declaration
>       * are setup at time of connection open, and never changed
>       * since. Thus no need to lock when accessing them

The lock delcaration is no longer here, so you should mention the
comment you've changed below.

> @@ -352,12 +352,10 @@ struct _virConnect {
>      void *            privateData;
> 
>      /*
> -     * The lock mutex must be acquired before accessing/changing
> -     * any of members following this point, or changing the ref
> -     * count of any virDomain/virNetwork object associated with
> -     * this connection
> +     * Object lock must be acquired before accessing/changing any of
> +     * members following this point, or changing the ref count of any
> +     * virDomain/virNetwork object associated with this connection.
>       */
> -    virMutex lock;
> 
>      /* Per-connection error. */
>      virError err;           /* the last error */

...

> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
> index b4dc13e..03bee1f 100644
> --- a/src/libvirt-host.c
> +++ b/src/libvirt-host.c

...

> @@ -1288,15 +1288,15 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
>          conn->closeCallback->freeCallback(conn->closeCallback->opaque);
>      conn->closeCallback->freeCallback = NULL;
> 
> -    virObjectUnref(conn);
>      virObjectUnlock(conn->closeCallback);
> -    virMutexUnlock(&conn->lock);
> +    virObjectUnlock(conn);
> +    virObjectUnref(conn);

So this is the only semantic change in this patch. I'm surprised it
didn't bite us at some point actually, since the connection dispose code
is/was locking conn->lock. Perhaps it didn't ever order in the wrong
way.

> 
>      return 0;
> 
>   error:
>      virObjectUnlock(conn->closeCallback);
> -    virMutexUnlock(&conn->lock);
> +    virObjectUnlock(conn);
>      virDispatchError(conn);
>      return -1;
>  }

ACK with the comment updated.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150415/ffa77d36/attachment-0001.sig>