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

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

[RISKÓ Gergely]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bluez-introspection.diff --]
[-- Type: text/x-diff, Size: 802 bytes --]

diff --git a/gdbus/object.c b/gdbus/object.c
index 3186921..8091a95 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -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;

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

[RISKÓ Gergely]

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

diff --git a/gdbus/object.c b/gdbus/object.c
index 3186921..8091a95 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -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;
-- 
1.6.0.4

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

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

[Luiz Augusto von Dentz]

Hi,

On Wed, Sep 2, 2009 at 7:00 AM, RISKÓ Gergely<gergely@risko.hu> wrote:
>
> ---
>  gdbus/object.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/gdbus/object.c b/gdbus/object.c
> index 3186921..8091a95 100644
> --- a/gdbus/object.c
> +++ b/gdbus/object.c
> @@ -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;

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.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

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

[RISKÓ Gergely]

Hi,

On Wed, 2 Sep 2009 11:46:15 -0300, Luiz Augusto von Dentz <luiz.dentz@gmail.com> writes:

> Hi,
>
> On Wed, Sep 2, 2009 at 7:00 AM, RISKÓ Gergely<gergely@risko.hu> wrote:
>>
>> ---
>>  gdbus/object.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/gdbus/object.c b/gdbus/object.c
>> index 3186921..8091a95 100644
>> --- a/gdbus/object.c
>> +++ b/gdbus/object.c
>> @@ -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;
>
> 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

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

[Johan Hedberg]

Hi Gergely,

On Wed, Sep 02, 2009, RISKÓ Gergely wrote:
> 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

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

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

[RISKÓ Gergely]

Hi Johan,

On Mon, 14 Sep 2009 17:18:16 +0300, Johan Hedberg <johan.hedberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> 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

>From 91b64b5cd3b174317bde5f9ccf4cc1d420086eb7 Mon Sep 17 00:00:00 2001
From: Gergely Risko <gergely@risko.hu>
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(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 3186921..811c2e1 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -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;
-- 
1.6.0.4

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

[Johan Hedberg]

Hi Gergely,

On Mon, Sep 14, 2009, RISKÓ Gergely wrote:
> Thank you for all of your advise, here is the result.
> 
> BR,
> Gergely
> 
> From 91b64b5cd3b174317bde5f9ccf4cc1d420086eb7 Mon Sep 17 00:00:00 2001
> From: Gergely Risko <gergely@risko.hu>
> 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

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

[Luiz Augusto von Dentz]

Hi,

On Mon, Sep 14, 2009 at 6:11 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> 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?

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.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

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

[RISKÓ Gergely]

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

>From 2188640a694ba9dd14f59ed250f8a37f3a1bec34 Mon Sep 17 00:00:00 2001
From: Gergely Risko <gergely+context@risko.hu>
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(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 811c2e1..3b6d3f2 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -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);
-- 
1.6.0.4



On Tue, 15 Sep 2009 07:50:42 -0300, Luiz Augusto von Dentz <luiz.dentz@gmail.com> writes:

> Hi,
>
> On Mon, Sep 14, 2009 at 6:11 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> 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?
>
> 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.

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

[Johan Hedberg]

Hi,

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

On Tue, Sep 15, 2009, RISKÓ Gergely wrote:
> -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)

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

Unnecessary explicit type-cast of a void pointer.

>  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

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

> +	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)

> +	/* 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

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

[Marcel Holtmann]

Hi Johan,

> The general idea of the patch looks good but there are a few things I'd
> still fix:
> 
> On Tue, Sep 15, 2009, RISKÓ Gergely wrote:
> > -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)
> 
> > +	struct generic_data *data = (struct generic_data *) data_;
> 
> Unnecessary explicit type-cast of a void pointer.
> 
> >  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
> 
> Either add the macro or remove this comment. Also, we don't use C++ style
> commenting.
> 
> > +	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)
> 
> > +	/* 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

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

[Johan Hedberg]

Hi Marcel,

On Tue, Sep 15, 2009, Marcel Holtmann wrote:
> 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

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

[RISKÓ Gergely]

Hi,

On Tue, 15 Sep 2009 20:55:23 +0300, Johan Hedberg <johan.hedberg@gmail.com> writes:

> Hi Marcel,
>
> On Tue, Sep 15, 2009, Marcel Holtmann wrote:
>> 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).

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

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

[RISKÓ Gergely]

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

>From de18b546d3773e33c65616680a6608457804d894 Mon Sep 17 00:00:00 2001
From: Gergely Risko <gergely@risko.hu>
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(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 811c2e1..09e9af7 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -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;
 
-- 
1.6.0.4

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

[Johan Hedberg]

Hi Gregely,

On Wed, Sep 16, 2009, RISKÓ Gergely wrote:
> 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:

> +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).

> +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;
> +}

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

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

[RISKÓ Gergely]

Hi,

Thanks for the quick reply.

On Wed, 16 Sep 2009 14:39:59 +0300, Johan Hedberg <johan.hedberg@gmail.com> writes:

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

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

>> +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.

>> +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;
>> +}
>
> 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.

Agree, fixed.

Gergely

>From 68b79f26969017d62118734ac8f118d973445cf9 Mon Sep 17 00:00:00 2001
From: Gergely Risko <gergely@risko.hu>
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(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 811c2e1..fde01ad 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -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;
 
-- 
1.6.0.4

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

[Johan Hedberg]

Hi,

On Wed, Sep 16, 2009, RISKÓ Gergely wrote:
> > 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 <gergely@risko.hu>
> 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(-)

The patch has now been pushed upstream.

Johan