[PATCH BlueZ 1/4] gdbus: Fix having multiple path exporting ObjectManager

[PATCH BlueZ 1/4] gdbus: Fix having multiple path exporting ObjectManager

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

ObjectManager should only be available on the root path so if the
current is a child of the object being registered the root should be
changed.
---
 gdbus/object.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 3101ca6..43154f3 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -84,6 +84,8 @@ struct property_data {
 	DBusMessage *message;
 };
 
+static struct generic_data *root = NULL;
+
 static gboolean process_changes(gpointer user_data);
 static void process_properties_from_interface(struct generic_data *data,
 						struct interface_data *iface);
@@ -574,11 +576,7 @@ static void emit_interfaces_added(struct generic_data *data)
 	if (parent == NULL)
 		return;
 
-	/* Find root data */
-	while (parent->parent)
-		parent = parent->parent;
-
-	signal = dbus_message_new_signal(parent->path,
+	signal = dbus_message_new_signal(root->path,
 					DBUS_INTERFACE_OBJECT_MANAGER,
 					"InterfacesAdded");
 	if (signal == NULL)
@@ -948,11 +946,7 @@ static void emit_interfaces_removed(struct generic_data *data)
 	if (parent == NULL)
 		return;
 
-	/* Find root data */
-	while (parent->parent)
-		parent = parent->parent;
-
-	signal = dbus_message_new_signal(parent->path,
+	signal = dbus_message_new_signal(root->path,
 					DBUS_INTERFACE_OBJECT_MANAGER,
 					"InterfacesRemoved");
 	if (signal == NULL)
@@ -1008,6 +1002,9 @@ static void generic_unregister(DBusConnection *connection, void *user_data)
 	g_slist_foreach(data->objects, reset_parent, data->parent);
 	g_slist_free(data->objects);
 
+	if (root == data)
+		root = NULL;
+
 	dbus_connection_unref(data->conn);
 	g_free(data->introspect);
 	g_free(data->path);
@@ -1175,6 +1172,20 @@ static void add_interface(struct generic_data *data,
 	data->process_id = g_idle_add(process_changes, data);
 }
 
+static void set_root(struct generic_data *data)
+{
+	if (root != NULL) {
+		data->objects = g_slist_prepend(data->objects, root);
+		root->parent = data;
+		remove_interface(root, DBUS_INTERFACE_OBJECT_MANAGER);
+	}
+
+	add_interface(data, DBUS_INTERFACE_OBJECT_MANAGER,
+					manager_methods, manager_signals,
+					NULL, data, NULL);
+	root = data;
+}
+
 static struct generic_data *object_path_ref(DBusConnection *connection,
 							const char *path)
 {
@@ -1209,9 +1220,7 @@ static struct generic_data *object_path_ref(DBusConnection *connection,
 
 	/* Only root path export ObjectManager interface */
 	if (data->parent == NULL)
-		add_interface(data, DBUS_INTERFACE_OBJECT_MANAGER,
-					manager_methods, manager_signals,
-					NULL, data, NULL);
+		set_root(data);
 
 	add_interface(data, DBUS_INTERFACE_PROPERTIES, properties_methods,
 					properties_signals, NULL, data, NULL);
-- 
1.7.11.7

[PATCH BlueZ 2/4] gdbus: Automatically register '/' path

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This makes g_dbus_setup_bus to automatically register '/' path so
user application that don't export any interface on '/' will have it
registered for ObjectManager.

Note that it is now required to call g_dbus_close before exit.
---
 gdbus/gdbus.h    |  1 +
 gdbus/mainloop.c | 55 ++++++++++++++++++++++++++++++-------------------------
 gdbus/object.c   |  2 +-
 3 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index ba49621..5056340 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -53,6 +53,7 @@ DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
 
 DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
 							DBusError *error);
+void g_dbus_close(DBusConnection *connection);
 
 gboolean g_dbus_request_name(DBusConnection *connection, const char *name,
 							DBusError *error);
diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index 099b67f..49e6538 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -286,27 +286,36 @@ static gboolean setup_bus(DBusConnection *conn, const char *name,
 	return TRUE;
 }
 
-DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
+static gboolean g_dbus_connect(DBusConnection *conn, const char *name,
 							DBusError *error)
 {
-	DBusConnection *conn;
-
-	conn = dbus_bus_get(type, error);
-
 	if (error != NULL) {
 		if (dbus_error_is_set(error) == TRUE)
-			return NULL;
+			return FALSE;
 	}
 
-	if (conn == NULL)
-		return NULL;
+	if (setup_bus(conn, name, error) == FALSE)
+		return FALSE;
 
-	if (setup_bus(conn, name, error) == FALSE) {
-		dbus_connection_unref(conn);
-		return NULL;
-	}
+	if (!g_dbus_register_interface(conn, "/", NULL, NULL, NULL, NULL, NULL,
+									NULL))
+		return FALSE;
 
-	return conn;
+	return TRUE;
+}
+
+DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
+							DBusError *error)
+{
+	DBusConnection *conn;
+
+	conn = dbus_bus_get(type, error);
+
+	if (g_dbus_connect(conn, name, error))
+		return conn;
+
+	dbus_connection_unref(conn);
+	return NULL;
 }
 
 DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
@@ -316,20 +325,16 @@ DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
 
 	conn = dbus_bus_get_private(type, error);
 
-	if (error != NULL) {
-		if (dbus_error_is_set(error) == TRUE)
-			return NULL;
-	}
-
-	if (conn == NULL)
-		return NULL;
+	if (g_dbus_connect(conn, name, error))
+		return conn;
 
-	if (setup_bus(conn, name, error) == FALSE) {
-		dbus_connection_unref(conn);
-		return NULL;
-	}
+	dbus_connection_unref(conn);
+	return NULL;
+}
 
-	return conn;
+void g_dbus_close(DBusConnection *connection)
+{
+	g_dbus_unregister_interface(connection, "/", NULL);
 }
 
 gboolean g_dbus_request_name(DBusConnection *connection, const char *name,
diff --git a/gdbus/object.c b/gdbus/object.c
index 43154f3..2a2982d 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -615,7 +615,7 @@ static struct interface_data *find_interface(GSList *interfaces,
 
 	for (list = interfaces; list; list = list->next) {
 		struct interface_data *iface = list->data;
-		if (!strcmp(name, iface->name))
+		if (g_strcmp0(name, iface->name) == 0)
 			return iface;
 	}
 
-- 
1.7.11.7

Re: [PATCH BlueZ 2/4] gdbus: Automatically register '/' path

[Lucas De Marchi]

Hi Luiz,

On Mon, Nov 19, 2012 at 6:46 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This makes g_dbus_setup_bus to automatically register '/' path so
> user application that don't export any interface on '/' will have it
> registered for ObjectManager.
>
> Note that it is now required to call g_dbus_close before exit.

I'm no really fan of this. Why do we need to register '/' now?  If we
are not going to support ObjectManager interfaces in subtrees, it
would be easier to just move the ObjectManager to the shortest path
registered rather than always registering '/'.


Lucas De Marchi

Re: [PATCH BlueZ 2/4] gdbus: Automatically register '/' path

[Luiz Augusto von Dentz]

Hi Lucas,

On Mon, Nov 19, 2012 at 2:06 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> Hi Luiz,
>
> On Mon, Nov 19, 2012 at 6:46 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This makes g_dbus_setup_bus to automatically register '/' path so
>> user application that don't export any interface on '/' will have it
>> registered for ObjectManager.
>>
>> Note that it is now required to call g_dbus_close before exit.
>
> I'm no really fan of this. Why do we need to register '/' now?  If we
> are not going to support ObjectManager interfaces in subtrees, it
> would be easier to just move the ObjectManager to the shortest path
> registered rather than always registering '/'.

Maybe I should have made clear this in the description, if you look at
the spec it suggest that each sub-tree root should implement
ObjectManager, the current implementation does that but if you think
about it probably doesn't make sense to have sub-trees because that
would creates extra round trips that ObjectManager was meant to
prevent, also Im not completely sure now but I remember someone
mentioning the '/' is kind special and should always be available no
matter what, so by registering '/' directly on g_dbus_setup_bus we
guarantee we have it always and that no sub-tree is going to be
generated since '/' is the root no matter what path the user code
register.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/4] gdbus: Automatically register '/' path

[Lucas De Marchi]

On Mon, Nov 19, 2012 at 10:49 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Mon, Nov 19, 2012 at 2:06 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> Hi Luiz,
>>
>> On Mon, Nov 19, 2012 at 6:46 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This makes g_dbus_setup_bus to automatically register '/' path so
>>> user application that don't export any interface on '/' will have it
>>> registered for ObjectManager.
>>>
>>> Note that it is now required to call g_dbus_close before exit.
>>
>> I'm no really fan of this. Why do we need to register '/' now?  If we
>> are not going to support ObjectManager interfaces in subtrees, it
>> would be easier to just move the ObjectManager to the shortest path
>> registered rather than always registering '/'.
>
> Maybe I should have made clear this in the description, if you look at
> the spec it suggest that each sub-tree root should implement
> ObjectManager, the current implementation does that but if you think
> about it probably doesn't make sense to have sub-trees because that
> would creates extra round trips that ObjectManager was meant to

HUmn... I think you are a bit confused now.

1 entire tree can be managed by a single ObjectManager, or multiple
ones if there are objects that are not interesting to everybody and
only a few types of clients would be interested in the objects changed
beyond that path.

Example:

/org/bluez
  - ObjectManager
/org/bluez/adapter/bla/bli
  - MyInterfaceWithLotsOfObjects

If you put only 1 ObjectManager it means you get called for each
change in /org/bluez/adapter/bla/bli even though most of the clients
will not be interested in that. Therefore the spec allows you to
attach another ObjectManager in /org/bluez/adapter/[...]  so the first
ObjectManager only send updates and "manage" the objects until the
path containing another ObjectManager.

I do think we don't really need that feature right now. I think we are
fine with a single ObjectManager in "/", but always registering "/"
IMO is not the right solution.

> prevent, also Im not completely sure now but I remember someone
> mentioning the '/' is kind special and should always be available no
> matter what, so by registering '/' directly on g_dbus_setup_bus we
> guarantee we have it always and that no sub-tree is going to be
> generated since '/' is the root no matter what path the user code
> register.

Kind of true... what really needs to be treated are calls to
Introspectable interface on "/". This is used by tools like d-feet to
navigate the tree (looking for the "<node ...>" tags) -- I *think*
this is also used by some bindings like python's. If you look in other
projects around, they prefer using the project's namespace rather than
"/"  (examples: systemd, udisks, policykit, dconf).

So, if we register "/", but not an Introspectable interface in there,
it means we will break tools like d-feet. If we don't register it,
libdbus will handle the messages to Introspectable (this is why other
projects don't even bother about "/").


Lucas De Marchi

Re: [PATCH BlueZ 2/4] gdbus: Automatically register '/' path

[Luiz Augusto von Dentz]

Hi Lucas,

On Mon, Nov 19, 2012 at 3:18 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Mon, Nov 19, 2012 at 10:49 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Lucas,
>>
>> On Mon, Nov 19, 2012 at 2:06 PM, Lucas De Marchi
>> <lucas.demarchi@profusion.mobi> wrote:
>>> Hi Luiz,
>>>
>>> On Mon, Nov 19, 2012 at 6:46 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>
>>>> This makes g_dbus_setup_bus to automatically register '/' path so
>>>> user application that don't export any interface on '/' will have it
>>>> registered for ObjectManager.
>>>>
>>>> Note that it is now required to call g_dbus_close before exit.
>>>
>>> I'm no really fan of this. Why do we need to register '/' now?  If we
>>> are not going to support ObjectManager interfaces in subtrees, it
>>> would be easier to just move the ObjectManager to the shortest path
>>> registered rather than always registering '/'.
>>
>> Maybe I should have made clear this in the description, if you look at
>> the spec it suggest that each sub-tree root should implement
>> ObjectManager, the current implementation does that but if you think
>> about it probably doesn't make sense to have sub-trees because that
>> would creates extra round trips that ObjectManager was meant to
>
> HUmn... I think you are a bit confused now.
>
> 1 entire tree can be managed by a single ObjectManager, or multiple
> ones if there are objects that are not interesting to everybody and
> only a few types of clients would be interested in the objects changed
> beyond that path.
>
> Example:
>
> /org/bluez
>   - ObjectManager
> /org/bluez/adapter/bla/bli
>   - MyInterfaceWithLotsOfObjects
>
> If you put only 1 ObjectManager it means you get called for each
> change in /org/bluez/adapter/bla/bli even though most of the clients
> will not be interested in that. Therefore the spec allows you to
> attach another ObjectManager in /org/bluez/adapter/[...]  so the first
> ObjectManager only send updates and "manage" the objects until the
> path containing another ObjectManager.
>
> I do think we don't really need that feature right now. I think we are
> fine with a single ObjectManager in "/", but always registering "/"
> IMO is not the right solution.

As far I know /org/bluez/adapter/bla/bli is a child of /org/bluez thus
you don't need another ObjectManager, anyway the point is that
multiple ObjectManagers for the same connection might not make sense
as it will create more round trips to discover the object tree e.g:
/org/bluez vs /org/somethingelse

There is also the problem of supporting changing the root, the spec
doesn't mention anything about that even though it says it can support
sub-trees thus the application could in theory register a new root
e.g. /org at any point which basically invalidates the entire
sub-trees which I don't think would be convenient for anyone watching
it.

>> prevent, also Im not completely sure now but I remember someone
>> mentioning the '/' is kind special and should always be available no
>> matter what, so by registering '/' directly on g_dbus_setup_bus we
>> guarantee we have it always and that no sub-tree is going to be
>> generated since '/' is the root no matter what path the user code
>> register.
>
> Kind of true... what really needs to be treated are calls to
> Introspectable interface on "/". This is used by tools like d-feet to
> navigate the tree (looking for the "<node ...>" tags) -- I *think*
> this is also used by some bindings like python's. If you look in other
> projects around, they prefer using the project's namespace rather than
> "/"  (examples: systemd, udisks, policykit, dconf).
>
> So, if we register "/", but not an Introspectable interface in there,
> it means we will break tools like d-feet. If we don't register it,
> libdbus will handle the messages to Introspectable (this is why other
> projects don't even bother about "/").

Yep, I assume it was more convenient to have in '/' because it would
basically work in any case, but we can default to something else it
just a matter to have this set directly when we do g_dbus_setup_bus so
it is available since the beginning.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/4] gdbus: Automatically register '/' path

[Lucas De Marchi]

On Mon, Nov 19, 2012 at 1:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Mon, Nov 19, 2012 at 3:18 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> On Mon, Nov 19, 2012 at 10:49 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Lucas,
>>>
>>> On Mon, Nov 19, 2012 at 2:06 PM, Lucas De Marchi
>>> <lucas.demarchi@profusion.mobi> wrote:
>>>> Hi Luiz,
>>>>
>>>> On Mon, Nov 19, 2012 at 6:46 AM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>
>>>>> This makes g_dbus_setup_bus to automatically register '/' path so
>>>>> user application that don't export any interface on '/' will have it
>>>>> registered for ObjectManager.
>>>>>
>>>>> Note that it is now required to call g_dbus_close before exit.
>>>>
>>>> I'm no really fan of this. Why do we need to register '/' now?  If we
>>>> are not going to support ObjectManager interfaces in subtrees, it
>>>> would be easier to just move the ObjectManager to the shortest path
>>>> registered rather than always registering '/'.
>>>
>>> Maybe I should have made clear this in the description, if you look at
>>> the spec it suggest that each sub-tree root should implement
>>> ObjectManager, the current implementation does that but if you think
>>> about it probably doesn't make sense to have sub-trees because that
>>> would creates extra round trips that ObjectManager was meant to
>>
>> HUmn... I think you are a bit confused now.
>>
>> 1 entire tree can be managed by a single ObjectManager, or multiple
>> ones if there are objects that are not interesting to everybody and
>> only a few types of clients would be interested in the objects changed
>> beyond that path.
>>
>> Example:
>>
>> /org/bluez
>>   - ObjectManager
>> /org/bluez/adapter/bla/bli
>>   - MyInterfaceWithLotsOfObjects
>>
>> If you put only 1 ObjectManager it means you get called for each
>> change in /org/bluez/adapter/bla/bli even though most of the clients
>> will not be interested in that. Therefore the spec allows you to
>> attach another ObjectManager in /org/bluez/adapter/[...]  so the first
>> ObjectManager only send updates and "manage" the objects until the
>> path containing another ObjectManager.
>>
>> I do think we don't really need that feature right now. I think we are
>> fine with a single ObjectManager in "/", but always registering "/"
>> IMO is not the right solution.
>
> As far I know /org/bluez/adapter/bla/bli is a child of /org/bluez thus
> you don't need another ObjectManager, anyway the point is that

Sure we don't need it, but it was made that way so projects could
actually use multiple object managers for the reasons pointed out in
the previous email. Projects could even have separate ObjectManagers
for completely different things, like one in /org/bluez/bla and
another in /org/bluez/bli instead of 1 in /org/bluez. But it's an
application decision where to put the ObjectManager.

If all projects using gdbus decide to have a single ObjectManager on
"/" because it's simpler and fit the needs for everybody, then fine.
I'd suggest to even register it earlier than it is. Otherwise we might
choose to let the application (bluez, connman, ofono, etc) decide
where to put it, by adding a "g_dbus_attach_object_manager(conn,
path)" or the like.


> multiple ObjectManagers for the same connection might not make sense
> as it will create more round trips to discover the object tree e.g:
> /org/bluez vs /org/somethingelse

I'm not arguing about one object manager per object path component.

>
> There is also the problem of supporting changing the root, the spec
> doesn't mention anything about that even though it says it can support
> sub-trees thus the application could in theory register a new root
> e.g. /org at any point which basically invalidates the entire
> sub-trees which I don't think would be convenient for anyone watching
> it.

then it's a misuse of object manager by the application, not a problem
with the spec. We need to define in the API where the object
manager(s) will be and not randomly adding/removing them.

Lucas De Marchi

[PATCH BlueZ 3/4] gdbus: Fix not calling dbus_connection_close for private connections

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

dbus_bus_get_private documentation says:

  "Unlike dbus_bus_get(), always creates a new connection. This
  connection will not be saved or recycled by libdbus. Caller owns a
  reference to the bus and must either close it or know it to be closed
  prior to releasing this reference."
---
 gdbus/mainloop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index 49e6538..4771f20 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -328,6 +328,7 @@ DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
 	if (g_dbus_connect(conn, name, error))
 		return conn;
 
+	dbus_connection_close(conn);
 	dbus_connection_unref(conn);
 	return NULL;
 }
-- 
1.7.11.7

[PATCH BlueZ 4/4] core: Make use of g_dbus_close

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 src/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/main.c b/src/main.c
index 414849a..1ca6015 100644
--- a/src/main.c
+++ b/src/main.c
@@ -383,6 +383,7 @@ static void disconnect_dbus(void)
 
 	set_dbus_connection(NULL);
 
+	g_dbus_close(conn);
 	dbus_connection_unref(conn);
 }
 
-- 
1.7.11.7