Hi Sonny, On Wed, Dec 2, 2020 at 10:58 AM Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> wrote: > > Hi Luiz, > > The purpose here is that each application can have its own namespace > (e.g. /my/app, its objects should be in the form of /my/app/...) so > that it doesn't get mixed up with objects of other applications. I see > that there is already a pattern of using D-Bus ObjectManager this way > in bluez (there exists g_dbus_client_new_full to listen to objects in > a subtree), and also it is documented in D-Bus specification > (https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager) > so I don't think this violates any D-Bus best practices. The subtree support is meant for clients that actually want to do this in a subtree, rather then using '/', but bluetoothctl is already using '/' which would be parent to any subtree you would like to do. So while we support client using substres, I don't feel like bluetoothctl should use that as we already support adding GATT applications using '/' except if you really want to have multiple apps/subtrees but it doesn't look like this is the case and anyway when that happen that normally involves using different D-Bus connection. > On Wed, Dec 2, 2020 at 9:46 AM Luiz Augusto von Dentz > <luiz.dentz@xxxxxxxxx> wrote: > > > > Hi Manish, > > > > On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@xxxxxxxxxx> wrote: > > > > > > Bluetoothctl shares the gdbus library implementation with bluetoothd. > > > When bluetoothctl starts, it registers itself with the dbus with root > > > path same as the bluez root path i.e. '/'. > > > > > > When advertisement monitor objects are created or removed, > > > InterfacesAdded/InterfacesRemoved signals are emitted. These signals > > > are emitted at the registered root path by default. > > > > > > However, these signals need to be emitted at the app root path > > > registered using the "RegisterMonitor" method while registering the > > > client app with bluetoothd. > > > > > > This patch adds support in the gdbus library to emit signals at the app > > > root path. > > > > There can only be one root path really to guarantee there is one > > ObjectManager and we don't end up with ObjectManager listing other > > object managers which would likely cause problems. > > > > > Signed-off-by: Manish Mandlik <mmandlik@xxxxxxxxxx> > > > Reviewed-by: sonnysasaka@xxxxxxxxxxxx > > > Reviewed-by: howardchung@xxxxxxxxxx > > > Reviewed-by: mcchou@xxxxxxxxxxxx > > > > > > --- > > > > > > gdbus/gdbus.h | 15 +++++++++++++++ > > > gdbus/object.c | 39 ++++++++++++++++++++++++++++++--------- > > > 2 files changed, 45 insertions(+), 9 deletions(-) > > > > > > diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h > > > index 28b802296..3bddaf9e6 100644 > > > --- a/gdbus/gdbus.h > > > +++ b/gdbus/gdbus.h > > > @@ -210,6 +210,13 @@ struct GDBusSecurityTable { > > > void g_dbus_set_flags(int flags); > > > int g_dbus_get_flags(void); > > > > > > +/* Note that, when new interface is registered, InterfacesAdded signal is > > > + * emitted. This signal is by default emitted at root path "/" registered > > > + * while registering a client using g_dbus_client_new(). If this behavior > > > + * is undesired, use g_dbus_register_interface_full() with a desired root > > > + * path to ensure InterfacesAdded / InterfacesRemoved signals get emitted > > > + * at the correct path. > > > + */ > > > gboolean g_dbus_register_interface(DBusConnection *connection, > > > const char *path, const char *name, > > > const GDBusMethodTable *methods, > > > @@ -217,6 +224,14 @@ gboolean g_dbus_register_interface(DBusConnection *connection, > > > const GDBusPropertyTable *properties, > > > void *user_data, > > > GDBusDestroyFunction destroy); > > > +gboolean g_dbus_register_interface_full(DBusConnection *connection, > > > + const char *path, const char *name, > > > + const char *root_path, > > > + const GDBusMethodTable *methods, > > > + const GDBusSignalTable *signals, > > > + const GDBusPropertyTable *properties, > > > + void *user_data, > > > + GDBusDestroyFunction destroy); > > > gboolean g_dbus_unregister_interface(DBusConnection *connection, > > > const char *path, const char *name); > > > > > > diff --git a/gdbus/object.c b/gdbus/object.c > > > index 50a8b4ff1..0d8a0696e 100644 > > > --- a/gdbus/object.c > > > +++ b/gdbus/object.c > > > @@ -38,6 +38,7 @@ struct generic_data { > > > unsigned int refcount; > > > DBusConnection *conn; > > > char *path; > > > + char *root_path; > > > GSList *interfaces; > > > GSList *objects; > > > GSList *added; > > > @@ -551,9 +552,10 @@ static void emit_interfaces_added(struct generic_data *data) > > > if (root == NULL || data == root) > > > return; > > > > > > - signal = dbus_message_new_signal(root->path, > > > - DBUS_INTERFACE_OBJECT_MANAGER, > > > - "InterfacesAdded"); > > > + signal = dbus_message_new_signal( > > > + data->root_path ? data->root_path : root->path, > > > + DBUS_INTERFACE_OBJECT_MANAGER, > > > + "InterfacesAdded"); > > > if (signal == NULL) > > > return; > > > > > > @@ -953,9 +955,10 @@ static void emit_interfaces_removed(struct generic_data *data) > > > if (root == NULL || data == root) > > > return; > > > > > > - signal = dbus_message_new_signal(root->path, > > > - DBUS_INTERFACE_OBJECT_MANAGER, > > > - "InterfacesRemoved"); > > > + signal = dbus_message_new_signal( > > > + data->root_path ? data->root_path : root->path, > > > + DBUS_INTERFACE_OBJECT_MANAGER, > > > + "InterfacesRemoved"); > > > if (signal == NULL) > > > return; > > > > > > @@ -1026,6 +1029,7 @@ static void generic_unregister(DBusConnection *connection, void *user_data) > > > > > > dbus_connection_unref(data->conn); > > > g_free(data->introspect); > > > + g_free(data->root_path); > > > g_free(data->path); > > > g_free(data); > > > } > > > @@ -1222,7 +1226,8 @@ done: > > > } > > > > > > static struct generic_data *object_path_ref(DBusConnection *connection, > > > - const char *path) > > > + const char *path, > > > + const char *root_path) > > > { > > > struct generic_data *data; > > > > > > @@ -1237,6 +1242,8 @@ static struct generic_data *object_path_ref(DBusConnection *connection, > > > data = g_new0(struct generic_data, 1); > > > data->conn = dbus_connection_ref(connection); > > > data->path = g_strdup(path); > > > + if (root_path) > > > + data->root_path = g_strdup(root_path); > > > data->refcount = 1; > > > > > > data->introspect = g_strdup(DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE "<node></node>"); > > > @@ -1245,6 +1252,7 @@ static struct generic_data *object_path_ref(DBusConnection *connection, > > > &generic_table, data)) { > > > dbus_connection_unref(data->conn); > > > g_free(data->path); > > > + g_free(data->root_path); > > > g_free(data->introspect); > > > g_free(data); > > > return NULL; > > > @@ -1330,6 +1338,19 @@ gboolean g_dbus_register_interface(DBusConnection *connection, > > > const GDBusPropertyTable *properties, > > > void *user_data, > > > GDBusDestroyFunction destroy) > > > +{ > > > + return g_dbus_register_interface_full(connection, path, name, NULL, > > > + methods, signals, properties, user_data, destroy); > > > +} > > > + > > > +gboolean g_dbus_register_interface_full(DBusConnection *connection, > > > + const char *path, const char *name, > > > + const char *root_path, > > > + const GDBusMethodTable *methods, > > > + const GDBusSignalTable *signals, > > > + const GDBusPropertyTable *properties, > > > + void *user_data, > > > + GDBusDestroyFunction destroy) > > > { > > > struct generic_data *data; > > > > > > @@ -1343,7 +1364,7 @@ gboolean g_dbus_register_interface(DBusConnection *connection, > > > return FALSE; > > > } > > > > > > - data = object_path_ref(connection, path); > > > + data = object_path_ref(connection, path, root_path); > > > if (data == NULL) > > > return FALSE; > > > > > > @@ -1811,7 +1832,7 @@ gboolean g_dbus_attach_object_manager(DBusConnection *connection) > > > { > > > struct generic_data *data; > > > > > > - data = object_path_ref(connection, "/"); > > > + data = object_path_ref(connection, "/", NULL); > > > if (data == NULL) > > > return FALSE; > > > > > > -- > > > 2.29.2.454.gaff20da3a2-goog > > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz
