On Tue, Nov 13, 2012 at 6:30 PM, Christophe Fergeau <cfergeau(a)redhat.com&gt; wrote: With g_type_init defined for newer glib, we'll want to keep the g_type_init() calls, no? Looks good otherwise. -- Regards, Zeeshan Ali (Khattak) FSF member#5124

Without this change, gvir_storage_pool_get_volume segfaults when trying to do the hash table lookup with a NULL 'name' key. --- libvirt-gobject/libvirt-gobject-storage-pool.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index dc34c2e..a380079 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -538,6 +538,7 @@ GVirStorageVol *gvir_storage_pool_get_volume(GVirStoragePool *pool, GVirStorageVol *volume; g_return_val_if_fail(GVIR_IS_STORAGE_POOL(pool), NULL); + g_return_val_if_fail(name != NULL, NULL); priv = pool->priv; g_mutex_lock(priv->lock); -- 1.8.0

When creating an object wrapping a libvirt object (GVirDomain, GVirStoragePool, GVirStorageVol), libvirt-gobject gets a reference to a libvirt object to be used as a handle, and then creates the wrapper object by calling g_object_new(..., "handle", handle, NULL); However, the underlying libvirt object is registered as a boxed type with the gobject type system, and the handle setter for these object calls g_value_dup_boxed, which in turn adds a reference on the libvirt handle. Thus we must release the initial ref we hold on the libvirt handle after calling g_object_new(). I noticed this bug after running in valgrind some code which calls gvir_storage_pool_refresh and gvir_connection_fetch_storage_pools. --- libvirt-gobject/libvirt-gobject-connection.c | 7 +++++++ libvirt-gobject/libvirt-gobject-storage-pool.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index e6ccfd0..3157a66 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -726,6 +726,7 @@ gboolean gvir_connection_fetch_domains(GVirConnection *conn, dom = GVIR_DOMAIN(g_object_new(GVIR_TYPE_DOMAIN, "handle", vdom, NULL)); + virDomainFree(vdom); g_hash_table_insert(doms, (gpointer)gvir_domain_get_uuid(dom), @@ -744,6 +745,7 @@ gboolean gvir_connection_fetch_domains(GVirConnection *conn, dom = GVIR_DOMAIN(g_object_new(GVIR_TYPE_DOMAIN, "handle", vdom, NULL)); + virDomainFree(vdom); g_hash_table_insert(doms, (gpointer)gvir_domain_get_uuid(dom), @@ -857,6 +859,7 @@ gboolean gvir_connection_fetch_storage_pools(GVirConnection *conn, pool = GVIR_STORAGE_POOL(g_object_new(GVIR_TYPE_STORAGE_POOL, "handle", vpool, NULL)); + virStoragePoolFree(vpool); g_hash_table_insert(pools, (gpointer)gvir_storage_pool_get_uuid(pool), @@ -877,6 +880,7 @@ gboolean gvir_connection_fetch_storage_pools(GVirConnection *conn, pool = GVIR_STORAGE_POOL(g_object_new(GVIR_TYPE_STORAGE_POOL, "handle", vpool, NULL)); + virStoragePoolFree(vpool); g_hash_table_insert(pools, (gpointer)gvir_storage_pool_get_uuid(pool), @@ -1427,6 +1431,7 @@ GVirDomain *gvir_connection_create_domain(GVirConnection *conn, domain = GVIR_DOMAIN(g_object_new(GVIR_TYPE_DOMAIN, "handle", handle, NULL)); + virDomainFree(handle); g_mutex_lock(priv->lock); g_hash_table_insert(priv->domains, @@ -1481,6 +1486,7 @@ GVirDomain *gvir_connection_start_domain(GVirConnection *conn, domain = GVIR_DOMAIN(g_object_new(GVIR_TYPE_DOMAIN, "handle", handle, NULL)); + virDomainFree(handle); g_mutex_lock(priv->lock); g_hash_table_insert(priv->domains, @@ -1532,6 +1538,7 @@ GVirStoragePool *gvir_connection_create_storage_pool pool = GVIR_STORAGE_POOL(g_object_new(GVIR_TYPE_STORAGE_POOL, "handle", handle, NULL)); + virStoragePoolFree(handle); g_mutex_lock(priv->lock); g_hash_table_insert(priv->pools, diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index a380079..96670df 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -406,7 +406,7 @@ gboolean gvir_storage_pool_refresh(GVirStoragePool *pool, "handle", vvolume, "pool", pool, NULL)); - + virStorageVolFree(vvolume); g_hash_table_insert(vol_hash, g_strdup(volumes[i]), volume); } -- 1.8.0

When GLib deprecated g_mutex_new/g_mutex_free, we introduced a compatibility wrapper to implement these using non-deprecated functions. This was done by allocating 0-filled memory by the mutex, and then letting GLib initialize the structure when needed. However, we must call g_mutex_clear when destroying the mutex to free these resources, which this patch fix. --- libvirt-gobject/libvirt-gobject-compat.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-compat.h b/libvirt-gobject/libvirt-gobject-compat.h index 839dfe1..40e118d 100644 --- a/libvirt-gobject/libvirt-gobject-compat.h +++ b/libvirt-gobject/libvirt-gobject-compat.h @@ -27,8 +27,24 @@ #include <gio/gio.h> #if GLIB_CHECK_VERSION(2, 31, 0) -#define g_mutex_new() g_new0(GMutex, 1) -#define g_mutex_free(m) g_free(m) +static inline GMutex *gvir_mutex_new(void) +{ + GMutex *mutex; + + mutex = g_new(GMutex, 1); + g_mutex_init(mutex); + + return mutex; +} + +static inline void gvir_mutex_free(GMutex *mutex) +{ + g_mutex_clear(mutex); + g_free(mutex); +} + +#define g_mutex_new gvir_mutex_new +#define g_mutex_free gvir_mutex_free #endif #if !GLIB_CHECK_VERSION(2,26,0) -- 1.8.0

--- libvirt-gobject/libvirt-gobject-stream.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c index f0e43d0..2cb8967 100644 --- a/libvirt-gobject/libvirt-gobject-stream.c +++ b/libvirt-gobject/libvirt-gobject-stream.c @@ -414,8 +414,7 @@ gvir_stream_receive_all(GVirStream *self, * If there is no data available, a %G_IO_ERROR_WOULD_BLOCK error will be * returned. * - * Returns: Number of bytes read, or 0 if the end of stream reached, - * or -1 on error. + * Returns: Number of bytes written. */ gssize gvir_stream_send(GVirStream *self, const gchar *buffer, -- 1.8.0