Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201202011930.2130671-1-mmandlik@google.com> <20201201171838.bluez.v1.2.Ia29ecb3a2ddfd9e2939c94feca9eca0f06655ad9@changeid>
In-Reply-To: <20201201171838.bluez.v1.2.Ia29ecb3a2ddfd9e2939c94feca9eca0f06655ad9@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 2 Dec 2020 09:46:33 -0800
Message-ID: <CABBYNZJv4EAvECG7j+fgZCpvp9GYRZ45RWcdB5s-ZaSR_uRvSA@mail.gmail.com>
Subject: Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app
 root path
To:     Manish Mandlik <mmandlik@google.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>,
        Yun-hao Chung <howardchung@google.com>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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