[libvirt] [[libvirt-glib PATCHv3] 1/3] Add proper error reporting to gvir_storage_vol_get_path

gvir_storage_vol_get_name currently call g_error when an error occurs. Since g_error trigger a coredump, calling it in a library is harmful. Replace this with proper GError error reporting. --- libvirt-gobject/libvirt-gobject-storage-vol.c | 7 +++++-- libvirt-gobject/libvirt-gobject-storage-vol.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index 5b18877..5e83e1b 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -184,13 +184,16 @@ const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol) return name; } -const gchar *gvir_storage_vol_get_path(GVirStorageVol *vol) +const gchar *gvir_storage_vol_get_path(GVirStorageVol *vol, GError **error) { GVirStorageVolPrivate *priv = vol->priv; const char *path; if (!(path = virStorageVolGetPath(priv->handle))) { - g_error("Failed to get storage_vol path on %p", priv->handle); + gvir_set_error(error, GVIR_STORAGE_VOL_ERROR, 0, + "Failed to get storage_vol path on %p", + priv->handle); + return NULL; } return path; diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.h b/libvirt-gobject/libvirt-gobject-storage-vol.h index db63865..25f683a 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.h +++ b/libvirt-gobject/libvirt-gobject-storage-vol.h @@ -78,7 +78,7 @@ GType gvir_storage_vol_info_get_type(void); GType gvir_storage_vol_handle_get_type(void); const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol); -const gchar *gvir_storage_vol_get_path(GVirStorageVol *vol); +const gchar *gvir_storage_vol_get_path(GVirStorageVol *vol, GError **error); gboolean gvir_storage_vol_delete(GVirStorageVol *vol, guint flags, -- 1.7.7.6

g_error will trigger an abort of the running process so it's not desirable to call it in a library. This commit uses g_warning when this occurs and returns NULL. libvirt will only return NULL for these strings if some kind of memory corruption is going on, or if we are using an invalid pointer. --- libvirt-gobject/libvirt-gobject-connection.c | 6 ++++++ libvirt-gobject/libvirt-gobject-domain-snapshot.c | 3 ++- libvirt-gobject/libvirt-gobject-domain.c | 3 ++- libvirt-gobject/libvirt-gobject-interface.c | 3 ++- libvirt-gobject/libvirt-gobject-network-filter.c | 3 ++- libvirt-gobject/libvirt-gobject-network.c | 3 ++- libvirt-gobject/libvirt-gobject-node-device.c | 3 ++- libvirt-gobject/libvirt-gobject-storage-pool.c | 13 +++++++++---- libvirt-gobject/libvirt-gobject-storage-vol.c | 3 ++- 9 files changed, 29 insertions(+), 11 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index a90581a..cb19e9d 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1114,6 +1114,9 @@ GVirDomain *gvir_connection_find_domain_by_name(GVirConnection *conn, GVirDomain *dom = value; const gchar *thisname = gvir_domain_get_name(dom); + if (thisname == NULL) + continue; + if (strcmp(thisname, name) == 0) { g_object_ref(dom); g_mutex_unlock(priv->lock); @@ -1145,6 +1148,9 @@ GVirStoragePool *gvir_connection_find_storage_pool_by_name(GVirConnection *conn, GVirStoragePool *pool = value; const gchar *thisname = gvir_storage_pool_get_name(pool); + if (thisname == NULL) + continue; + if (strcmp(thisname, name) == 0) { g_object_ref(pool); g_mutex_unlock(priv->lock); diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.c b/libvirt-gobject/libvirt-gobject-domain-snapshot.c index 950555a..c386491 100644 --- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c +++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c @@ -166,7 +166,8 @@ const gchar *gvir_domain_snapshot_get_name(GVirDomainSnapshot *snapshot) const char *name; if (!(name = virDomainSnapshotGetName(priv->handle))) { - g_error("Failed to get domain_snapshot name on %p", priv->handle); + g_warning("Failed to get domain_snapshot name on %p", priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index c1a67a5..82da0c3 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -270,7 +270,8 @@ const gchar *gvir_domain_get_name(GVirDomain *dom) const char *name; if (!(name = virDomainGetName(priv->handle))) { - g_error("Failed to get domain name on %p", priv->handle); + g_warning("Failed to get domain name on %p", priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-interface.c b/libvirt-gobject/libvirt-gobject-interface.c index f7bdde8..892db8b 100644 --- a/libvirt-gobject/libvirt-gobject-interface.c +++ b/libvirt-gobject/libvirt-gobject-interface.c @@ -163,7 +163,8 @@ const gchar *gvir_interface_get_name(GVirInterface *iface) const char *name; if (!(name = virInterfaceGetName(priv->handle))) { - g_error("Failed to get interface name on %p", priv->handle); + g_warning("Failed to get interface name on %p", priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-network-filter.c b/libvirt-gobject/libvirt-gobject-network-filter.c index fe1a042..b1543a3 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.c +++ b/libvirt-gobject/libvirt-gobject-network-filter.c @@ -179,7 +179,8 @@ const gchar *gvir_network_filter_get_name(GVirNetworkFilter *filter) const char *name; if (!(name = virNWFilterGetName(priv->handle))) { - g_error("Failed to get network_filter name on %p", priv->handle); + g_warning("Failed to get network_filter name on %p", priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-network.c b/libvirt-gobject/libvirt-gobject-network.c index 75e010d..8cbea07 100644 --- a/libvirt-gobject/libvirt-gobject-network.c +++ b/libvirt-gobject/libvirt-gobject-network.c @@ -177,7 +177,8 @@ const gchar *gvir_network_get_name(GVirNetwork *network) const char *name; if (!(name = virNetworkGetName(priv->handle))) { - g_error("Failed to get network name on %p", priv->handle); + g_warning("Failed to get network name on %p", priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-node-device.c b/libvirt-gobject/libvirt-gobject-node-device.c index f8d536e..b565052 100644 --- a/libvirt-gobject/libvirt-gobject-node-device.c +++ b/libvirt-gobject/libvirt-gobject-node-device.c @@ -163,7 +163,8 @@ const gchar *gvir_node_device_get_name(GVirNodeDevice *device) const char *name; if (!(name = virNodeDeviceGetName(priv->handle))) { - g_error("Failed to get node_device name on %p", priv->handle); + g_warning("Failed to get node_device name on %p", priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index bf25641..aa71029 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -209,7 +209,8 @@ const gchar *gvir_storage_pool_get_name(GVirStoragePool *pool) const char *name; if (!(name = virStoragePoolGetName(priv->handle))) { - g_error("Failed to get storage_pool name on %p", priv->handle); + g_warning("Failed to get storage_pool name on %p", priv->handle); + return NULL; } return name; @@ -541,15 +542,19 @@ GVirStorageVol *gvir_storage_pool_create_volume } GVirStorageVol *volume; + char *name; volume = GVIR_STORAGE_VOL(g_object_new(GVIR_TYPE_STORAGE_VOL, "handle", handle, NULL)); + name = gvir_storage_vol_get_name(volume); + if (name == NULL) { + g_object_unref(G_OBJECT(volume)); + return NULL; + } g_mutex_lock(priv->lock); - g_hash_table_insert(priv->volumes, - g_strdup(gvir_storage_vol_get_name(volume)), - volume); + g_hash_table_insert(priv->volumes, g_strdup(name), volume); g_mutex_unlock(priv->lock); return g_object_ref(volume); diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index 5e83e1b..671d010 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -178,7 +178,8 @@ const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol) const char *name; if (!(name = virStorageVolGetName(priv->handle))) { - g_error("Failed to get storage_vol name on %p", priv->handle); + g_warning("Failed to get storage_vol name on %p", priv->handle); + return NULL; } return name; -- 1.7.7.6

On Tue, Jan 31, 2012 at 03:34:10PM +0100, Christophe Fergeau wrote:
g_error will trigger an abort of the running process so it's not desirable to call it in a library. This commit uses g_warning when this occurs and returns NULL. libvirt will only return NULL for these strings if some kind of memory corruption is going on, or if we are using an invalid pointer. --- libvirt-gobject/libvirt-gobject-connection.c | 6 ++++++ libvirt-gobject/libvirt-gobject-domain-snapshot.c | 3 ++- libvirt-gobject/libvirt-gobject-domain.c | 3 ++- libvirt-gobject/libvirt-gobject-interface.c | 3 ++- libvirt-gobject/libvirt-gobject-network-filter.c | 3 ++- libvirt-gobject/libvirt-gobject-network.c | 3 ++- libvirt-gobject/libvirt-gobject-node-device.c | 3 ++- libvirt-gobject/libvirt-gobject-storage-pool.c | 13 +++++++++---- libvirt-gobject/libvirt-gobject-storage-vol.c | 3 ++- 9 files changed, 29 insertions(+), 11 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

g_error generates a fatal error message, meaning it will abort the currently running process. It's nicer to use g_warning here. --- libvirt-gobject/libvirt-gobject-domain.c | 9 ++++++++- libvirt-gobject/libvirt-gobject-network-filter.c | 9 ++++++++- libvirt-gobject/libvirt-gobject-network.c | 9 ++++++++- libvirt-gobject/libvirt-gobject-secret.c | 8 +++++++- libvirt-gobject/libvirt-gobject-storage-pool.c | 8 +++++++- 5 files changed, 38 insertions(+), 5 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index 82da0c3..d9e4c00 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -134,7 +134,14 @@ static void gvir_domain_constructed(GObject *object) /* xxx we may want to turn this into an initable */ if (virDomainGetUUIDString(priv->handle, priv->uuid) < 0) { - g_error("Failed to get domain UUID on %p", priv->handle); + virErrorPtr verr = virGetLastError(); + if (verr) { + g_warning("Failed to get domain UUID on %p: %s", + priv->handle, verr->message); + } else { + g_warning("Failed to get domain UUID on %p", + priv->handle); + } } } diff --git a/libvirt-gobject/libvirt-gobject-network-filter.c b/libvirt-gobject/libvirt-gobject-network-filter.c index b1543a3..5956a3d 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.c +++ b/libvirt-gobject/libvirt-gobject-network-filter.c @@ -119,7 +119,14 @@ static void gvir_network_filter_constructed(GObject *object) /* xxx we may want to turn this into an initable */ if (virNWFilterGetUUIDString(priv->handle, priv->uuid) < 0) { - g_error("Failed to get network filter UUID on %p", priv->handle); + virErrorPtr verr = virGetLastError(); + if (verr) { + g_warning("Failed to get network filter UUID on %p: %s", + priv->handle, verr->message); + } else { + g_warning("Failed to get network filter UUID on %p", + priv->handle); + } } } diff --git a/libvirt-gobject/libvirt-gobject-network.c b/libvirt-gobject/libvirt-gobject-network.c index 8cbea07..16cd2f1 100644 --- a/libvirt-gobject/libvirt-gobject-network.c +++ b/libvirt-gobject/libvirt-gobject-network.c @@ -118,7 +118,14 @@ static void gvir_network_constructed(GObject *object) /* xxx we may want to turn this into an initable */ if (virNetworkGetUUIDString(priv->handle, priv->uuid) < 0) { - g_error("Failed to get network UUID on %p", priv->handle); + virErrorPtr verr = virGetLastError(); + if (verr) { + g_warning("Failed to get network UUID on %p: %s", + priv->handle, verr->message); + } else { + g_warning("Failed to get network UUID on %p", + priv->handle); + } } } diff --git a/libvirt-gobject/libvirt-gobject-secret.c b/libvirt-gobject/libvirt-gobject-secret.c index bc5ee3b..3c9da86 100644 --- a/libvirt-gobject/libvirt-gobject-secret.c +++ b/libvirt-gobject/libvirt-gobject-secret.c @@ -119,7 +119,13 @@ static void gvir_secret_constructed(GObject *object) /* xxx we may want to turn this into an initable */ if (virSecretGetUUIDString(priv->handle, priv->uuid) < 0) { - g_error("Failed to get secret UUID on %p", priv->handle); + virErrorPtr verr = virGetLastError(); + if (verr) { + g_warning("Failed to get secret UUID on %p: %s", + priv->handle, verr->message); + } else { + g_warning("Failed to get secret UUID on %p", priv->handle); + } } } diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index aa71029..db496f3 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -129,7 +129,13 @@ static void gvir_storage_pool_constructed(GObject *object) /* xxx we may want to turn this into an initable */ if (virStoragePoolGetUUIDString(priv->handle, priv->uuid) < 0) { - g_error("Failed to get storage pool UUID on %p", priv->handle); + virErrorPtr verr = virGetLastError(); + if (verr) { + g_warning("Failed to get storage pool UUID on %p: %s", + priv->handle, verr->message); + } else { + g_warning("Failed to get storage pool UUID on %p", priv->handle); + } } } -- 1.7.7.6

On Tue, Jan 31, 2012 at 03:34:11PM +0100, Christophe Fergeau wrote:
g_error generates a fatal error message, meaning it will abort the currently running process. It's nicer to use g_warning here. --- libvirt-gobject/libvirt-gobject-domain.c | 9 ++++++++- libvirt-gobject/libvirt-gobject-network-filter.c | 9 ++++++++- libvirt-gobject/libvirt-gobject-network.c | 9 ++++++++- libvirt-gobject/libvirt-gobject-secret.c | 8 +++++++- libvirt-gobject/libvirt-gobject-storage-pool.c | 8 +++++++- 5 files changed, 38 insertions(+), 5 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Tue, Jan 31, 2012 at 03:34:09PM +0100, Christophe Fergeau wrote:
gvir_storage_vol_get_name currently call g_error when an error occurs. Since g_error trigger a coredump, calling it in a library is harmful. Replace this with proper GError error reporting. --- libvirt-gobject/libvirt-gobject-storage-vol.c | 7 +++++-- libvirt-gobject/libvirt-gobject-storage-vol.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Christophe Fergeau
-
Daniel P. Berrange