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>
 <CABBYNZJv4EAvECG7j+fgZCpvp9GYRZ45RWcdB5s-ZaSR_uRvSA@mail.gmail.com>
In-Reply-To: <CABBYNZJv4EAvECG7j+fgZCpvp9GYRZ45RWcdB5s-ZaSR_uRvSA@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Wed, 2 Dec 2020 10:58:44 -0800
Message-ID: <CAO271m=hutVbyDZsaoF3u-qt561atD5SF2THRp4WWVkqYCyhrw@mail.gmail.com>
Subject: Re: [bluez PATCH v1 2/3] gdbus: Emit InterfacesAdded/Removed at app
 root path
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Manish Mandlik <mmandlik@google.com>,
        Marcel Holtmann <marcel@holtmann.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Yun-hao Chung <howardchung@google.com>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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