8:01 a.m.
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
8:01 a.m.
New subject: [libvirt] [libvirt-glib 1/3] 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 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
8:01 a.m.
New subject: [libvirt] [libvirt-glib 2/3] Don't use g_error in xxx_get_name
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
8:01 a.m.
New subject: [libvirt] [libvirt-glib 3/3] Replace g_error with g_warning in constructors
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
7:05 a.m.
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
7:19 a.m.
On Tue, Jan 17, 2012 at 2:05 PM, Christophe Fergeau <cfergeau(a)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
7:43 a.m.
On Tue, Jan 17, 2012 at 3:19 PM, Marc-André Lureau
<marcandre.lureau(a)gmail.com> wrote:
On Tue, Jan 17, 2012 at 2:05 PM, Christophe Fergeau
<cfergeau(a)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
9:52 a.m.
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(a)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
11:52 a.m.
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
12:11 p.m.
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(a)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
12:22 p.m.
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(a)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(a)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
12:25 p.m.
On Mon, Jan 30, 2012 at 7:22 PM, Marc-André Lureau
<marcandre.lureau(a)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
5:14 a.m.
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
4682
days inactive
4697
days old
13 comments
3 participants
participants (3)
-
Christophe Fergeau -
Marc-André Lureau -
Zeeshan Ali (Khattak)