[RFC obexd v0 0/2] D-Bus error from GError

[RFC obexd v0 0/2] D-Bus error from GError

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

It is common that a D-Bus error message must be create from a GError object. Often this also involves freeing the GError's memory, which leads to boilerplate code. This RFC proposes a helper function in gdbus in order to remove this code.

The proposed name is g_dbus_steal_from_gerror(), which creates a DBusMessage out of a GError object, automatically frees the memory, and sets the pointer to NULL.

This removes 75 error-handling lines from the obex-client codebase, as included in patch 2/2. There are some other occurrences in the server side and also in BlueZ, but these have not been included here.

As returning GError objects starts to be more common in internal APIs (as planned for obex-client), this helper function would be even more convenient.

Mikel Astiz (2):
  gdbus: Add helper function for error-handling
  client: Use new D-Bus helper function

 client/ftp.c     |   85 +++++++++++++++--------------------------------------
 client/manager.c |   12 ++-----
 client/map.c     |   31 ++++++--------------
 client/pbap.c    |   29 +++++-------------
 client/sync.c    |   20 ++++---------
 gdbus/gdbus.h    |    4 ++
 gdbus/object.c   |   11 +++++++
 7 files changed, 66 insertions(+), 126 deletions(-)

-- 
1.7.7.6

[RFC obexd v0 1/2] gdbus: Add helper function for error-handling

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The new function is a convenient way to create a D-Bus error message and
automatically free the GError object.
---
 gdbus/gdbus.h  |    4 ++++
 gdbus/object.c |   11 +++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index a0583e6..485ee01 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -138,6 +138,10 @@ DBusMessage *g_dbus_create_error(DBusMessage *message, const char *name,
 					__attribute__((format(printf, 3, 4)));
 DBusMessage *g_dbus_create_error_valist(DBusMessage *message, const char *name,
 					const char *format, va_list args);
+
+DBusMessage *g_dbus_steal_from_gerror(DBusMessage *message, const char *name,
+						GError **err);
+
 DBusMessage *g_dbus_create_reply(DBusMessage *message, int type, ...);
 DBusMessage *g_dbus_create_reply_valist(DBusMessage *message,
 						int type, va_list args);
diff --git a/gdbus/object.c b/gdbus/object.c
index 8bc12f5..c6e5550 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -765,6 +765,17 @@ DBusMessage *g_dbus_create_error(DBusMessage *message, const char *name,
 	return reply;
 }
 
+DBusMessage *g_dbus_steal_from_gerror(DBusMessage *message, const char *name,
+								GError **err)
+{
+	DBusMessage *reply;
+
+	reply = g_dbus_create_error(message, name, "%s", (*err)->message);
+	g_clear_error(err);
+
+	return reply;
+}
+
 DBusMessage *g_dbus_create_reply_valist(DBusMessage *message,
 						int type, va_list args)
 {
-- 
1.7.7.6

[RFC obexd v0 2/2] client: Use new D-Bus helper function

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This refactoring makes use of the recently introduced helper function to
remove boilerplate error-handling code.
---
 client/ftp.c     |   85 +++++++++++++++--------------------------------------
 client/manager.c |   12 ++-----
 client/map.c     |   31 ++++++--------------
 client/pbap.c    |   29 +++++-------------
 client/sync.c    |   20 ++++---------
 5 files changed, 51 insertions(+), 126 deletions(-)

diff --git a/client/ftp.c b/client/ftp.c
index 0e6af47..7607107 100644
--- a/client/ftp.c
+++ b/client/ftp.c
@@ -80,14 +80,9 @@ static DBusMessage *change_folder(DBusConnection *connection,
 				"org.openobex.Error.InvalidArguments", NULL);
 
 	obc_session_setpath(session, folder, async_cb, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply;
-		reply =  g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
@@ -236,14 +231,9 @@ static DBusMessage *create_folder(DBusConnection *connection,
 				"org.openobex.Error.InvalidArguments", NULL);
 
 	obc_session_mkdir(session, folder, async_cb, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply;
-		reply = g_dbus_create_error(message,
-				"org.openobex.Error.Failed",
-				"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
@@ -259,13 +249,9 @@ static DBusMessage *list_folder(DBusConnection *connection,
 
 	obc_session_get(session, "x-obex/folder-listing", NULL, NULL,
 				NULL, 0, list_folder_callback, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
@@ -289,13 +275,9 @@ static DBusMessage *get_file(DBusConnection *connection,
 
 	obc_session_get(session, NULL, source_file, target_file, NULL, 0,
 					get_file_callback, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
@@ -319,13 +301,9 @@ static DBusMessage *put_file(DBusConnection *connection,
 				"Invalid arguments in method call");
 
 	obc_session_send(session, sourcefile, targetfile, &err);
-	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	return dbus_message_new_method_return(message);
 }
@@ -346,14 +324,9 @@ static DBusMessage *copy_file(DBusConnection *connection,
 				"org.openobex.Error.InvalidArguments", NULL);
 
 	obc_session_copy(session, filename, destname, async_cb, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply;
-		reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
@@ -376,14 +349,9 @@ static DBusMessage *move_file(DBusConnection *connection,
 				"org.openobex.Error.InvalidArguments", NULL);
 
 	obc_session_move(session, filename, destname, async_cb, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply;
-		reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
@@ -405,14 +373,9 @@ static DBusMessage *delete(DBusConnection *connection,
 				"org.openobex.Error.InvalidArguments", NULL);
 
 	obc_session_delete(session, file, async_cb, message, &err);
-	if (err != NULL) {
-		DBusMessage *reply;
-		reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 
 	dbus_message_ref(message);
 
diff --git a/client/manager.c b/client/manager.c
index 6d08702..1447a09 100644
--- a/client/manager.c
+++ b/client/manager.c
@@ -287,10 +287,8 @@ static void pull_obc_session_callback(struct obc_session *session,
 	obc_session_pull(session, "text/x-vcard", data->filename,
 					pull_complete_callback, data, &gerr);
 	if (gerr != NULL) {
-		reply = g_dbus_create_error(data->message,
-						"org.openobex.Error.Failed",
-						"%s", gerr->message);
-		g_error_free(gerr);
+		reply = g_dbus_steal_from_gerror(data->message,
+					"org.openobex.Error.Failed", &gerr);
 		goto fail;
 	}
 
@@ -504,10 +502,8 @@ static void capability_obc_session_callback(struct obc_session *session,
 	obc_session_pull(session, "x-obex/capability", NULL,
 				capabilities_complete_callback, data, &gerr);
 	if (gerr != NULL) {
-		reply = g_dbus_create_error(data->message,
-					"org.openobex.Error.Failed",
-					"%s", gerr->message);
-		g_error_free(gerr);
+		reply = g_dbus_steal_from_gerror(data->message,
+					"org.openobex.Error.Failed", &gerr);
 		goto fail;
 	}
 
diff --git a/client/map.c b/client/map.c
index 1b4e404..4be73e4 100644
--- a/client/map.c
+++ b/client/map.c
@@ -80,14 +80,9 @@ static DBusMessage *map_setpath(DBusConnection *connection,
 					NULL);
 
 	obc_session_setpath(map->session, folder, simple_cb, map, &err);
-	if (err != NULL) {
-		DBusMessage *reply;
-		reply =  g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					"org.openobex.Error.Failed", &err);
 
 	map->msg = dbus_message_ref(message);
 
@@ -137,13 +132,9 @@ static DBusMessage *map_get_folder_listing(DBusConnection *connection,
 	obc_session_get(map->session, "x-obex/folder-listing", NULL,
 							NULL, NULL, 0,
 							buffer_cb, map, &err);
-	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					"org.openobex.Error.Failed", &err);
 
 	map->msg = dbus_message_ref(message);
 
@@ -169,13 +160,9 @@ static DBusMessage *map_get_message_listing(DBusConnection *connection,
 	obc_session_get(map->session, "x-bt/MAP-msg-listing", folder,
 							NULL, NULL, 0,
 							buffer_cb, map, &err);
-	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+					"org.openobex.Error.Failed", &err);
 
 	map->msg = dbus_message_ref(message);
 
diff --git a/client/pbap.c b/client/pbap.c
index baf2ca6..ccca69c 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -491,12 +491,9 @@ static DBusMessage *pull_phonebook(struct pbap_data *pbap,
 				(guint8 *) &apparam, sizeof(apparam),
 				func, request, &err);
 	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
 		pending_request_free(request);
-		return reply;
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 	}
 
 	return NULL;
@@ -557,13 +554,9 @@ static DBusMessage *pull_vcard_listing(struct pbap_data *pbap,
 				pull_vcard_listing_callback, request, &err);
 	g_free(apparam);
 	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
 		pending_request_free(request);
-		return reply;
-
+		return g_dbus_steal_from_gerror(message,
+					 "org.openobex.Error.Failed", &err);
 	}
 
 	return NULL;
@@ -713,13 +706,10 @@ static DBusMessage *pbap_select(DBusConnection *connection,
 	obc_session_setpath(pbap->session, path, pbap_setpath_cb, request,
 									&err);
 	if (err != NULL) {
-		DBusMessage *reply;
-		reply =  g_dbus_create_error(message, ERROR_INF ".Failed",
-							"%s", err->message);
-		g_error_free(err);
 		g_free(path);
 		pending_request_free(request);
-		return reply;
+		return g_dbus_steal_from_gerror(message,
+						 ERROR_INF ".Failed", &err);
 	}
 
 	g_free(pbap->path);
@@ -781,12 +771,9 @@ static DBusMessage *pbap_pull_vcard(DBusConnection *connection,
 				(guint8 *)&apparam, sizeof(apparam),
 				pull_phonebook_callback, request, &err);
 	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						"org.openobex.Error.Failed",
-						"%s", err->message);
-		g_error_free(err);
 		pending_request_free(request);
-		return reply;
+		return g_dbus_steal_from_gerror(message,
+					"org.openobex.Error.Failed", &err);
 	}
 
 	return NULL;
diff --git a/client/sync.c b/client/sync.c
index 9a26f5b..3ef4e0a 100644
--- a/client/sync.c
+++ b/client/sync.c
@@ -139,13 +139,9 @@ static DBusMessage *sync_getphonebook(DBusConnection *connection,
 					NULL, NULL, 0,
 					sync_getphonebook_callback, sync,
 					&err);
-	if (err != 0) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						ERROR_INF ".Failed",
-						err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != 0)
+		return g_dbus_steal_from_gerror(message,
+						ERROR_INF ".Failed", &err);
 
 	sync->msg = dbus_message_ref(message);
 
@@ -171,13 +167,9 @@ static DBusMessage *sync_putphonebook(DBusConnection *connection,
 
 	obc_session_put(sync->session, buf, strlen(buf), sync->phonebook_path,
 									&err);
-	if (err != NULL) {
-		DBusMessage *reply = g_dbus_create_error(message,
-						ERROR_INF ".Failed",
-						err->message);
-		g_error_free(err);
-		return reply;
-	}
+	if (err != NULL)
+		return g_dbus_steal_from_gerror(message,
+						ERROR_INF ".Failed", &err);
 
 	return dbus_message_new_method_return(message);
 }
-- 
1.7.7.6

Re: [RFC obexd v0 1/2] gdbus: Add helper function for error-handling

[Johan Hedberg]

Hi Mikel,

On Thu, May 03, 2012, Mikel Astiz wrote:
> +DBusMessage *g_dbus_steal_from_gerror(DBusMessage *message, const char *name,
> +								GError **err)
> +{
> +	DBusMessage *reply;
> +
> +	reply = g_dbus_create_error(message, name, "%s", (*err)->message);
> +	g_clear_error(err);
> +
> +	return reply;
> +}

Did you consider following the semantics of dbus_set_error_from_message?

The calling code could then look something like:

	if (g_dbus_steal_from_gerror(&gerr, "foo.bar.baz", &reply))
		return reply;

	do other stuff since there was no error...

One thing that bothers me a bit is that you're forcing the use of a
single generic D-Bus error code for what may be a multitude of various
GError codes/domains (the only inherited part being the error message).
It'd be nice if we could somehow provide a mapping table from GError
codes/domains to D-Bus errors and only in the no-match case fall back to
a generic "org.bluez.Error.Failed".

Johan