bluez git HEAD doesn't put introspection methods into the introspection output

PATCH New RISKÓ Gergely 4 participants 17 messages Sep 1, 2009 – Sep 24, 2009 Bluetooth drivers

lore

Share & tools

lore

RISKÓ Gergely

author

2009-09-01 14:10 UTC #0 lore raw

PATCH 1/1 bluez git HEAD doesn't put introspection methods into the introspection output

Hi,

I have the following problem:
4252@17:04 errge@lisa:~$ qdbus --system org.bluez
/
Cannot introspect object / at org.bluez:
org.freedesktop.DBus.Error.UnknownInterface (Interface 'org.freedesktop.DBus.Introspectable' was not found)

This is caused by the fact that:
- QtDBus is buggy as hell, :)
- BlueZ doesn't include introspection API in the introspection output.

Either of the causes got solved, the whole issue is solved. After checking the code of QtDBus I decided to go with the second. See the attached patch. After applying it everything works fine:
4252@17:04 errge@lisa:~$ qdbus --system org.bluez
/
/org
/org/bluez
/org/bluez/9746
/org/bluez/9746/any
/org/bluez/9746/hci0
/org/bluez/test

If you need clarification or have comments about any of these, please don't hesitate to reply!

Thanks in advance,
Gergely

gdbus/object.c +5 -0

index 3186921..8091a95 100644
@@ -156,6 +156,11 @@ static void generate_introspection_xml(DBusConnection *conn,
 	gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
 	g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
+	g_string_append_printf(gstr, "\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n");
+	g_string_append_printf(gstr, "\t\t<method name=\"Introspect\">\n");
+	g_string_append_printf(gstr, "\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n");
+	g_string_append_printf(gstr, "\t\t</method>\n");
+	g_string_append_printf(gstr, "\t</interface>\n");
 
 	for (list = data->interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;

RISKÓ Gergely

author

2009-09-02 10:00 UTC #1 lore raw

PATCH 1/1 Add introspection interface to the output of introspection calls.

---
 gdbus/object.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

gdbus/object.c +5 -0

index 3186921..8091a95 100644
@@ -156,6 +156,11 @@ static void generate_introspection_xml(DBusConnection *conn,
 	gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
 	g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
+	g_string_append_printf(gstr, "\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n");
+	g_string_append_printf(gstr, "\t\t<method name=\"Introspect\">\n");
+	g_string_append_printf(gstr, "\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n");
+	g_string_append_printf(gstr, "\t\t</method>\n");
+	g_string_append_printf(gstr, "\t</interface>\n");
 
 	for (list = data->interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;

Signature · 4 lines

1.6.0.4

Sorry about the previous patch, now I have used git format-patch instead
of attaching the diff by hand.

Luiz Augusto von Dentz

maintainer

2009-09-02 14:46 UTC #2 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Quoted 11 lines · RISKÓ Gergely

---
gdbus/object.c | 5 +++++

 1 files changed, 5 insertions(+), 0 deletions(-)

gdbus/object.c +0 -0

index 3186921..8091a95 100644
@@ -156,6 +156,11 @@ static void generate_introspection_xml(DBusConnection *conn,
        gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);

gdbus/object.c static void generate_introspection_xml(DBusConnection *conn, · line 166

Quoted 10 lines

gdbus/object.c +5 -0

 
        g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
+       g_string_append_printf(gstr, "\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n");
+       g_string_append_printf(gstr, "\t\t<method name=\"Introspect\">\n");
+       g_string_append_printf(gstr, "\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n");
+       g_string_append_printf(gstr, "\t\t</method>\n");
+       g_string_append_printf(gstr, "\t</interface>\n");
 
        for (list = data->interfaces; list; list = list->next) {
                struct interface_data *iface = list->data;

Im afraid this is on purpose, we don't export the introspectable interface because it is implicit you can only have the introspection data if the object export it so it is meaningless to export it again on the .xml. You can check with d-feet that most services does this in the same way.

Signature · 2 lines

Luiz Augusto von Dentz
Engenheiro de Computação

RISKÓ Gergely

author

2009-09-02 17:48 UTC #3 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Quoted 22 lines · Luiz Augusto von Dentz

Hi,

Quoted 19 lines · RISKÓ Gergely

---
gdbus/object.c | 5 +++++

 1 files changed, 5 insertions(+), 0 deletions(-)

gdbus/object.c +5 -0

index 3186921..8091a95 100644
@@ -156,6 +156,11 @@ static void generate_introspection_xml(DBusConnection *conn,
        gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
        g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
+       g_string_append_printf(gstr, "\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n");
+       g_string_append_printf(gstr, "\t\t<method name=\"Introspect\">\n");
+       g_string_append_printf(gstr, "\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n");
+       g_string_append_printf(gstr, "\t\t</method>\n");
+       g_string_append_printf(gstr, "\t</interface>\n");

Quoted 8 lines

gdbus/object.c static void generate_introspection_xml(DBusConnection *conn, · line 166
Quoted 2 lines
gdbus/object.c +0 -0
        for (list = data->interfaces; list; list = list->next) {
                struct interface_data *iface = list->data;
Im afraid this is on purpose, we don't export the introspectable interface because it is implicit you can only have the introspection data if the object export it so it is meaningless to export it again on the .xml. You can check with d-feet that most services does this in the same way.

Thanks for your reply, I appreciate it, however I don't agree after I checked. Probably some time ago this was the custom, but apparently it changed.

About the other services on my home system:
22975@20:39 risko@bubble:~$ qdbus --system | grep -v '^:'
org.freedesktop.Avahi
org.freedesktop.Hal
org.bluez
org.freedesktop.ConsoleKit
org.freedesktop.DBus

22978@20:42 risko@bubble:~$ for i in qdbus --system | grep -v '^:' ; do QDBUS_DEBUG=1 qdbus --system $i 2>&1| grep -q '<interface.*Introspectable' || echo $i; done org.bluez

So Avahi, Hal, ConsoleKit and even DBus itself exposes the introspection API in the introspection API, the only exception is BlueZ.

And this issue is causing problems for QDBus users everywhere, not just for me: https://bugs.launchpad.net/ubuntu/+source/bluez/+bug/371299

The guy who handled the bug report even thought that BlueZ doesn't support introspection because of this.

I still think that this is bad, because all of the other common services behave the other way around and it is incompatible with some silly implementations (QtDBus which I know of, probably others).

If there is any other research I can do, please say so, happy to help, Gergely

Johan Hedberg

2009-09-14 14:18 UTC #4 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi Gergely,

Quoted 20 lines · RISKÓ Gergely

About the other services on my home system:
22975@20:39 risko@bubble:~$ qdbus --system | grep -v '^:'
org.freedesktop.Avahi
org.freedesktop.Hal
org.bluez
org.freedesktop.ConsoleKit
org.freedesktop.DBus
22978@20:42 risko@bubble:~$ for i in qdbus --system | grep -v '^:' ; do QDBUS_DEBUG=1 qdbus --system $i 2>&1| grep -q '<interface.*Introspectable' || echo $i; done org.bluez
So Avahi, Hal, ConsoleKit and even DBus itself exposes the introspection API in the introspection API, the only exception is BlueZ.
And this issue is causing problems for QDBus users everywhere, not just for me: https://bugs.launchpad.net/ubuntu/+source/bluez/+bug/371299
The guy who handled the bug report even thought that BlueZ doesn't support introspection because of this.

Quoted 6 lines

I still think that this is bad, because all of the other common services behave the other way around and it is incompatible with some silly implementations (QtDBus which I know of, probably others).
If there is any other research I can do, please say so, happy to help, Gergely

I agree that it would be good to have this fix in. Since there's no official guideline for this and neither solution can be said to be completely incorrect I think that we should go with whatever works best with existing D-Bus applications, i.e. have the introspection interface declared as part of the XML. Could you still do the following please:

- Since the added XML content is fixed, use just one g_string_append_printf call for it (and split the string into multiple lines). Also try to have the lines max 80 columns to adhere to the coding style.

- Create a "git am" compatible patch out of the change by commiting it to your tree with a descriptive commit message and then use git format-patch

Johan

RISKÓ Gergely

author

2009-09-14 14:52 UTC #5 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi Johan,

Quoted 1 line · Johan Hedberg

declared as part of the XML. Could you still do the following please:

Thank you for all of your advise, here is the result.

BR,
Gergely

Quoted 1 line

From 91b64b5cd3b174317bde5f9ccf4cc1d420086eb7 Mon Sep 17 00:00:00 2001

From: Gergely Risko <[email protected]>
Date: Mon, 14 Sep 2009 17:46:29 +0300
Subject: [PATCH] Add introspection interface description into the DBus introspection replies.

---
 gdbus/object.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

gdbus/object.c +7 -1

index 3186921..811c2e1 100644
@@ -155,7 +155,13 @@ static void generate_introspection_xml(DBusConnection *conn,
 
 	gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
-	g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
+	g_string_append_printf(gstr,
+		"<node name=\"%s\">\n"
+		"\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n"
+		"\t\t<method name=\"Introspect\">\n"
+		"\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n"
+		"\t\t</method>\n"
+		"\t</interface>\n", path);
 
 	for (list = data->interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;

Signature · 1 line

1.6.0.4

Johan Hedberg

2009-09-14 21:11 UTC #6 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi Gergely,

Quoted 5 lines · RISKÓ Gergely

Thank you for all of your advise, here is the result.
BR,
Gergely

Quoted 8 lines

From 91b64b5cd3b174317bde5f9ccf4cc1d420086eb7 Mon Sep 17 00:00:00 2001
From: Gergely Risko <[email protected]>
Date: Mon, 14 Sep 2009 17:46:29 +0300
Subject: [PATCH] Add introspection interface description into the DBus introspection replies.
---
 gdbus/object.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Thanks! The patch has now been pushed upstream.

One thing that came to my mind is that it might be possible to simplify the code by making the introspection interface less of a special case within gdbus:

Instead of hard coding this XML snippet and handling the Introspect method call separately from the rest of the method calls for a particular object path gdbus could use its own public interface registration system (i.e. g_dbus_register_interface) to implicitly register the Introspectable interface as the first interface when creating new object paths. This way the existing interface callback mechanism would do most of the work and there wouldn't be a need to explicitly add the extra snippet to the XML output like your patch now does.

Any thoughts on this?

Johan

Luiz Augusto von Dentz

maintainer

2009-09-15 10:50 UTC #7 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Quoted 15 lines · Johan Hedberg

Thanks! The patch has now been pushed upstream.
One thing that came to my mind is that it might be possible to simplify the code by making the introspection interface less of a special case within gdbus:
Instead of hard coding this XML snippet and handling the Introspect method call separately from the rest of the method calls for a particular object path gdbus could use its own public interface registration system (i.e. g_dbus_register_interface) to implicitly register the Introspectable interface as the first interface when creating new object paths. This way the existing interface callback mechanism would do most of the work and there wouldn't be a need to explicitly add the extra snippet to the XML output like your patch now does.

Quoted 1 line

Any thoughts on this?

Yep, that sounds better for me too.

About the qdbus tool, I remember having this issue some time ago, but it seems that any instance of QDBusInterface triggers introspect, but qdbus code add another check for Introspectable interface which seems unnecessary because it does that to call introspect again.

Signature · 2 lines

Luiz Augusto von Dentz
Engenheiro de Computação

RISKÓ Gergely

author

2009-09-15 12:28 UTC #8 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Thank you for the comments, you are both right, Introspection should be handled generally and not specially in the code. So I have done the necessary modifications and modified the handling of introspection calls in gdbus/object.c to use the generic_message infrastructure.

Comments?

Cheers,
Gergely

Quoted 1 line

From 2188640a694ba9dd14f59ed250f8a37f3a1bec34 Mon Sep 17 00:00:00 2001

From: Gergely Risko <[email protected]>
Date: Tue, 15 Sep 2009 15:23:24 +0300
Subject: [PATCH] gdbus: handle introspection generally in generic_message.

Previously it was a specific case, now introspection is just another interface, which is always implemented. It is registered/unregistered when an object path is referenced first/last.

---
 gdbus/object.c |   57 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 21 deletions(-)

gdbus/object.c +36 ~~-21~~

index 811c2e1..3b6d3f2 100644
@@ -155,13 +155,7 @@ static void generate_introspection_xml(DBusConnection *conn,
 
 	gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
-	g_string_append_printf(gstr,
-		"<node name=\"%s\">\n"
-		"\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n"
-		"\t\t<method name=\"Introspect\">\n"
-		"\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n"
-		"\t\t</method>\n"
-		"\t</interface>\n", path);
+	g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
 
 	for (list = data->interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;

@@ -189,14 +183,15 @@ done:
 	data->introspect = g_string_free(gstr, FALSE);
 }
 
-static DBusHandlerResult introspect(DBusConnection *connection,
-				DBusMessage *message, struct generic_data *data)
+static DBusMessage *introspect(DBusConnection *connection,
+				DBusMessage *message, void *data_)
 {
+	struct generic_data *data = (struct generic_data *) data_;
 	DBusMessage *reply;
 
 	if (!dbus_message_has_signature(message, DBUS_TYPE_INVALID_AS_STRING)) {
 		error("Unexpected signature to introspect call");
-		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+		return NULL;
 	}
 
 	if (!data->introspect)

@@ -205,16 +200,12 @@ static DBusHandlerResult introspect(DBusConnection *connection,
 
 	reply = dbus_message_new_method_return(message);
 	if (!reply)
-		return DBUS_HANDLER_RESULT_NEED_MEMORY;
+		return NULL;
 
 	dbus_message_append_args(reply, DBUS_TYPE_STRING, &data->introspect,
 					DBUS_TYPE_INVALID);
 
-	dbus_connection_send(connection, reply, NULL);
-
-	dbus_message_unref(reply);
-
-	return DBUS_HANDLER_RESULT_HANDLED;
+	return reply;
 }
 
 static void generic_unregister(DBusConnection *connection, void *user_data)

@@ -250,11 +241,6 @@ static DBusHandlerResult generic_message(DBusConnection *connection,
 	GDBusMethodTable *method;
 	const char *interface;
 
-	if (dbus_message_is_method_call(message,
-					DBUS_INTERFACE_INTROSPECTABLE,
-								"Introspect"))
-		return introspect(connection, message, data);
-
 	interface = dbus_message_get_interface(message);
 
 	iface = find_interface(data->interfaces, interface);

@@ -335,10 +321,16 @@ done:
 	g_free(parent_path);
 }
 
+static GDBusMethodTable introspect_methods[] = {
+	{ "Introspect",	"",	"s", introspect	},
+	{ }
+};
+
 static struct generic_data *object_path_ref(DBusConnection *connection,
 							const char *path)
 {
 	struct generic_data *data;
+	struct interface_data *iface;
 
 	if (dbus_connection_get_object_path_data(connection, path,
 						(void *) &data) == TRUE) {

@@ -363,12 +355,23 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 
 	invalidate_parent_data(connection, path);
 
+	/* Register the introspection interface */
+	iface = g_new0(struct interface_data, 1);
+	iface->name = g_strdup("org.freedesktop.DBus.Introspectable"); // TODO: macro
+	iface->methods = introspect_methods;
+	iface->signals = NULL;
+	iface->properties = NULL;
+	iface->user_data = data;
+	iface->destroy = NULL;
+	data->interfaces = g_slist_append(data->interfaces, iface);
+
 	return data;
 }
 
 static void object_path_unref(DBusConnection *connection, const char *path)
 {
 	struct generic_data *data = NULL;
+	struct interface_data *iface;
 
 	if (dbus_connection_get_object_path_data(connection, path,
 						(void *) &data) == FALSE)

@@ -382,6 +385,18 @@ static void object_path_unref(DBusConnection *connection, const char *path)
 	if (data->refcount > 0)
 		return;
 
+	/* Free the introspection interface */
+	iface = find_interface(data->interfaces, "org.freedesktop.DBus.Introspectable");
+	if (iface) {
+		data->interfaces = g_slist_remove(data->interfaces, iface);
+
+		if (iface->destroy)
+			iface->destroy(iface->user_data);
+
+		g_free(iface->name);
+		g_free(iface);
+	}
+
 	invalidate_parent_data(connection, path);
 
 	dbus_connection_unregister_object_path(connection, path);

Signature · 1 line

1.6.0.4

Quoted 26 lines · Luiz Augusto von Dentz

Hi,
Quoted 15 lines · Johan Hedberg
Thanks! The patch has now been pushed upstream.
One thing that came to my mind is that it might be possible to simplify the code by making the introspection interface less of a special case within gdbus:
Instead of hard coding this XML snippet and handling the Introspect method call separately from the rest of the method calls for a particular object path gdbus could use its own public interface registration system (i.e. g_dbus_register_interface) to implicitly register the Introspectable interface as the first interface when creating new object paths. This way the existing interface callback mechanism would do most of the work and there wouldn't be a need to explicitly add the extra snippet to the XML output like your patch now does.
Quoted 1 line
Any thoughts on this?
Yep, that sounds better for me too.
About the qdbus tool, I remember having this issue some time ago, but it seems that any instance of QDBusInterface triggers introspect, but qdbus code add another check for Introspectable interface which seems unnecessary because it does that to call introspect again.

Johan Hedberg

2009-09-15 15:22 UTC #9 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

The general idea of the patch looks good but there are a few things I'd still fix:

gdbus/object.c done: · line 187

Quoted 4 lines · RISKÓ Gergely

gdbus/object.c +2 -2

-static DBusHandlerResult introspect(DBusConnection *connection,
-				DBusMessage *message, struct generic_data *data)
+static DBusMessage *introspect(DBusConnection *connection,
+				DBusMessage *message, void *data_)

s/data_/user_data/ (to conform to the rest of the code and glib conventions)

gdbus/object.c done: · line 189

Quoted 1 line

gdbus/object.c +1 -0

+	struct generic_data *data = (struct generic_data *) data_;

Unnecessary explicit type-cast of a void pointer.

Quoted 9 lines

static struct generic_data *object_path_ref(DBusConnection *connection,

gdbus/object.c +1 -0

 							const char *path)
 {
 	struct generic_data *data;
+	struct interface_data *iface;
 
 	if (dbus_connection_get_object_path_data(connection, path,
 						(void *) &data) == TRUE) {

gdbus/object.c static struct generic_data *object_path_ref(DBusConnection *connection, · line 360

Quoted 6 lines

gdbus/object.c +3 -0

@@ -363,12 +355,23 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 
 	invalidate_parent_data(connection, path);
 
+	/* Register the introspection interface */
+	iface = g_new0(struct interface_data, 1);
+	iface->name = g_strdup("org.freedesktop.DBus.Introspectable"); // TODO: macro

Either add the macro or remove this comment. Also, we don't use C++ style commenting.

gdbus/object.c static struct generic_data *object_path_ref(DBusConnection *connection, · line 367

Quoted 7 lines

gdbus/object.c +7 -0

+	iface->methods = introspect_methods;
+	iface->signals = NULL;
+	iface->properties = NULL;
+	iface->user_data = data;
+	iface->destroy = NULL;
+	data->interfaces = g_slist_append(data->interfaces, iface);
+

Instead of this duplicate interface initialization code I think it would make sense to refactor and create a new private function, e.g

static void add_interface(struct generic_cata *data, ...

and call that from the necessary places (afaics there are two of them)

gdbus/object.c static void object_path_unref(DBusConnection *connection, const char *path) · line 397

Quoted 10 lines

gdbus/object.c +10 -0

+	/* Free the introspection interface */
+	iface = find_interface(data->interfaces, "org.freedesktop.DBus.Introspectable");
+	if (iface) {
+		data->interfaces = g_slist_remove(data->interfaces, iface);
+
+		if (iface->destroy)
+			iface->destroy(iface->user_data);
+
+		g_free(iface->name);
+		g_free(iface);

Would it make sense to refactor create a remove_interface function for this too (to balance with the add_interface function)?

Johan

Marcel Holtmann

maintainer

2009-09-15 17:27 UTC #10 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi Johan,

Quoted 50 lines

The general idea of the patch looks good but there are a few things I'd still fix:
gdbus/object.c done: · line 187
Quoted 4 lines · RISKÓ Gergely
gdbus/object.c +2 -2
-static DBusHandlerResult introspect(DBusConnection *connection,
-				DBusMessage *message, struct generic_data *data)
+static DBusMessage *introspect(DBusConnection *connection,
+				DBusMessage *message, void *data_)
s/data_/user_data/ (to conform to the rest of the code and glib conventions)
gdbus/object.c done: · line 189
Quoted 1 line
gdbus/object.c +1 -0
+	struct generic_data *data = (struct generic_data *) data_;
Unnecessary explicit type-cast of a void pointer.
Quoted 9 lines
static struct generic_data *object_path_ref(DBusConnection *connection,
gdbus/object.c +1 -0
 							const char *path)
 {
 	struct generic_data *data;
+	struct interface_data *iface;
 
 	if (dbus_connection_get_object_path_data(connection, path,
 						(void *) &data) == TRUE) {
gdbus/object.c static struct generic_data *object_path_ref(DBusConnection *connection, · line 360
Quoted 6 lines
gdbus/object.c +3 -0
@@ -363,12 +355,23 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 
 	invalidate_parent_data(connection, path);
 
+	/* Register the introspection interface */
+	iface = g_new0(struct interface_data, 1);
+	iface->name = g_strdup("org.freedesktop.DBus.Introspectable"); // TODO: macro
Either add the macro or remove this comment. Also, we don't use C++ style commenting.
gdbus/object.c static struct generic_data *object_path_ref(DBusConnection *connection, · line 367
Quoted 7 lines
gdbus/object.c +7 -0
+	iface->methods = introspect_methods;
+	iface->signals = NULL;
+	iface->properties = NULL;
+	iface->user_data = data;
+	iface->destroy = NULL;
+	data->interfaces = g_slist_append(data->interfaces, iface);
+
Instead of this duplicate interface initialization code I think it would make sense to refactor and create a new private function, e.g
static void add_interface(struct generic_cata *data, ...
and call that from the necessary places (afaics there are two of them)

Quoted 13 lines

gdbus/object.c static void object_path_unref(DBusConnection *connection, const char *path) · line 397
Quoted 10 lines
gdbus/object.c +10 -0
+	/* Free the introspection interface */
+	iface = find_interface(data->interfaces, "org.freedesktop.DBus.Introspectable");
+	if (iface) {
+		data->interfaces = g_slist_remove(data->interfaces, iface);
+
+		if (iface->destroy)
+			iface->destroy(iface->user_data);
+
+		g_free(iface->name);
+		g_free(iface);
Would it make sense to refactor create a remove_interface function for this too (to balance with the add_interface function)?

what are we gaining from doing it like this? I really don't see the benefit of doing it. Someone please explain it to me. It does add more code than it deletes.

Regards

Marcel

Johan Hedberg

2009-09-15 17:55 UTC #11 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi Marcel,

Quoted 3 lines · Marcel Holtmann

what are we gaining from doing it like this? I really don't see the benefit of doing it. Someone please explain it to me. It does add more code than it deletes.

It's about avoiding special-casing the Introspect method and using the framework that gdbus already has for method callbacks. This feels like a cleaner approach to me and with the two refactoring tasks I suggested I suspect we'd also be reducing the total line count (the current patch adds 15 lines to the total but there's something like 15-20 new lines in it that unnecessarily duplicate exsiting code).

Johan

RISKÓ Gergely

author

2009-09-15 19:25 UTC #12 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Quoted 7 lines · Johan Hedberg

Hi Marcel,
Quoted 3 lines · Marcel Holtmann
what are we gaining from doing it like this? I really don't see the benefit of doing it. Someone please explain it to me. It does add more code than it deletes.

Quoted 6 lines

It's about avoiding special-casing the Introspect method and using the framework that gdbus already has for method callbacks. This feels like a cleaner approach to me and with the two refactoring tasks I suggested I suspect we'd also be reducing the total line count (the current patch adds 15 lines to the total but there's something like 15-20 new lines in it that unnecessarily duplicate exsiting code).

Yes, and actually I totally agree about all of your comments, so it was just my too big hurry, why even code with C++ comment could enter the mailing list.

I will fix the issues and send a new patch tomorrow, thanks for the comments.

Gergely

RISKÓ Gergely

author

2009-09-16 11:07 UTC #13 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

In reply to Johan Hedberg

Hi,

Thanks for all the comments again, here is the next round.

Diffstat says 58 insertions, 43 deletions, I think the extra 15 lines really worth that bluez no longer treats the introspection as a very specific interface.

BR,
Gergely

Quoted 1 line

From de18b546d3773e33c65616680a6608457804d894 Mon Sep 17 00:00:00 2001

From: Gergely Risko <[email protected]>
Date: Tue, 15 Sep 2009 15:23:24 +0300
Subject: [PATCH] gdbus: handle introspection generally in generic_message.

Previously it was a specific case, now introspection is just another interface, which is always implemented. It is registered/unregistered when an object path is referenced first/last.

---
 gdbus/object.c |  101 ++++++++++++++++++++++++++++++++------------------------
 1 files changed, 58 insertions(+), 43 deletions(-)

gdbus/object.c +58 ~~-43~~

index 811c2e1..09e9af7 100644
@@ -155,13 +155,7 @@ static void generate_introspection_xml(DBusConnection *conn,
 
 	gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
-	g_string_append_printf(gstr,
-		"<node name=\"%s\">\n"
-		"\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n"
-		"\t\t<method name=\"Introspect\">\n"
-		"\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n"
-		"\t\t</method>\n"
-		"\t</interface>\n", path);
+	g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
 
 	for (list = data->interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;

@@ -189,14 +183,15 @@ done:
 	data->introspect = g_string_free(gstr, FALSE);
 }
 
-static DBusHandlerResult introspect(DBusConnection *connection,
-				DBusMessage *message, struct generic_data *data)
+static DBusMessage *introspect(DBusConnection *connection,
+				DBusMessage *message, void *user_data)
 {
+	struct generic_data *data = user_data;
 	DBusMessage *reply;
 
 	if (!dbus_message_has_signature(message, DBUS_TYPE_INVALID_AS_STRING)) {
 		error("Unexpected signature to introspect call");
-		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+		return NULL;
 	}
 
 	if (!data->introspect)

@@ -205,16 +200,12 @@ static DBusHandlerResult introspect(DBusConnection *connection,
 
 	reply = dbus_message_new_method_return(message);
 	if (!reply)
-		return DBUS_HANDLER_RESULT_NEED_MEMORY;
+		return NULL;
 
 	dbus_message_append_args(reply, DBUS_TYPE_STRING, &data->introspect,
 					DBUS_TYPE_INVALID);
 
-	dbus_connection_send(connection, reply, NULL);
-
-	dbus_message_unref(reply);
-
-	return DBUS_HANDLER_RESULT_HANDLED;
+	return reply;
 }
 
 static void generic_unregister(DBusConnection *connection, void *user_data)

@@ -250,11 +241,6 @@ static DBusHandlerResult generic_message(DBusConnection *connection,
 	GDBusMethodTable *method;
 	const char *interface;
 
-	if (dbus_message_is_method_call(message,
-					DBUS_INTERFACE_INTROSPECTABLE,
-								"Introspect"))
-		return introspect(connection, message, data);
-
 	interface = dbus_message_get_interface(message);
 
 	iface = find_interface(data->interfaces, interface);

@@ -335,6 +321,31 @@ done:
 	g_free(parent_path);
 }
 
+static GDBusMethodTable introspect_methods[] = {
+	{ "Introspect",	"",	"s", introspect	},
+	{ }
+};
+
+static void add_interface(struct generic_data *data, const char *name,
+			     GDBusMethodTable *methods,
+			     GDBusSignalTable *signals,
+			     GDBusPropertyTable *properties,
+			     void *user_data,
+			     GDBusDestroyFunction destroy)
+{
+	struct interface_data *iface;
+
+	iface = g_new0(struct interface_data, 1);
+	iface->name = g_strdup(name);
+	iface->methods = methods;
+	iface->signals = signals;
+	iface->properties = properties;
+	iface->user_data = user_data;
+	iface->destroy = destroy;
+
+	data->interfaces = g_slist_append(data->interfaces, iface);
+}
+
 static struct generic_data *object_path_ref(DBusConnection *connection,
 							const char *path)
 {

@@ -363,9 +374,30 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 
 	invalidate_parent_data(connection, path);
 
+	add_interface(data, DBUS_INTERFACE_INTROSPECTABLE,
+			introspect_methods, NULL, NULL, data, NULL);
+
 	return data;
 }
 
+static gboolean remove_interface(struct generic_data *data, const char *name)
+{
+	struct interface_data *iface;
+
+	iface = find_interface(data->interfaces, name);
+	if (iface) {
+		data->interfaces = g_slist_remove(data->interfaces, iface);
+
+		if (iface->destroy)
+			iface->destroy(iface->user_data);
+
+		g_free(iface->name);
+		g_free(iface);
+		return TRUE;
+	} else
+		return FALSE;
+}
+
 static void object_path_unref(DBusConnection *connection, const char *path)
 {
 	struct generic_data *data = NULL;

@@ -382,6 +414,8 @@ static void object_path_unref(DBusConnection *connection, const char *path)
 	if (data->refcount > 0)
 		return;
 
+	remove_interface(data, DBUS_INTERFACE_INTROSPECTABLE);
+
 	invalidate_parent_data(connection, path);
 
 	dbus_connection_unregister_object_path(connection, path);

@@ -474,7 +508,6 @@ gboolean g_dbus_register_interface(DBusConnection *connection,
 					GDBusDestroyFunction destroy)
 {
 	struct generic_data *data;
-	struct interface_data *iface;
 
 	data = object_path_ref(connection, path);
 	if (data == NULL)

@@ -483,16 +516,8 @@ gboolean g_dbus_register_interface(DBusConnection *connection,
 	if (find_interface(data->interfaces, name))
 		return FALSE;
 
-	iface = g_new0(struct interface_data, 1);
-
-	iface->name = g_strdup(name);
-	iface->methods = methods;
-	iface->signals = signals;
-	iface->properties = properties;
-	iface->user_data = user_data;
-	iface->destroy = destroy;
-
-	data->interfaces = g_slist_append(data->interfaces, iface);
+	add_interface(data, name, methods, signals,
+			properties, user_data, destroy);
 
 	g_free(data->introspect);
 	data->introspect = NULL;

@@ -504,7 +529,6 @@ gboolean g_dbus_unregister_interface(DBusConnection *connection,
 					const char *path, const char *name)
 {
 	struct generic_data *data = NULL;
-	struct interface_data *iface;
 
 	if (!path)
 		return FALSE;

@@ -516,18 +540,9 @@ gboolean g_dbus_unregister_interface(DBusConnection *connection,
 	if (data == NULL)
 		return FALSE;
 
-	iface = find_interface(data->interfaces, name);
-	if (!iface)
+	if (remove_interface(data, name) == FALSE)
 		return FALSE;
 
-	data->interfaces = g_slist_remove(data->interfaces, iface);
-
-	if (iface->destroy)
-		iface->destroy(iface->user_data);
-
-	g_free(iface->name);
-	g_free(iface);
-
 	g_free(data->introspect);
 	data->introspect = NULL;

Signature · 1 line

1.6.0.4

Johan Hedberg

2009-09-16 11:39 UTC #14 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi Gregely,

Quoted 3 lines · RISKÓ Gergely

Diffstat says 58 insertions, 43 deletions, I think the extra 15 lines really worth that bluez no longer treats the introspection as a very specific interface.

Looks good to me, except for the following (rather minor) issues:

gdbus/object.c done: · line 334

Quoted 6 lines

gdbus/object.c +6 -0

+static void add_interface(struct generic_data *data, const char *name,
+			     GDBusMethodTable *methods,
+			     GDBusSignalTable *signals,
+			     GDBusPropertyTable *properties,
+			     void *user_data,
+			     GDBusDestroyFunction destroy)

There seems to be mixed tabs & spaces here for indentation. Can you confirm this? We only use tabs for indentation in BlueZ (which I believe is also the usual kernel coding style).

Quoted 7 lines

gdbus/object.c +7 -0

+static gboolean remove_interface(struct generic_data *data, const char *name)
+{
+	struct interface_data *iface;
+
+	iface = find_interface(data->interfaces, name);
+	if (iface) {
+		data->interfaces = g_slist_remove(data->interfaces, iface);

gdbus/object.c static struct generic_data *object_path_ref(DBusConnection *connection, · line 399

Quoted 10 lines

gdbus/object.c +10 -0

+
+		if (iface->destroy)
+			iface->destroy(iface->user_data);
+
+		g_free(iface->name);
+		g_free(iface);
+		return TRUE;
+	} else
+		return FALSE;
+}

Marcel usually likes to do this as follows (and I agree with him):

iface = find_interface(data->interfaces, name);
if (!iface)

	return FALSE;

...rest of the code...

return TRUE;

That saves you one level of indentation for most of the function and could be considered also esier to read.

With those things fixed I think this is now up to Marcel whether he agrees with the change. In my opinion, even though it increases the total line count it still makes the code look nicer/cleaner since it removes the special casing for the introspection. E.g. one added benefit that wasn't previously mentioned is that now g_dbus_register_interface will correctly return an error if someone tries to register the introspection interface themselves.

Johan

RISKÓ Gergely

author

2009-09-16 12:03 UTC #15 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Thanks for the quick reply.

Quoted 1 line · Johan Hedberg

Looks good to me, except for the following (rather minor) issues:

All of them fixed in the patch at the end of the mail

Quoted 10 lines

Review
Quoted 6 lines
(quoted diff) +6 -0
+static void add_interface(struct generic_data *data, const char *name,
+			     GDBusMethodTable *methods,
+			     GDBusSignalTable *signals,
+			     GDBusPropertyTable *properties,
+			     void *user_data,
+			     GDBusDestroyFunction destroy)
There seems to be mixed tabs & spaces here for indentation. Can you confirm this? We only use tabs for indentation in BlueZ (which I believe is also the usual kernel coding style).

Hmm, interesting... And then they just don't care about "correct" indentation in respect to the opening parantheses in the previous line. Yeah, my Emacs config lines up things even with spaces if needed, but I fixed the patch as requested.

Quoted 22 lines

Quoted 7 lines
(quoted diff) +7 -0
+static gboolean remove_interface(struct generic_data *data, const char *name)
+{
+	struct interface_data *iface;
+
+	iface = find_interface(data->interfaces, name);
+	if (iface) {
+		data->interfaces = g_slist_remove(data->interfaces, iface);
Review
Quoted 10 lines
(quoted diff) +10 -0
+
+		if (iface->destroy)
+			iface->destroy(iface->user_data);
+
+		g_free(iface->name);
+		g_free(iface);
+		return TRUE;
+	} else
+		return FALSE;
+}
Marcel usually likes to do this as follows (and I agree with him):
iface = find_interface(data->interfaces, name);
if (!iface)

Quoted 8 lines

	return FALSE;
...rest of the code...
return TRUE;
That saves you one level of indentation for most of the function and could be considered also esier to read.

Agree, fixed.

Gergely

Quoted 1 line

From 68b79f26969017d62118734ac8f118d973445cf9 Mon Sep 17 00:00:00 2001

From: Gergely Risko <[email protected]>
Date: Tue, 15 Sep 2009 15:23:24 +0300
Subject: [PATCH] gdbus: handle introspection generally in generic_message.

Previously it was a specific case, now introspection is just another interface, which is always implemented. It is registered/unregistered when an object path is referenced first/last.

---
 gdbus/object.c |  101 ++++++++++++++++++++++++++++++++------------------------
 1 files changed, 58 insertions(+), 43 deletions(-)

gdbus/object.c +58 ~~-43~~

index 811c2e1..fde01ad 100644
@@ -155,13 +155,7 @@ static void generate_introspection_xml(DBusConnection *conn,
 
 	gstr = g_string_new(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE);
 
-	g_string_append_printf(gstr,
-		"<node name=\"%s\">\n"
-		"\t<interface name=\"org.freedesktop.DBus.Introspectable\">\n"
-		"\t\t<method name=\"Introspect\">\n"
-		"\t\t\t<arg name=\"xml_data\" type=\"s\" direction=\"out\"/>\n"
-		"\t\t</method>\n"
-		"\t</interface>\n", path);
+	g_string_append_printf(gstr, "<node name=\"%s\">\n", path);
 
 	for (list = data->interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;

@@ -189,14 +183,15 @@ done:
 	data->introspect = g_string_free(gstr, FALSE);
 }
 
-static DBusHandlerResult introspect(DBusConnection *connection,
-				DBusMessage *message, struct generic_data *data)
+static DBusMessage *introspect(DBusConnection *connection,
+				DBusMessage *message, void *user_data)
 {
+	struct generic_data *data = user_data;
 	DBusMessage *reply;
 
 	if (!dbus_message_has_signature(message, DBUS_TYPE_INVALID_AS_STRING)) {
 		error("Unexpected signature to introspect call");
-		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+		return NULL;
 	}
 
 	if (!data->introspect)

@@ -205,16 +200,12 @@ static DBusHandlerResult introspect(DBusConnection *connection,
 
 	reply = dbus_message_new_method_return(message);
 	if (!reply)
-		return DBUS_HANDLER_RESULT_NEED_MEMORY;
+		return NULL;
 
 	dbus_message_append_args(reply, DBUS_TYPE_STRING, &data->introspect,
 					DBUS_TYPE_INVALID);
 
-	dbus_connection_send(connection, reply, NULL);
-
-	dbus_message_unref(reply);
-
-	return DBUS_HANDLER_RESULT_HANDLED;
+	return reply;
 }
 
 static void generic_unregister(DBusConnection *connection, void *user_data)

@@ -250,11 +241,6 @@ static DBusHandlerResult generic_message(DBusConnection *connection,
 	GDBusMethodTable *method;
 	const char *interface;
 
-	if (dbus_message_is_method_call(message,
-					DBUS_INTERFACE_INTROSPECTABLE,
-								"Introspect"))
-		return introspect(connection, message, data);
-
 	interface = dbus_message_get_interface(message);
 
 	iface = find_interface(data->interfaces, interface);

@@ -335,6 +321,31 @@ done:
 	g_free(parent_path);
 }
 
+static GDBusMethodTable introspect_methods[] = {
+	{ "Introspect",	"",	"s", introspect	},
+	{ }
+};
+
+static void add_interface(struct generic_data *data, const char *name,
+				GDBusMethodTable *methods,
+				GDBusSignalTable *signals,
+				GDBusPropertyTable *properties,
+				void *user_data,
+				GDBusDestroyFunction destroy)
+{
+	struct interface_data *iface;
+
+	iface = g_new0(struct interface_data, 1);
+	iface->name = g_strdup(name);
+	iface->methods = methods;
+	iface->signals = signals;
+	iface->properties = properties;
+	iface->user_data = user_data;
+	iface->destroy = destroy;
+
+	data->interfaces = g_slist_append(data->interfaces, iface);
+}
+
 static struct generic_data *object_path_ref(DBusConnection *connection,
 							const char *path)
 {

@@ -363,9 +374,30 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 
 	invalidate_parent_data(connection, path);
 
+	add_interface(data, DBUS_INTERFACE_INTROSPECTABLE,
+			introspect_methods, NULL, NULL, data, NULL);
+
 	return data;
 }
 
+static gboolean remove_interface(struct generic_data *data, const char *name)
+{
+	struct interface_data *iface;
+
+	iface = find_interface(data->interfaces, name);
+	if (!iface)
+		return FALSE;
+
+	data->interfaces = g_slist_remove(data->interfaces, iface);
+
+	if (iface->destroy)
+		iface->destroy(iface->user_data);
+
+	g_free(iface->name);
+	g_free(iface);
+	return TRUE;
+}
+
 static void object_path_unref(DBusConnection *connection, const char *path)
 {
 	struct generic_data *data = NULL;

@@ -382,6 +414,8 @@ static void object_path_unref(DBusConnection *connection, const char *path)
 	if (data->refcount > 0)
 		return;
 
+	remove_interface(data, DBUS_INTERFACE_INTROSPECTABLE);
+
 	invalidate_parent_data(connection, path);
 
 	dbus_connection_unregister_object_path(connection, path);

@@ -474,7 +508,6 @@ gboolean g_dbus_register_interface(DBusConnection *connection,
 					GDBusDestroyFunction destroy)
 {
 	struct generic_data *data;
-	struct interface_data *iface;
 
 	data = object_path_ref(connection, path);
 	if (data == NULL)

@@ -483,16 +516,8 @@ gboolean g_dbus_register_interface(DBusConnection *connection,
 	if (find_interface(data->interfaces, name))
 		return FALSE;
 
-	iface = g_new0(struct interface_data, 1);
-
-	iface->name = g_strdup(name);
-	iface->methods = methods;
-	iface->signals = signals;
-	iface->properties = properties;
-	iface->user_data = user_data;
-	iface->destroy = destroy;
-
-	data->interfaces = g_slist_append(data->interfaces, iface);
+	add_interface(data, name, methods, signals,
+			properties, user_data, destroy);
 
 	g_free(data->introspect);
 	data->introspect = NULL;

@@ -504,7 +529,6 @@ gboolean g_dbus_unregister_interface(DBusConnection *connection,
 					const char *path, const char *name)
 {
 	struct generic_data *data = NULL;
-	struct interface_data *iface;
 
 	if (!path)
 		return FALSE;

@@ -516,18 +540,9 @@ gboolean g_dbus_unregister_interface(DBusConnection *connection,
 	if (data == NULL)
 		return FALSE;
 
-	iface = find_interface(data->interfaces, name);
-	if (!iface)
+	if (remove_interface(data, name) == FALSE)
 		return FALSE;
 
-	data->interfaces = g_slist_remove(data->interfaces, iface);
-
-	if (iface->destroy)
-		iface->destroy(iface->user_data);
-
-	g_free(iface->name);
-	g_free(iface);
-
 	g_free(data->introspect);
 	data->introspect = NULL;

Signature · 1 line

1.6.0.4

Johan Hedberg

2009-09-24 17:22 UTC #16 lore raw

Re: [PATCH] Add introspection interface to the output of introspection calls.

Hi,

Quoted 12 lines · RISKÓ Gergely

Quoted 2 lines
That saves you one level of indentation for most of the function and could be considered also esier to read.
Agree, fixed.
Gergely
From 68b79f26969017d62118734ac8f118d973445cf9 Mon Sep 17 00:00:00 2001
From: Gergely Risko <[email protected]>
Date: Tue, 15 Sep 2009 15:23:24 +0300
Subject: [PATCH] gdbus: handle introspection generally in generic_message.

Quoted 6 lines

Previously it was a specific case, now introspection is just another interface, which is always implemented. It is registered/unregistered when an object path is referenced first/last.
---
 gdbus/object.c |  101 ++++++++++++++++++++++++++++++++------------------------
 1 files changed, 58 insertions(+), 43 deletions(-)

The patch has now been pushed upstream.

Johan