[libvirt] g_error considered harmful
Hi, g_error calls abort() when it's called, thus it should only be used for bad assertion failures. We currently are using it in some places in libvirt-gobject as if it was g_warning. This patch series gets rid of most occurrences of g_error by either replacing it with g_warning or by adding a GError ** argument to the function calling g_error. Christophe
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 4eb8eec..13a7f59 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -529,6 +529,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)); @@ -546,11 +547,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 4f62732..08ef17d 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -174,25 +174,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.5
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 f0c9ff6..e162867 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1116,7 +1116,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); @@ -1147,7 +1150,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 e68321d..3680cb8 100644 --- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c +++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c @@ -168,7 +168,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 2974bb8..dc3f55e 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -268,13 +268,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 7e6e9b0..09c01dc 100644 --- a/libvirt-gobject/libvirt-gobject-interface.c +++ b/libvirt-gobject/libvirt-gobject-interface.c @@ -158,13 +158,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 f0fd024..c0a6a2b 100644 --- a/libvirt-gobject/libvirt-gobject-network-filter.c +++ b/libvirt-gobject/libvirt-gobject-network-filter.c @@ -175,13 +175,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 847c236..36f9a00 100644 --- a/libvirt-gobject/libvirt-gobject-network.c +++ b/libvirt-gobject/libvirt-gobject-network.c @@ -173,13 +173,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 59fe84b..bf514b1 100644 --- a/libvirt-gobject/libvirt-gobject-node-device.c +++ b/libvirt-gobject/libvirt-gobject-node-device.c @@ -159,13 +159,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 13a7f59..f0b801e 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -204,13 +204,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 778844b..754af73 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.5
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 dc3f55e..691e29b 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 c0a6a2b..3760844 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 36f9a00..bfc867c 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 0c81921..8546a0c 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 f0b801e..fd0f0dd 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.5
On Mon, Jan 16, 2012 at 03:01:00PM +0100, Christophe Fergeau wrote:
Hi,
g_error calls abort() when it's called, thus it should only be used for bad assertion failures. We currently are using it in some places in libvirt-gobject as if it was g_warning. This patch series gets rid of most occurrences of g_error by either replacing it with g_warning or by adding a GError ** argument to the function calling g_error.
And I forgot to mention that the only remaining use of g_error are in gvir_init_object and gvir_init_config where it's probably acceptable to die if things didn't work as expected. Christophe
On Tue, Jan 17, 2012 at 2:05 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
And I forgot to mention that the only remaining use of g_error are in gvir_init_object and gvir_init_config where it's probably acceptable to die if things didn't work as expected.
Not really, it would be better to return an error. -- Marc-André Lureau
On Tue, Jan 17, 2012 at 3:19 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
On Tue, Jan 17, 2012 at 2:05 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
And I forgot to mention that the only remaining use of g_error are in gvir_init_object and gvir_init_config where it's probably acceptable to die if things didn't work as expected.
Not really, it would be better to return an error.
Thats something that is not supposed to happen so I don't think returning an error is wise here. GError is for *recoverable* runtime errors, nothing for reporting "oh we are totally screwed". -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Tue, Jan 17, 2012 at 02:19:47PM +0100, Marc-André Lureau wrote:
On Tue, Jan 17, 2012 at 2:05 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
And I forgot to mention that the only remaining use of g_error are in gvir_init_object and gvir_init_config where it's probably acceptable to die if things didn't work as expected.
Not really, it would be better to return an error.
Looking at what gtk+ does, it has gtk_init which is documented to abort, and gtk_init_check which apps can use when they need this. g_type_init has no _check alternative, but it does a few sanity checks at runtime using g_assert. I like the gtk+ approach of aborting in the function most commonly use as it avoids tracking down bugs caused by the _init failing but not being checked by the application developer. I think we should do the same and add _check functions if/when needed (this is me being lazy) Christophe
On Tue, Jan 17, 2012 at 04:52:19PM +0100, Christophe Fergeau wrote:
Looking at what gtk+ does, it has gtk_init which is documented to abort, and gtk_init_check which apps can use when they need this. g_type_init has no _check alternative, but it does a few sanity checks at runtime using g_assert.
I like the gtk+ approach of aborting in the function most commonly use as it avoids tracking down bugs caused by the _init failing but not being checked by the application developer. I think we should do the same and add _check functions if/when needed (this is me being lazy)
Actually these _init_check functions already exist, so we have libvirt_{glib,gconfig,gobject}_init which abort, and libvirt_{glib,gconfig,gobject}_init_check which return FALSE and an error when there's a problem during initilization. This mail should be read as a "ping?" on this series :) Christophe
On Mon, Jan 30, 2012 at 07:05:51PM +0100, Marc-André Lureau wrote:
On Mon, Jan 30, 2012 at 6:52 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
This mail should be read as a "ping?" on this series :)
/me lost the series, you didn't use the same title or messageid/replyto?
Hmm, I did and it was properly threaded here. Though mailing list have this "do not duplicate messages sent both to me and the mailing list" feature enabled by default, so maybe that's what broke the threading here? Anyway, here is a link to the patches http://www.redhat.com/archives/libvir-list/2012-January/msg00593.html Christophe
Hi In general I agree with the patch series dropping g_error() in favour of normal GError reporting, so programs can cope with errors. However, it removes the forced logging and it's too easy for the caller to ignore them, making it hard to track down when something goes wrong. I think this is even more relevant, because libvirt-glib is logging *tons* of normal/useless runtime messages, and now we are making silent the error messages. I would strongly prefer the other way around. On Mon, Jan 30, 2012 at 7:11 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Mon, Jan 30, 2012 at 07:05:51PM +0100, Marc-André Lureau wrote:
On Mon, Jan 30, 2012 at 6:52 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
This mail should be read as a "ping?" on this series :)
/me lost the series, you didn't use the same title or messageid/replyto?
Hmm, I did and it was properly threaded here. Though mailing list have this "do not duplicate messages sent both to me and the mailing list" feature enabled by default, so maybe that's what broke the threading here? Anyway, here is a link to the patches http://www.redhat.com/archives/libvir-list/2012-January/msg00593.html
Christophe
-- Marc-André Lureau
On Mon, Jan 30, 2012 at 7:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
Hi
In general I agree with the patch series dropping g_error() in favour of normal GError reporting, so programs can cope with errors.
However, it removes the forced logging and it's too easy for the caller to ignore them, making it hard to track down when something goes wrong.
I think this is even more relevant, because libvirt-glib is logging *tons* of normal/useless runtime messages, and now we are making silent the error messages. I would strongly prefer the other way around.
I think one way of solving that issue would be to use g_warning (withing libvirt-glib domains) when calling gvir_error_new_literal(). For the rest of the messages, I will eventually send patches to remove/lower the one I am unhappy with that really clutter a debugging session. -- Marc-André Lureau
On Mon, Jan 30, 2012 at 07:22:54PM +0100, Marc-André Lureau wrote:
Hi
In general I agree with the patch series dropping g_error() in favour of normal GError reporting, so programs can cope with errors.
However, it removes the forced logging and it's too easy for the caller to ignore them, making it hard to track down when something goes wrong.
I added a patch to the series to add a g_warn_if_reached when this is triggered
I think this is even more relevant, because libvirt-glib is logging *tons* of normal/useless runtime messages
To be honest I didn't really notice that... What kind of useless messages are you getting ? Christophe
participants (3)
-
Christophe Fergeau -
Marc-André Lureau -
Zeeshan Ali (Khattak)