[libvirt] [libvirt-glib] miscellaneous fixes
This series does misc leak fixes in libvirt-glib, as well as an API doc fix, and a deprecation warning fix. Christophe
This function call is deprecated and calling it causes a compilation warning. --- libvirt-gconfig/Makefile.am | 1 + libvirt-gconfig/libvirt-gconfig-compat.h | 32 +++++++++++++++++++++++++++++++ libvirt-gconfig/libvirt-gconfig-main.c | 1 + libvirt-gconfig/tests/test-domain-parse.c | 2 -- libvirt-gobject/libvirt-gobject-main.c | 1 + 5 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 libvirt-gconfig/libvirt-gconfig-compat.h diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am index caa62f0..507e733 100644 --- a/libvirt-gconfig/Makefile.am +++ b/libvirt-gconfig/Makefile.am @@ -76,6 +76,7 @@ GCONFIG_HEADER_FILES = \ noinst_HEADERS = \ libvirt-gconfig-private.h \ libvirt-gconfig-capabilities-cpu-private.h \ + libvirt-gconfig-compat.h \ libvirt-gconfig-domain-device-private.h \ libvirt-gconfig-helpers-private.h \ libvirt-gconfig-object-private.h \ diff --git a/libvirt-gconfig/libvirt-gconfig-compat.h b/libvirt-gconfig/libvirt-gconfig-compat.h new file mode 100644 index 0000000..85a420d --- /dev/null +++ b/libvirt-gconfig/libvirt-gconfig-compat.h @@ -0,0 +1,32 @@ +/* + * libvirt-gconfig-compat.h: libvirt configuration + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Christophe Fergeau <cfergeau@redhat.com> + */ + +#ifndef __LIBVIRT_GCONFIG_COMPAT_H__ +#define __LIBVIRT_GCONFIG_COMPAT_H__ + +#include <glib-object.h> + +#if GLIB_CHECK_VERSION(2, 34, 0) +#define g_type_init() +#endif + +#endif /* __LIBVIRT_GCONFIG_COMPAT_H__ */ diff --git a/libvirt-gconfig/libvirt-gconfig-main.c b/libvirt-gconfig/libvirt-gconfig-main.c index 6fca85c..fa1963c 100644 --- a/libvirt-gconfig/libvirt-gconfig-main.c +++ b/libvirt-gconfig/libvirt-gconfig-main.c @@ -28,6 +28,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gconfig/libvirt-gconfig.h" +#include "libvirt-gconfig/libvirt-gconfig-compat.h" /** * gvirt_config_init: diff --git a/libvirt-gconfig/tests/test-domain-parse.c b/libvirt-gconfig/tests/test-domain-parse.c index 11880de..33fc7be 100644 --- a/libvirt-gconfig/tests/test-domain-parse.c +++ b/libvirt-gconfig/tests/test-domain-parse.c @@ -52,8 +52,6 @@ int main(int argc, char **argv) return 2; } - g_type_init(); - domain = gvir_config_domain_new_from_xml(xml, &error); if (error != NULL) { g_print("Couldn't parse %s: %s\n", argv[1], error->message); diff --git a/libvirt-gobject/libvirt-gobject-main.c b/libvirt-gobject/libvirt-gobject-main.c index 9dd6e3c..8544124 100644 --- a/libvirt-gobject/libvirt-gobject-main.c +++ b/libvirt-gobject/libvirt-gobject-main.c @@ -28,6 +28,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" +#include "libvirt-gconfig/libvirt-gconfig-compat.h" /** * gvir_init_object: -- 1.8.0
On Tue, Nov 13, 2012 at 6:30 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
This function call is deprecated and calling it causes a compilation warning. --- libvirt-gconfig/Makefile.am | 1 + libvirt-gconfig/libvirt-gconfig-compat.h | 32 +++++++++++++++++++++++++++++++ libvirt-gconfig/libvirt-gconfig-main.c | 1 + libvirt-gconfig/tests/test-domain-parse.c | 2 -- libvirt-gobject/libvirt-gobject-main.c | 1 + 5 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 libvirt-gconfig/libvirt-gconfig-compat.h
diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am index caa62f0..507e733 100644 --- a/libvirt-gconfig/Makefile.am +++ b/libvirt-gconfig/Makefile.am @@ -76,6 +76,7 @@ GCONFIG_HEADER_FILES = \ noinst_HEADERS = \ libvirt-gconfig-private.h \ libvirt-gconfig-capabilities-cpu-private.h \ + libvirt-gconfig-compat.h \ libvirt-gconfig-domain-device-private.h \ libvirt-gconfig-helpers-private.h \ libvirt-gconfig-object-private.h \ diff --git a/libvirt-gconfig/libvirt-gconfig-compat.h b/libvirt-gconfig/libvirt-gconfig-compat.h new file mode 100644 index 0000000..85a420d --- /dev/null +++ b/libvirt-gconfig/libvirt-gconfig-compat.h @@ -0,0 +1,32 @@ +/* + * libvirt-gconfig-compat.h: libvirt configuration + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Christophe Fergeau <cfergeau@redhat.com> + */ + +#ifndef __LIBVIRT_GCONFIG_COMPAT_H__ +#define __LIBVIRT_GCONFIG_COMPAT_H__ + +#include <glib-object.h> + +#if GLIB_CHECK_VERSION(2, 34, 0) +#define g_type_init() +#endif + +#endif /* __LIBVIRT_GCONFIG_COMPAT_H__ */ diff --git a/libvirt-gconfig/libvirt-gconfig-main.c b/libvirt-gconfig/libvirt-gconfig-main.c index 6fca85c..fa1963c 100644 --- a/libvirt-gconfig/libvirt-gconfig-main.c +++ b/libvirt-gconfig/libvirt-gconfig-main.c @@ -28,6 +28,7 @@
#include "libvirt-glib/libvirt-glib.h" #include "libvirt-gconfig/libvirt-gconfig.h" +#include "libvirt-gconfig/libvirt-gconfig-compat.h"
/** * gvirt_config_init: diff --git a/libvirt-gconfig/tests/test-domain-parse.c b/libvirt-gconfig/tests/test-domain-parse.c index 11880de..33fc7be 100644 --- a/libvirt-gconfig/tests/test-domain-parse.c +++ b/libvirt-gconfig/tests/test-domain-parse.c @@ -52,8 +52,6 @@ int main(int argc, char **argv) return 2; }
- g_type_init(); -
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
On Tue, Nov 13, 2012 at 06:53:48PM +0100, Zeeshan Ali (Khattak) wrote:
On Tue, Nov 13, 2012 at 6:30 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
diff --git a/libvirt-gconfig/tests/test-domain-parse.c b/libvirt-gconfig/tests/test-domain-parse.c index 11880de..33fc7be 100644 --- a/libvirt-gconfig/tests/test-domain-parse.c +++ b/libvirt-gconfig/tests/test-domain-parse.c @@ -52,8 +52,6 @@ int main(int argc, char **argv) return 2; }
- g_type_init(); -
With g_type_init defined for newer glib, we'll want to keep the g_type_init() calls, no?
I didn't mention this in the log, but test-domain-parse calls gvir_config_init which calls g_type_init() so the call to g_type_init() is redundant. I'll split that in a separate commit with an explanation. Christophe
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
On Tue, Nov 13, 2012 at 6:30 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
Without this change, gvir_storage_pool_get_volume segfaults when trying to do the hash table lookup with a NULL 'name' key.
ACK -- Regards, Zeeshan Ali (Khattak) FSF member#5124
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
On Tue, Nov 13, 2012 at 6:30 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
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().
Looks good. ACK. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
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
On Tue, Nov 13, 2012 at 6:30 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
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.
I didn't ACK this patch before because I wasn't sure if 'inline' functions is something we want to use here. Since we dont use them anywhere else in this codebase, I'd recommend you put these in the .c file as normal functions. No strong feelings either way so ACK with or without my suggestion acted on. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
--- 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
On Tue, Nov 13, 2012 at 06:30:26PM +0100, Christophe Fergeau wrote:
This series does misc leak fixes in libvirt-glib, as well as an API doc fix, and a deprecation warning fix.
Pushed all of this except 4/5, I've split the g_type_init() one into 2 separate commits. Christophe
participants (2)
-
Christophe Fergeau -
Zeeshan Ali (Khattak)