* [bluez PATCH v1 0/3] Emit InterfacesAdded/InterfacesRemoved at correct root path
@ 2020-12-02 1:19 Manish Mandlik
2020-12-02 1:19 ` [bluez PATCH v1 1/3] adv_monitor: Register client app with app-base-path Manish Mandlik
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Manish Mandlik @ 2020-12-02 1:19 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Manish Mandlik
Hello Maintainers,
Existing advertisement monitor implementation registers client app with
bluez-root-path i.e. "/". Because of which client app needs to emit
InterfacesAdded and InterfacesRemoved signals - when monitor objects are
added or removed - at the bluez-root-path.
This may cause confusion for application developers as the app need to
register with bluez with app-root-path for exposing monitor object, but
need to emit InterfacesAdded and InterfacesRemoved signals on the
bluez-root-path.
This patch series fixes advertisement monitor implementation to register
client with client specified app-root-path. Also, adds support in gdbus
library to emit signals at the app-root-path so that bluetoothctl can
emit InterfacesAdded/InterfacesRemoved signals correctly when adv-
monitors are created.
These changes are verified by running the bluetoothctl as well as the
python tester app and verifying that the monitor objects are getting
exposed and DeviceFound/DeviceLost events are getting invoked correctly.
Regards,
Manish.
Manish Mandlik (3):
adv_monitor: Register client app with app-base-path
gdbus: Emit InterfacesAdded/Removed at app root path
client: Fix add advertisement monitor
client/adv_monitor.c | 3 ++-
gdbus/gdbus.h | 15 +++++++++++++++
gdbus/object.c | 39 ++++++++++++++++++++++++++++++---------
src/adv_monitor.c | 2 +-
4 files changed, 48 insertions(+), 11 deletions(-)
--
2.29.2.454.gaff20da3a2-goog
^ permalink raw reply [flat|nested] 11+ messages in thread* [bluez PATCH v1 1/3] adv_monitor: Register client app with app-base-path 2020-12-02 1:19 [bluez PATCH v1 0/3] Emit InterfacesAdded/InterfacesRemoved at correct root path Manish Mandlik @ 2020-12-02 1:19 ` Manish Mandlik 2020-12-02 1:48 ` Emit InterfacesAdded/InterfacesRemoved at correct root path bluez.test.bot 2020-12-02 1:19 ` [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app " Manish Mandlik 2020-12-02 1:19 ` [bluez PATCH v1 3/3] client: Fix add advertisement monitor Manish Mandlik 2 siblings, 1 reply; 11+ messages in thread From: Manish Mandlik @ 2020-12-02 1:19 UTC (permalink / raw) To: marcel, luiz.dentz Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Manish Mandlik, sonnysasaka, howardchung, mcchou When a client app is registered with g_dbus_client_new(), bluez root path, i.e. '/', is used as the app root path and signal watches are added at the root path. Because of this, InterfacesAdded/InterfacesRemoved signals emitted by app while creating/removing advertisement monitor objects at app root path are not received by bluetoothd. Use g_dbus_client_new_full() to register client app with correct app root path. Signed-off-by: Manish Mandlik <mmandlik@google.com> Reviewed-by: sonnysasaka@chromium.org Reviewed-by: howardchung@google.com Reviewed-by: mcchou@chromium.org --- src/adv_monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/adv_monitor.c b/src/adv_monitor.c index fc058dbf1..a5bc61c48 100644 --- a/src/adv_monitor.c +++ b/src/adv_monitor.c @@ -775,7 +775,7 @@ static struct adv_monitor_app *app_create(DBusConnection *conn, app->manager = manager; app->reg = NULL; - app->client = g_dbus_client_new(conn, sender, path); + app->client = g_dbus_client_new_full(conn, sender, path, path); if (!app->client) { app_destroy(app); return NULL; -- 2.29.2.454.gaff20da3a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: Emit InterfacesAdded/InterfacesRemoved at correct root path 2020-12-02 1:19 ` [bluez PATCH v1 1/3] adv_monitor: Register client app with app-base-path Manish Mandlik @ 2020-12-02 1:48 ` bluez.test.bot 0 siblings, 0 replies; 11+ messages in thread From: bluez.test.bot @ 2020-12-02 1:48 UTC (permalink / raw) To: linux-bluetooth, mmandlik [-- Attachment #1: Type: text/plain, Size: 557 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=394297 ---Test result--- ############################## Test: CheckPatch - PASS ############################## Test: CheckGitLint - PASS ############################## Test: CheckBuild - PASS ############################## Test: MakeCheck - PASS --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 11+ messages in thread
* [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app root path 2020-12-02 1:19 [bluez PATCH v1 0/3] Emit InterfacesAdded/InterfacesRemoved at correct root path Manish Mandlik 2020-12-02 1:19 ` [bluez PATCH v1 1/3] adv_monitor: Register client app with app-base-path Manish Mandlik @ 2020-12-02 1:19 ` Manish Mandlik 2020-12-02 17:46 ` Luiz Augusto von Dentz 2020-12-02 1:19 ` [bluez PATCH v1 3/3] client: Fix add advertisement monitor Manish Mandlik 2 siblings, 1 reply; 11+ messages in thread From: Manish Mandlik @ 2020-12-02 1:19 UTC (permalink / raw) To: marcel, luiz.dentz Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Manish Mandlik, sonnysasaka, howardchung, mcchou 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. Signed-off-by: Manish Mandlik <mmandlik@google.com> Reviewed-by: sonnysasaka@chromium.org Reviewed-by: howardchung@google.com Reviewed-by: mcchou@chromium.org --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app root path 2020-12-02 1:19 ` [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app " Manish Mandlik @ 2020-12-02 17:46 ` Luiz Augusto von Dentz 2020-12-02 18:58 ` Sonny Sasaka 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-12-02 17:46 UTC (permalink / raw) To: Manish Mandlik Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org, ChromeOS Bluetooth Upstreaming, Sonny Sasaka, Yun-hao Chung, Miao-chen Chou Hi Manish, On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@google.com> 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@google.com> > Reviewed-by: sonnysasaka@chromium.org > Reviewed-by: howardchung@google.com > Reviewed-by: mcchou@chromium.org > > --- > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app root path 2020-12-02 17:46 ` Luiz Augusto von Dentz @ 2020-12-02 18:58 ` Sonny Sasaka 2020-12-02 19:50 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Sonny Sasaka @ 2020-12-02 18:58 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Manish Mandlik, Marcel Holtmann, linux-bluetooth@vger.kernel.org, ChromeOS Bluetooth Upstreaming, Yun-hao Chung, Miao-chen Chou 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. On Wed, Dec 2, 2020 at 9:46 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Manish, > > On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@google.com> 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@google.com> > > Reviewed-by: sonnysasaka@chromium.org > > Reviewed-by: howardchung@google.com > > Reviewed-by: mcchou@chromium.org > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app root path 2020-12-02 18:58 ` Sonny Sasaka @ 2020-12-02 19:50 ` Luiz Augusto von Dentz 2020-12-02 23:27 ` Sonny Sasaka 0 siblings, 1 reply; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-12-02 19:50 UTC (permalink / raw) To: Sonny Sasaka Cc: Manish Mandlik, Marcel Holtmann, linux-bluetooth@vger.kernel.org, ChromeOS Bluetooth Upstreaming, Yun-hao Chung, Miao-chen Chou Hi Sonny, On Wed, Dec 2, 2020 at 10:58 AM Sonny Sasaka <sonnysasaka@chromium.org> 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@gmail.com> wrote: > > > > Hi Manish, > > > > On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@google.com> 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@google.com> > > > Reviewed-by: sonnysasaka@chromium.org > > > Reviewed-by: howardchung@google.com > > > Reviewed-by: mcchou@chromium.org > > > > > > --- > > > > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app root path 2020-12-02 19:50 ` Luiz Augusto von Dentz @ 2020-12-02 23:27 ` Sonny Sasaka 2020-12-03 1:10 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 11+ messages in thread From: Sonny Sasaka @ 2020-12-02 23:27 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Manish Mandlik, Marcel Holtmann, linux-bluetooth@vger.kernel.org, ChromeOS Bluetooth Upstreaming, Yun-hao Chung, Miao-chen Chou Hi Luiz, If I understand correctly, your concern is only on the bluetoothctl part? If so, it is okay for bluetoothd to listen to subtrees for clients that want to use subtree, is that correct? On Wed, Dec 2, 2020 at 11:50 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Sonny, > > On Wed, Dec 2, 2020 at 10:58 AM Sonny Sasaka <sonnysasaka@chromium.org> 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@gmail.com> wrote: > > > > > > Hi Manish, > > > > > > On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@google.com> 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@google.com> > > > > Reviewed-by: sonnysasaka@chromium.org > > > > Reviewed-by: howardchung@google.com > > > > Reviewed-by: mcchou@chromium.org > > > > > > > > --- > > > > > > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app root path 2020-12-02 23:27 ` Sonny Sasaka @ 2020-12-03 1:10 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-12-03 1:10 UTC (permalink / raw) To: Sonny Sasaka Cc: Manish Mandlik, Marcel Holtmann, linux-bluetooth@vger.kernel.org, ChromeOS Bluetooth Upstreaming, Yun-hao Chung, Miao-chen Chou Hi Sonny, On Wed, Dec 2, 2020 at 3:27 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > Hi Luiz, > > If I understand correctly, your concern is only on the bluetoothctl > part? If so, it is okay for bluetoothd to listen to subtrees for > clients that want to use subtree, is that correct? Yep, in fact using the same tree may be even preferable in case of bluetoothctl since we can just reference the same client in case it is registering a monitor and GATT service for example. > On Wed, Dec 2, 2020 at 11:50 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Sonny, > > > > On Wed, Dec 2, 2020 at 10:58 AM Sonny Sasaka <sonnysasaka@chromium.org> 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@gmail.com> wrote: > > > > > > > > Hi Manish, > > > > > > > > On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@google.com> 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@google.com> > > > > > Reviewed-by: sonnysasaka@chromium.org > > > > > Reviewed-by: howardchung@google.com > > > > > Reviewed-by: mcchou@chromium.org > > > > > > > > > > --- > > > > > > > > > > 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 -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
* [bluez PATCH v1 3/3] client: Fix add advertisement monitor 2020-12-02 1:19 [bluez PATCH v1 0/3] Emit InterfacesAdded/InterfacesRemoved at correct root path Manish Mandlik 2020-12-02 1:19 ` [bluez PATCH v1 1/3] adv_monitor: Register client app with app-base-path Manish Mandlik 2020-12-02 1:19 ` [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app " Manish Mandlik @ 2020-12-02 1:19 ` Manish Mandlik 2020-12-02 17:48 ` Luiz Augusto von Dentz 2 siblings, 1 reply; 11+ messages in thread From: Manish Mandlik @ 2020-12-02 1:19 UTC (permalink / raw) To: marcel, luiz.dentz Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, Manish Mandlik, sonnysasaka, howardchung, mcchou This patch fixes bluetoothctl add-monitor to emit InterfacesAdded/InterfacesRemoved signals correctly. Signed-off-by: Manish Mandlik <mmandlik@google.com> Reviewed-by: sonnysasaka@chromium.org Reviewed-by: howardchung@google.com Reviewed-by: mcchou@chromium.org --- client/adv_monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/adv_monitor.c b/client/adv_monitor.c index 8e81857af..1d1745831 100644 --- a/client/adv_monitor.c +++ b/client/adv_monitor.c @@ -602,8 +602,9 @@ void adv_monitor_add_monitor(DBusConnection *conn, char *type, adv_monitor->patterns = patterns; adv_monitor->path = g_strdup_printf("%s/%hhu", ADV_MONITOR_APP_PATH, adv_mon_idx); - if (g_dbus_register_interface(conn, adv_monitor->path, + if (g_dbus_register_interface_full(conn, adv_monitor->path, ADV_MONITOR_INTERFACE, + ADV_MONITOR_APP_PATH, adv_monitor_methods, NULL, adv_monitor_props, adv_monitor, free_adv_monitor) == FALSE) { -- 2.29.2.454.gaff20da3a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [bluez PATCH v1 3/3] client: Fix add advertisement monitor 2020-12-02 1:19 ` [bluez PATCH v1 3/3] client: Fix add advertisement monitor Manish Mandlik @ 2020-12-02 17:48 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 11+ messages in thread From: Luiz Augusto von Dentz @ 2020-12-02 17:48 UTC (permalink / raw) To: Manish Mandlik Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org, ChromeOS Bluetooth Upstreaming, Sonny Sasaka, Yun-hao Chung, Miao-chen Chou Hi Manish, On Tue, Dec 1, 2020 at 5:20 PM Manish Mandlik <mmandlik@google.com> wrote: > > This patch fixes bluetoothctl add-monitor to emit > InterfacesAdded/InterfacesRemoved signals correctly. > > Signed-off-by: Manish Mandlik <mmandlik@google.com> > Reviewed-by: sonnysasaka@chromium.org > Reviewed-by: howardchung@google.com > Reviewed-by: mcchou@chromium.org > > --- > > client/adv_monitor.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/client/adv_monitor.c b/client/adv_monitor.c > index 8e81857af..1d1745831 100644 > --- a/client/adv_monitor.c > +++ b/client/adv_monitor.c > @@ -602,8 +602,9 @@ void adv_monitor_add_monitor(DBusConnection *conn, char *type, > adv_monitor->patterns = patterns; > adv_monitor->path = g_strdup_printf("%s/%hhu", ADV_MONITOR_APP_PATH, > adv_mon_idx); > - if (g_dbus_register_interface(conn, adv_monitor->path, > + if (g_dbus_register_interface_full(conn, adv_monitor->path, > ADV_MONITOR_INTERFACE, > + ADV_MONITOR_APP_PATH, > adv_monitor_methods, NULL, > adv_monitor_props, adv_monitor, > free_adv_monitor) == FALSE) { > -- > 2.29.2.454.gaff20da3a2-goog Nack, the right fix here is to use the real root path which is '"/" just like is done in client/gatt.c -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-03 1:11 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-02 1:19 [bluez PATCH v1 0/3] Emit InterfacesAdded/InterfacesRemoved at correct root path Manish Mandlik 2020-12-02 1:19 ` [bluez PATCH v1 1/3] adv_monitor: Register client app with app-base-path Manish Mandlik 2020-12-02 1:48 ` Emit InterfacesAdded/InterfacesRemoved at correct root path bluez.test.bot 2020-12-02 1:19 ` [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app " Manish Mandlik 2020-12-02 17:46 ` Luiz Augusto von Dentz 2020-12-02 18:58 ` Sonny Sasaka 2020-12-02 19:50 ` Luiz Augusto von Dentz 2020-12-02 23:27 ` Sonny Sasaka 2020-12-03 1:10 ` Luiz Augusto von Dentz 2020-12-02 1:19 ` [bluez PATCH v1 3/3] client: Fix add advertisement monitor Manish Mandlik 2020-12-02 17:48 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).