[libvirt] [[libvirt-glib PATCHv2] 1/4] Add proper error reporting to GVirStorageVol getters
gvir_storage_vol_get_name and gvir_storage_vol_get_path 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-pool.c | 10 +++++++--- libvirt-gobject/libvirt-gobject-storage-vol.c | 14 ++++++++++---- libvirt-gobject/libvirt-gobject-storage-vol.h | 4 ++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index bf25641..a88699e 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -528,6 +528,7 @@ GVirStorageVol *gvir_storage_pool_create_volume const gchar *xml; virStorageVolPtr handle; GVirStoragePoolPrivate *priv = pool->priv; + const char *name; xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf)); @@ -545,11 +546,14 @@ GVirStorageVol *gvir_storage_pool_create_volume volume = GVIR_STORAGE_VOL(g_object_new(GVIR_TYPE_STORAGE_VOL, "handle", handle, NULL)); + name = gvir_storage_vol_get_name(volume, err); + 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 5b18877..c427e42 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -172,25 +172,31 @@ gvir_storage_vol_info_free(GVirStorageVolInfo *info) G_DEFINE_BOXED_TYPE(GVirStorageVolInfo, gvir_storage_vol_info, gvir_storage_vol_info_copy, gvir_storage_vol_info_free) -const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol) +const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol, GError **error) { GVirStorageVolPrivate *priv = vol->priv; const char *name; if (!(name = virStorageVolGetName(priv->handle))) { - g_error("Failed to get storage_vol name on %p", priv->handle); + gvir_set_error(error, GVIR_STORAGE_VOL_ERROR, 0, + "Failed to get storage_vol name on %p", + priv->handle); + return NULL; } 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..fd2be80 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.h +++ b/libvirt-gobject/libvirt-gobject-storage-vol.h @@ -77,8 +77,8 @@ GType gvir_storage_vol_get_type(void); 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_name(GVirStorageVol *vol, GError **error); +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 adds a GError ** parameter to all _get_name methods which were calling g_error. --- libvirt-gobject/libvirt-gobject-connection.c | 10 ++++++++-- libvirt-gobject/libvirt-gobject-domain-snapshot.c | 5 ++++- libvirt-gobject/libvirt-gobject-domain.c | 7 +++++-- libvirt-gobject/libvirt-gobject-domain.h | 2 +- libvirt-gobject/libvirt-gobject-interface.c | 7 +++++-- libvirt-gobject/libvirt-gobject-interface.h | 2 +- libvirt-gobject/libvirt-gobject-network-filter.c | 8 ++++++-- libvirt-gobject/libvirt-gobject-network-filter.h | 2 +- libvirt-gobject/libvirt-gobject-network.c | 7 +++++-- libvirt-gobject/libvirt-gobject-network.h | 2 +- libvirt-gobject/libvirt-gobject-node-device.c | 7 +++++-- libvirt-gobject/libvirt-gobject-node-device.h | 2 +- libvirt-gobject/libvirt-gobject-storage-pool.c | 7 +++++-- libvirt-gobject/libvirt-gobject-storage-pool.h | 2 +- 14 files changed, 49 insertions(+), 21 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index a90581a..c19e411 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1112,7 +1112,10 @@ GVirDomain *gvir_connection_find_domain_by_name(GVirConnection *conn, while (g_hash_table_iter_next(&iter, &key, &value)) { GVirDomain *dom = value; - const gchar *thisname = gvir_domain_get_name(dom); + const gchar *thisname = gvir_domain_get_name(dom, NULL); + + if (thisname == NULL) + continue; if (strcmp(thisname, name) == 0) { g_object_ref(dom); @@ -1143,7 +1146,10 @@ GVirStoragePool *gvir_connection_find_storage_pool_by_name(GVirConnection *conn, while (g_hash_table_iter_next(&iter, &key, &value)) { GVirStoragePool *pool = value; - const gchar *thisname = gvir_storage_pool_get_name(pool); + const gchar *thisname = gvir_storage_pool_get_name(pool, NULL); + + if (thisname == NULL) + continue; if (strcmp(thisname, name) == 0) { g_object_ref(pool); diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.c b/libvirt-gobject/libvirt-gobject-domain-snapshot.c index 950555a..0560595 100644 --- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c +++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c @@ -166,7 +166,10 @@ 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); + gvir_set_error(err, GVIR_DOMAIN_SNAPSHOT_ERROR, 0, + "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..bfbb80f 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -264,13 +264,16 @@ G_DEFINE_BOXED_TYPE(GVirDomainInfo, gvir_domain_info, gvir_domain_info_copy, gvir_domain_info_free) -const gchar *gvir_domain_get_name(GVirDomain *dom) +const gchar *gvir_domain_get_name(GVirDomain *dom, GError **err) { GVirDomainPrivate *priv = dom->priv; const char *name; if (!(name = virDomainGetName(priv->handle))) { - g_error("Failed to get domain name on %p", priv->handle); + gvir_set_error(err, GVIR_DOMAIN_ERROR, 0, + "Failed to get domain name on %p", + priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-domain.h b/libvirt-gobject/libvirt-gobject-domain.h index 20388f2..d912b21 100644 --- a/libvirt-gobject/libvirt-gobject-domain.h +++ b/libvirt-gobject/libvirt-gobject-domain.h @@ -102,7 +102,7 @@ GType gvir_domain_get_type(void); GType gvir_domain_info_get_type(void); GType gvir_domain_handle_get_type(void); -const gchar *gvir_domain_get_name(GVirDomain *dom); +const gchar *gvir_domain_get_name(GVirDomain *dom, GError **error); const gchar *gvir_domain_get_uuid(GVirDomain *dom); gint gvir_domain_get_id(GVirDomain *dom, GError **err); diff --git a/libvirt-gobject/libvirt-gobject-interface.c b/libvirt-gobject/libvirt-gobject-interface.c index f7bdde8..095e22a 100644 --- a/libvirt-gobject/libvirt-gobject-interface.c +++ b/libvirt-gobject/libvirt-gobject-interface.c @@ -157,13 +157,16 @@ gvir_interface_handle_free(GVirInterfaceHandle *src) G_DEFINE_BOXED_TYPE(GVirInterfaceHandle, gvir_interface_handle, gvir_interface_handle_copy, gvir_interface_handle_free) -const gchar *gvir_interface_get_name(GVirInterface *iface) +const gchar *gvir_interface_get_name(GVirInterface *iface, GError **err) { GVirInterfacePrivate *priv = iface->priv; const char *name; if (!(name = virInterfaceGetName(priv->handle))) { - g_error("Failed to get interface name on %p", priv->handle); + gvir_set_error(err, GVIR_INTERFACE_ERROR, 0, + "Failed to get interface name on %p", + priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-interface.h b/libvirt-gobject/libvirt-gobject-interface.h index e4b302b..5831cfa 100644 --- a/libvirt-gobject/libvirt-gobject-interface.h +++ b/libvirt-gobject/libvirt-gobject-interface.h @@ -62,7 +62,7 @@ struct _GVirInterfaceClass GType gvir_interface_get_type(void); GType gvir_interface_handle_get_type(void); -const gchar *gvir_interface_get_name(GVirInterface *iface); +const gchar *gvir_interface_get_name(GVirInterface *iface, GError **err); GVirConfigInterface *gvir_interface_get_config(GVirInterface *iface, guint flags, diff --git a/libvirt-gobject/libvirt-gobject-network-filter.c b/libvirt-gobject/libvirt-gobject-network-filter.c index fe1a042..473ef5d 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.c +++ b/libvirt-gobject/libvirt-gobject-network-filter.c @@ -173,13 +173,17 @@ gvir_network_filter_handle_free(GVirNetworkFilterHandle *src) G_DEFINE_BOXED_TYPE(GVirNetworkFilterHandle, gvir_network_filter_handle, gvir_network_filter_handle_copy, gvir_network_filter_handle_free) -const gchar *gvir_network_filter_get_name(GVirNetworkFilter *filter) +const gchar *gvir_network_filter_get_name(GVirNetworkFilter *filter, + GError **err) { GVirNetworkFilterPrivate *priv = filter->priv; const char *name; if (!(name = virNWFilterGetName(priv->handle))) { - g_error("Failed to get network_filter name on %p", priv->handle); + gvir_set_error(err, GVIR_NETWORK_FILTER_ERROR, 0, + "Failed to get network_filter name on %p", + priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-network-filter.h b/libvirt-gobject/libvirt-gobject-network-filter.h index 24defdc..5465b0e 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.h +++ b/libvirt-gobject/libvirt-gobject-network-filter.h @@ -61,7 +61,7 @@ struct _GVirNetworkFilterClass GType gvir_network_filter_get_type(void); GType gvir_network_filter_handle_get_type(void); -const gchar *gvir_network_filter_get_name(GVirNetworkFilter *filter); +const gchar *gvir_network_filter_get_name(GVirNetworkFilter *filter, GError **err); const gchar *gvir_network_filter_get_uuid(GVirNetworkFilter *filter); GVirConfigNetworkFilter *gvir_network_filter_get_config diff --git a/libvirt-gobject/libvirt-gobject-network.c b/libvirt-gobject/libvirt-gobject-network.c index 75e010d..143e80b 100644 --- a/libvirt-gobject/libvirt-gobject-network.c +++ b/libvirt-gobject/libvirt-gobject-network.c @@ -171,13 +171,16 @@ gvir_network_handle_free(GVirNetworkHandle *src) G_DEFINE_BOXED_TYPE(GVirNetworkHandle, gvir_network_handle, gvir_network_handle_copy, gvir_network_handle_free) -const gchar *gvir_network_get_name(GVirNetwork *network) +const gchar *gvir_network_get_name(GVirNetwork *network, GError **err) { GVirNetworkPrivate *priv = network->priv; const char *name; if (!(name = virNetworkGetName(priv->handle))) { - g_error("Failed to get network name on %p", priv->handle); + gvir_set_error(err, GVIR_NETWORK_ERROR, 0, + "Failed to get network name on %p", + priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-network.h b/libvirt-gobject/libvirt-gobject-network.h index f85aed6..a6ffba9 100644 --- a/libvirt-gobject/libvirt-gobject-network.h +++ b/libvirt-gobject/libvirt-gobject-network.h @@ -65,7 +65,7 @@ struct _GVirNetworkClass GType gvir_network_get_type(void); GType gvir_network_handle_get_type(void); -const gchar *gvir_network_get_name(GVirNetwork *network); +const gchar *gvir_network_get_name(GVirNetwork *network, GError **err); const gchar *gvir_network_get_uuid(GVirNetwork *network); GVirConfigNetwork *gvir_network_get_config(GVirNetwork *network, diff --git a/libvirt-gobject/libvirt-gobject-node-device.c b/libvirt-gobject/libvirt-gobject-node-device.c index f8d536e..55f7098 100644 --- a/libvirt-gobject/libvirt-gobject-node-device.c +++ b/libvirt-gobject/libvirt-gobject-node-device.c @@ -157,13 +157,16 @@ gvir_node_device_handle_free(GVirNodeDeviceHandle *src) G_DEFINE_BOXED_TYPE(GVirNodeDeviceHandle, gvir_node_device_handle, gvir_node_device_handle_copy, gvir_node_device_handle_free) -const gchar *gvir_node_device_get_name(GVirNodeDevice *device) +const gchar *gvir_node_device_get_name(GVirNodeDevice *device, GError **err) { GVirNodeDevicePrivate *priv = device->priv; const char *name; if (!(name = virNodeDeviceGetName(priv->handle))) { - g_error("Failed to get node_device name on %p", priv->handle); + gvir_set_error(err, GVIR_NODE_DEVICE_ERROR, 0, + "Failed to get node_device name on %p", + priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-node-device.h b/libvirt-gobject/libvirt-gobject-node-device.h index 84a4234..3b57f99 100644 --- a/libvirt-gobject/libvirt-gobject-node-device.h +++ b/libvirt-gobject/libvirt-gobject-node-device.h @@ -61,7 +61,7 @@ struct _GVirNodeDeviceClass GType gvir_node_device_get_type(void); GType gvir_node_device_handle_get_type(void); -const gchar *gvir_node_device_get_name(GVirNodeDevice *device); +const gchar *gvir_node_device_get_name(GVirNodeDevice *device, GError **err); GVirConfigNodeDevice *gvir_node_device_get_config(GVirNodeDevice *device, guint flags, diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index a88699e..d2ee6d6 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -203,13 +203,16 @@ gvir_storage_pool_info_free(GVirStoragePoolInfo *info) G_DEFINE_BOXED_TYPE(GVirStoragePoolInfo, gvir_storage_pool_info, gvir_storage_pool_info_copy, gvir_storage_pool_info_free) -const gchar *gvir_storage_pool_get_name(GVirStoragePool *pool) +const gchar *gvir_storage_pool_get_name(GVirStoragePool *pool, GError **err) { GVirStoragePoolPrivate *priv = pool->priv; const char *name; if (!(name = virStoragePoolGetName(priv->handle))) { - g_error("Failed to get storage_pool name on %p", priv->handle); + gvir_set_error(err, GVIR_STORAGE_POOL_ERROR, 0, + "Failed to get storage_pool name on %p", + priv->handle); + return NULL; } return name; diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.h b/libvirt-gobject/libvirt-gobject-storage-pool.h index 4589844..78dd987 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.h +++ b/libvirt-gobject/libvirt-gobject-storage-pool.h @@ -80,7 +80,7 @@ GType gvir_storage_pool_get_type(void); GType gvir_storage_pool_info_get_type(void); GType gvir_storage_pool_handle_get_type(void); -const gchar *gvir_storage_pool_get_name(GVirStoragePool *pool); +const gchar *gvir_storage_pool_get_name(GVirStoragePool *pool, GError **err); const gchar *gvir_storage_pool_get_uuid(GVirStoragePool *pool); GVirConfigStoragePool *gvir_storage_pool_get_config(GVirStoragePool *pool, -- 1.7.7.6
On Tue, Jan 31, 2012 at 12:00:21PM +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 adds a GError ** parameter to all _get_name methods which were calling g_error. --- libvirt-gobject/libvirt-gobject-connection.c | 10 ++++++++-- libvirt-gobject/libvirt-gobject-domain-snapshot.c | 5 ++++- libvirt-gobject/libvirt-gobject-domain.c | 7 +++++-- libvirt-gobject/libvirt-gobject-domain.h | 2 +- libvirt-gobject/libvirt-gobject-interface.c | 7 +++++-- libvirt-gobject/libvirt-gobject-interface.h | 2 +- libvirt-gobject/libvirt-gobject-network-filter.c | 8 ++++++-- libvirt-gobject/libvirt-gobject-network-filter.h | 2 +- libvirt-gobject/libvirt-gobject-network.c | 7 +++++-- libvirt-gobject/libvirt-gobject-network.h | 2 +- libvirt-gobject/libvirt-gobject-node-device.c | 7 +++++-- libvirt-gobject/libvirt-gobject-node-device.h | 2 +- libvirt-gobject/libvirt-gobject-storage-pool.c | 7 +++++-- libvirt-gobject/libvirt-gobject-storage-pool.h | 2 +- 14 files changed, 49 insertions(+), 21 deletions(-)
I think that you can just replace g_error with g_warning. The vir*GetName() methods will never fail unless passed an invalid pointer, since they just return a const string from the struct, avoiding any RPC 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 :|
*_get_name failures used to trigger a crash of the application because of the use of g_error. Now that it's removed, such failures may go unnoticed and unchecked. Add a runtime warning when a failure occurs. --- libvirt-gobject/libvirt-gobject-domain-snapshot.c | 1 + libvirt-gobject/libvirt-gobject-domain.c | 1 + libvirt-gobject/libvirt-gobject-interface.c | 1 + libvirt-gobject/libvirt-gobject-network-filter.c | 1 + libvirt-gobject/libvirt-gobject-network.c | 1 + libvirt-gobject/libvirt-gobject-node-device.c | 1 + libvirt-gobject/libvirt-gobject-storage-pool.c | 1 + libvirt-gobject/libvirt-gobject-storage-vol.c | 2 ++ 8 files changed, 9 insertions(+), 0 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.c b/libvirt-gobject/libvirt-gobject-domain-snapshot.c index 0560595..3784b60 100644 --- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c +++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c @@ -169,6 +169,7 @@ const gchar *gvir_domain_snapshot_get_name(GVirDomainSnapshot *snapshot) gvir_set_error(err, GVIR_DOMAIN_SNAPSHOT_ERROR, 0, "Failed to get domain_snapshot name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index bfbb80f..0f64833 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -273,6 +273,7 @@ const gchar *gvir_domain_get_name(GVirDomain *dom, GError **err) gvir_set_error(err, GVIR_DOMAIN_ERROR, 0, "Failed to get domain name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-interface.c b/libvirt-gobject/libvirt-gobject-interface.c index 095e22a..83d4136 100644 --- a/libvirt-gobject/libvirt-gobject-interface.c +++ b/libvirt-gobject/libvirt-gobject-interface.c @@ -166,6 +166,7 @@ const gchar *gvir_interface_get_name(GVirInterface *iface, GError **err) gvir_set_error(err, GVIR_INTERFACE_ERROR, 0, "Failed to get interface name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-network-filter.c b/libvirt-gobject/libvirt-gobject-network-filter.c index 473ef5d..efefdf4 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.c +++ b/libvirt-gobject/libvirt-gobject-network-filter.c @@ -183,6 +183,7 @@ const gchar *gvir_network_filter_get_name(GVirNetworkFilter *filter, gvir_set_error(err, GVIR_NETWORK_FILTER_ERROR, 0, "Failed to get network_filter name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-network.c b/libvirt-gobject/libvirt-gobject-network.c index 143e80b..43c1a52 100644 --- a/libvirt-gobject/libvirt-gobject-network.c +++ b/libvirt-gobject/libvirt-gobject-network.c @@ -180,6 +180,7 @@ const gchar *gvir_network_get_name(GVirNetwork *network, GError **err) gvir_set_error(err, GVIR_NETWORK_ERROR, 0, "Failed to get network name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-node-device.c b/libvirt-gobject/libvirt-gobject-node-device.c index 55f7098..e34d37b 100644 --- a/libvirt-gobject/libvirt-gobject-node-device.c +++ b/libvirt-gobject/libvirt-gobject-node-device.c @@ -166,6 +166,7 @@ const gchar *gvir_node_device_get_name(GVirNodeDevice *device, GError **err) gvir_set_error(err, GVIR_NODE_DEVICE_ERROR, 0, "Failed to get node_device name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index d2ee6d6..ef8617f 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -212,6 +212,7 @@ const gchar *gvir_storage_pool_get_name(GVirStoragePool *pool, GError **err) gvir_set_error(err, GVIR_STORAGE_POOL_ERROR, 0, "Failed to get storage_pool name on %p", priv->handle); + g_warn_if_reached(); return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index c427e42..c27cfb7 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -181,6 +181,7 @@ const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol, GError **error) gvir_set_error(error, GVIR_STORAGE_VOL_ERROR, 0, "Failed to get storage_vol name on %p", priv->handle); + g_warn_if_reached(); return NULL; } @@ -196,6 +197,7 @@ const gchar *gvir_storage_vol_get_path(GVirStorageVol *vol, GError **error) gvir_set_error(error, GVIR_STORAGE_VOL_ERROR, 0, "Failed to get storage_vol path on %p", priv->handle); + g_warn_if_reached(); return NULL; } -- 1.7.7.6
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 | 4 +++- libvirt-gobject/libvirt-gobject-network-filter.c | 4 +++- libvirt-gobject/libvirt-gobject-network.c | 4 +++- libvirt-gobject/libvirt-gobject-secret.c | 4 +++- libvirt-gobject/libvirt-gobject-storage-pool.c | 4 +++- 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index 0f64833..e4d480a 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -134,7 +134,9 @@ 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(); + g_warning("Failed to get domain UUID on %p: %s", + priv->handle, verr->message); } } diff --git a/libvirt-gobject/libvirt-gobject-network-filter.c b/libvirt-gobject/libvirt-gobject-network-filter.c index efefdf4..eb9cc82 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.c +++ b/libvirt-gobject/libvirt-gobject-network-filter.c @@ -119,7 +119,9 @@ 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(); + g_warning("Failed to get network filter UUID on %p: %s", + priv->handle, verr->message); } } diff --git a/libvirt-gobject/libvirt-gobject-network.c b/libvirt-gobject/libvirt-gobject-network.c index 43c1a52..dcd2300 100644 --- a/libvirt-gobject/libvirt-gobject-network.c +++ b/libvirt-gobject/libvirt-gobject-network.c @@ -118,7 +118,9 @@ 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(); + g_warning("Failed to get network UUID on %p: %s", + priv->handle, verr->message); } } diff --git a/libvirt-gobject/libvirt-gobject-secret.c b/libvirt-gobject/libvirt-gobject-secret.c index bc5ee3b..cd819a5 100644 --- a/libvirt-gobject/libvirt-gobject-secret.c +++ b/libvirt-gobject/libvirt-gobject-secret.c @@ -119,7 +119,9 @@ 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(); + g_warning("Failed to get secret UUID on %p: %s", + priv->handle, verr->message); } } diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index ef8617f..ffb8b26 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -129,7 +129,9 @@ 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(); + g_warning("Failed to get storage pool UUID on %p: %s", + priv->handle, verr->message); } } -- 1.7.7.6
On Tue, Jan 31, 2012 at 12:00:23PM +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 | 4 +++- libvirt-gobject/libvirt-gobject-network-filter.c | 4 +++- libvirt-gobject/libvirt-gobject-network.c | 4 +++- libvirt-gobject/libvirt-gobject-secret.c | 4 +++- libvirt-gobject/libvirt-gobject-storage-pool.c | 4 +++- 5 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index 0f64833..e4d480a 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -134,7 +134,9 @@ 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(); + g_warning("Failed to get domain UUID on %p: %s", + priv->handle, verr->message); } }
This is what I think we should be doing for GetName() too, since it has the same error scenarios as GetUUID*(). NB, for extra paranoia you shouldn't assume verr is non-NULL. eg, verr ? verr->message : NULL 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 12:00:20PM +0100, Christophe Fergeau wrote:
gvir_storage_vol_get_name and gvir_storage_vol_get_path 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-pool.c | 10 +++++++--- libvirt-gobject/libvirt-gobject-storage-vol.c | 14 ++++++++++---- libvirt-gobject/libvirt-gobject-storage-vol.h | 4 ++-- 3 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index bf25641..a88699e 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -528,6 +528,7 @@ GVirStorageVol *gvir_storage_pool_create_volume const gchar *xml; virStorageVolPtr handle; GVirStoragePoolPrivate *priv = pool->priv; + const char *name;
xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
@@ -545,11 +546,14 @@ GVirStorageVol *gvir_storage_pool_create_volume volume = GVIR_STORAGE_VOL(g_object_new(GVIR_TYPE_STORAGE_VOL, "handle", handle, NULL)); + name = gvir_storage_vol_get_name(volume, err); + 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 5b18877..c427e42 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -172,25 +172,31 @@ gvir_storage_vol_info_free(GVirStorageVolInfo *info) G_DEFINE_BOXED_TYPE(GVirStorageVolInfo, gvir_storage_vol_info, gvir_storage_vol_info_copy, gvir_storage_vol_info_free)
-const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol) +const gchar *gvir_storage_vol_get_name(GVirStorageVol *vol, GError **error) { GVirStorageVolPrivate *priv = vol->priv; const char *name;
if (!(name = virStorageVolGetName(priv->handle))) { - g_error("Failed to get storage_vol name on %p", priv->handle); + gvir_set_error(error, GVIR_STORAGE_VOL_ERROR, 0, + "Failed to get storage_vol name on %p", + priv->handle); + return NULL; }
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..fd2be80 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.h +++ b/libvirt-gobject/libvirt-gobject-storage-vol.h @@ -77,8 +77,8 @@ GType gvir_storage_vol_get_type(void); 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_name(GVirStorageVol *vol, GError **error); +const gchar *gvir_storage_vol_get_path(GVirStorageVol *vol, GError **error);
gboolean gvir_storage_vol_delete(GVirStorageVol *vol, guint flags,
ACK for the change to get_path, but we don't need todo this for get_name. The virStorageVolGetName() method will always succeed, unless passed an invalid pointer, since it is not involving any RPC. It merely returns the static string in the virStorageVolPtr struct. 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