[libvirt] [PATCH 05/13] Convert public datatypes to inherit from virObject

[Eric Blake]

On 07/31/2012 10:58 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> This converts the following public API datatypes to use the
> virObject infrastructure:
> 
>   virConnectPtr
>   virDomainPtr
>   virDomainSnapshotPtr
>   virInterfacePtr
>   virNetworkPtr
>   virNodeDevicePtr
>   virNWFilterPtr
>   virSecretPtr
>   virStreamPtr
>   virStorageVolPtr
>   virStoragePoolPtr

Given recent changes, this now fails to apply cleanly.  Would you mind
refreshing the series, to make it easier to review?

>  16 files changed, 448 insertions(+), 1086 deletions(-)

Love the diffstat!

I still spotted some nits, even going solely by code inspection:

> @@ -225,64 +217,21 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>   * which may also be released if its ref count hits zero.
>   */
>  static void
> -virReleaseDomain(virDomainPtr domain) {
> -    virConnectPtr conn = domain->conn;
> +virDomainDispose(void *obj)
> +{

> -
> -    if (conn) {
> -        VIR_DEBUG("unref connection %p %d", conn, conn->refs);
> -        conn->refs--;
> -        if (conn->refs == 0) {
> -            virReleaseConnect(conn);
> -            /* Already unlocked mutex */
> -            return;
> -        }
> -        virMutexUnlock(&conn->lock);
> -    }
> +    if (domain->conn)
> +        virObjectUnref(domain->conn);

Technically, we have a bug if we ever have domain->conn==NULL when we
get here.  Besides, you coded virObjectUnref(NULL) to be a graceful
no-op, so that means you should add virObjectUnref to cfg.mk's list of
free-like functions and drop the conditional.

Oh, that reminds me - we DO have a bug in our RPC code where we fail to
check for NULL after strdup and such in the various make_nonnull_*
functions, and therefore I suppose that is an instance where we really
_could_ end up with domain->conn being NULL at the moment - I've been
meaning to spend some time on fixing that.

> @@ -349,60 +289,17 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>   * which may also be released if its ref count hits zero.
>   */
>  static void
> -virReleaseNetwork(virNetworkPtr network) {
> -    virConnectPtr conn = network->conn;
> +virNetworkDispose(void *obj)
> +{
> +    virNetworkPtr network = obj;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];

> +    if (network->conn)
> +        virObjectUnref(network->conn);

Another place where you can drop the conditional.

> @@ -484,58 +366,15 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) {
>   * which may also be released if its ref count hits zero.
>   */
>  static void
> -virReleaseInterface(virInterfacePtr iface) {
> -    virConnectPtr conn = iface->conn;
> +virInterfaceDispose(void *obj)
> +{
> +    virInterfacePtr iface = obj;
> +    if (iface->conn)
> +        virObjectUnref(iface->conn);

and again

> @@ -608,60 +438,17 @@ error:
>   * which may also be released if its ref count hits zero.
>   */
>  static void
> -virReleaseStoragePool(virStoragePoolPtr pool) {
> -    virConnectPtr conn = pool->conn;
> +virStoragePoolDispose(void *obj)
> +{
> +    virStoragePoolPtr pool = obj;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];

> +    if (pool->conn)
> +        virObjectUnref(pool->conn);

I'm turning into a broken record...

> @@ -748,59 +516,16 @@ error:
>   * which may also be released if its ref count hits zero.
>   */
>  static void
> -virReleaseStorageVol(virStorageVolPtr vol) {
> -    virConnectPtr conn = vol->conn;
> +virStorageVolDispose(void *obj)
> +{
> +    virStorageVolPtr vol = obj;

> +    if (vol->conn)
> +        virObjectUnref(vol->conn);

Need I say it?  :)
I'll quit mentioning it (again, a cfg.mk rule will make it easier to
find all instances).

> +++ b/src/datatypes.h

> +# define VIR_IS_CONNECT(obj) \
> +    (virObjectIsClass((virObjectPtr)(obj), virConnectClass))

Why the extra cast here?  The compiler can convert obj to 'void*'
without your help.

> +
> +# define VIR_IS_DOMAIN(obj) \
> +    (virObjectIsClass((virObjectPtr)(obj), virDomainClass))

Likewise throughout these macros.

> +++ b/src/libvirt.c

> @@ -1420,14 +1420,16 @@ error:
>   * matching virConnectClose, and all other references will be released
>   * after the corresponding operation completes.
>   *
> - * Returns the number of remaining references on success
> - * (positive implies that some other call still has a reference open,
> - * 0 implies that no references remain and the connection is closed),
> - * or -1 on failure.  It is possible for the last virConnectClose to
> - * return a positive value if some other object still has a temporary
> - * reference to the connection, but the application should not try to
> - * further use a connection after the virConnectClose that matches the
> - * initial open.
> + * Returns the a positive number if at least 1 reference remains on

s/the a/a/

> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> +virSecretClass;
> +virStorageVolClass;
> +virStoragePoolClass;

Sorting.

> +++ b/src/xen/xend_internal.c
> @@ -1237,7 +1237,7 @@ error:
>      virReportError(VIR_ERR_INTERNAL_ERROR,
>                     "%s", _("failed to parse Xend domain information"));
>      if (ret != NULL)
> -        virUnrefDomain(ret);
> +        virObjectUnref(ret);

Another useless if before free.

> +++ b/tests/sexpr2xmltest.c
> @@ -72,7 +72,7 @@ testCompareFiles(const char *xml, const char *sexpr, int xendConfigVersion)
>    VIR_FREE(gotxml);
>    virDomainDefFree(def);
>    if (conn)
> -    virUnrefConnect(conn);
> +    virObjectUnref(conn);

Oops, I promised not to point them out :)

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120803/370778df/attachment-0001.sig>