[libvirt] [libvirt-glib] [PATCH 1/2] libvirt-gobject-domain: Add _fetch_snapshots

[Christophe Fergeau]

[sorry, this probably breaks threading as I mistakenly deleted the
original email]

On Mon, Jun 23, 2014 at 05:29:10PM +0200, Timm Bäder wrote:
> This function can be used to fetch the snapshots of a domain (according
> to the given GVirDomainSnapshotListFlags) and save them in a
> domain-internal GHashTable. A function to access them from outside will
> be added in a later patch.
> ---
>  libvirt-gobject/libvirt-gobject-domain.c | 59 ++++++++++++++++++++++++++++++++
>  libvirt-gobject/libvirt-gobject-domain.h | 36 +++++++++++++++++++
>  libvirt-gobject/libvirt-gobject.sym      |  2 ++
>  3 files changed, 97 insertions(+)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c
> index c6e30e5..f6b5837 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.c
> +++ b/libvirt-gobject/libvirt-gobject-domain.c
> @@ -38,6 +38,7 @@ struct _GVirDomainPrivate
>  {
>      virDomainPtr handle;
>      gchar uuid[VIR_UUID_STRING_BUFLEN];
> +    GHashTable *snapshots;
>  };
>  
>  G_DEFINE_TYPE(GVirDomain, gvir_domain, G_TYPE_OBJECT);
> @@ -121,6 +122,8 @@ static void gvir_domain_finalize(GObject *object)
>  
>      g_debug("Finalize GVirDomain=%p", domain);
>  
> +    g_hash_table_unref (priv->snapshots);
> +

You need to check if it's NULL before trying to unref it.

>      virDomainFree(priv->handle);
>  
>      G_OBJECT_CLASS(gvir_domain_parent_class)->finalize(object);
> @@ -1514,3 +1517,59 @@ gvir_domain_create_snapshot(GVirDomain *dom,
>      g_free(custom_xml);
>      return dom_snapshot;
>  }
> +
> +
> +
> +/**
> + * gvir_domain_fetch_snapshots:
> + * @dom: The domain
> + * @list_flags: bitwise-OR of #GVirDomainSnapshotListFlags
> + * @error: (allow-none): Place-holder for error or NULL
> + *
> + * Returns: TRUE on success, FALSE otherwise.
> + */
> +gboolean gvir_domain_fetch_snapshots(GVirDomain *dom,
> +                                     guint list_flags,
> +                                     GError **error)
> +{
> +    GVirDomainPrivate *priv;
> +    virDomainSnapshotPtr *snapshots = NULL;
> +    GVirDomainSnapshot *snap;
> +    int n_snaps = 0;
> +    int i;
> +
> +    g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
> +    g_return_val_if_fail((error == NULL) || (*error == NULL), FALSE);
> +
> +    priv = dom->priv;
> +
> +    if (priv->snapshots != NULL) {
> +        g_hash_table_destroy (priv->snapshots);

g_hash_table_unref() would be more consistent with what is done in
_finalize.

> +    }
> +
> +    priv->snapshots = g_hash_table_new_full(g_str_hash,
> +                                            g_str_equal,
> +                                            g_free,

The g_free() here seems wrong (more details below)

> +                                            g_object_unref);
> +
> +
> +    n_snaps = virDomainListAllSnapshots(priv->handle, &snapshots, list_flags);
> +
> +    if (n_snaps < 0) {
> +        gvir_set_error(error, GVIR_DOMAIN_ERROR, 0,
> +                       "Unable to fetch snapshots of %s",
> +                       gvir_domain_get_name (dom));
> +        return FALSE;
> +    }
> +
> +    for (i = 0; i < n_snaps; i ++) {
> +        snap = GVIR_DOMAIN_SNAPSHOT(g_object_new(GVIR_TYPE_DOMAIN_SNAPSHOT,
> +                                                 "handle", snapshots[i],
> +                                                 NULL));
> +        g_hash_table_insert(priv->snapshots,
> +                            (gpointer)gvir_domain_snapshot_get_name(snap),
> +                            snap);

gvir_domain_snapshot_get_name() does not return a copy of the string,
but just a const char * you do not own, so the g_free you passed to
g_hash_table_new_full to destroy the key is not correct.

> +    }
> +    g_free(snapshots);

snapshots was allocated by libvirt, so it should be freed using free(),
not g_free(). Most of the time, g_free() ends up calling free(), but
this can be changed (by the library user) with g_mem_set_vtable().

Patch looks good otherwise. As calling into libvirt can be costly (we
could be talking to a remote libvirt), do you have plans to implement an
async() variant of this as well?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140625/04fd787e/attachment-0001.sig>