Re: [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface

[Miao-chen Chou <mcchou@chromium.org>]

Hi Luiz,

On Tue, Sep 8, 2020 at 10:19 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:30 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > This introduces the org.bluez.AdvertisementMonitorManager1 without
> > implementing handlers of methods and properties.
> >
> > The following test was performed.
> > - Upon adapter registration, the info line of creating an ADV monitor
> > manager gets printed, and system bus emits the interface events of
> > org.bluez.AdvertisementMonitorManager1.
> >
> > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> >  Makefile.am       |   3 +-
> >  src/adapter.c     |  14 +++++
> >  src/adapter.h     |   3 +
> >  src/adv_monitor.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++
> >  src/adv_monitor.h |  32 ++++++++++
> >  5 files changed, 200 insertions(+), 1 deletion(-)
> >  create mode 100644 src/adv_monitor.c
> >  create mode 100644 src/adv_monitor.h
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 7719c06f8..b14ee950e 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -293,7 +293,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
> >                         src/gatt-client.h src/gatt-client.c \
> >                         src/device.h src/device.c \
> >                         src/dbus-common.c src/dbus-common.h \
> > -                       src/eir.h src/eir.c
> > +                       src/eir.h src/eir.c \
> > +                       src/adv_monitor.h src/adv_monitor.c
>
> Id just name it monitor.{c, h}
It'd be preferable to avoid confusion by specifying "adv_" as the
prefix, since there is the other "monitor" (btmon) in BlueZ. Besides,
we also name the corresponding system configuration values by
"adv_monitor".
>
> >  src_bluetoothd_LDADD = lib/libbluetooth-internal.la \
> >                         gdbus/libgdbus-internal.la \
> >                         src/libshared-glib.la \
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 5e896a9f0..41e9de286 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -77,6 +77,7 @@
> >  #include "attrib-server.h"
> >  #include "gatt-database.h"
> >  #include "advertising.h"
> > +#include "adv_monitor.h"
> >  #include "eir.h"
> >
> >  #define ADAPTER_INTERFACE      "org.bluez.Adapter1"
> > @@ -272,6 +273,8 @@ struct btd_adapter {
> >         struct btd_gatt_database *database;
> >         struct btd_adv_manager *adv_manager;
> >
> > +       struct btd_adv_monitor_manager *adv_monitor_manager;
> > +
> >         gboolean initialized;
> >
> >         GSList *pin_callbacks;
> > @@ -6346,6 +6349,9 @@ static void adapter_remove(struct btd_adapter *adapter)
> >         btd_adv_manager_destroy(adapter->adv_manager);
> >         adapter->adv_manager = NULL;
> >
> > +       btd_adv_monitor_manager_destroy(adapter->adv_monitor_manager);
> > +       adapter->adv_monitor_manager = NULL;
> > +
> >         g_slist_free(adapter->pin_callbacks);
> >         adapter->pin_callbacks = NULL;
> >
> > @@ -8623,6 +8629,14 @@ static int adapter_register(struct btd_adapter *adapter)
> >
> >         adapter->adv_manager = btd_adv_manager_new(adapter, adapter->mgmt);
> >
> > +       adapter->adv_monitor_manager = btd_adv_monitor_manager_create(
> > +                                                       adapter, adapter->mgmt);
> > +       if (!adapter->adv_monitor_manager) {
> > +               btd_error(adapter->dev_id,
> > +                       "Failed to create Adv Monitor Manager for adapter");
> > +               return -EINVAL;
> > +       }
> > +
> >         db = btd_gatt_database_get_db(adapter->database);
> >         adapter->db_id = gatt_db_register(db, services_modified,
> >                                                         services_modified,
> > diff --git a/src/adapter.h b/src/adapter.h
> > index f8ac20261..5cb467141 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -26,6 +26,9 @@
> >  #include <dbus/dbus.h>
> >  #include <glib.h>
> >
> > +#include "lib/bluetooth.h"
> > +#include "lib/sdp.h"
> > +
>
> It might be better to have this included locally in a .c file needing them.
>
This fixes the complaint from the compiler where the symbols referred
adapter.h was not found. This was revealed due to the circular
dependency between adv_monitor and adapter where adv_monitor needs to
include adapter.h for calling btd_adapter_get_index(), and adv_monitor
doesn't have these two includes. In other words, adapter.h has been
relying on other headers to include lib/sdp.h and lib/bluetooth.h
which was not a good pattern in the first place. Besides, adapter.h
does refer to symbols from lib/bluetooth.h and lib/sdp.h, so it makes
sense to have them here.
> >  #define MAX_NAME_LENGTH                248
> >
> >  /* Invalid SSP passkey value used to indicate negative replies */
> > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > new file mode 100644
> > index 000000000..7044d3cca
> > --- /dev/null
> > +++ b/src/adv_monitor.c
> > @@ -0,0 +1,149 @@
> > +/*
> > + *
> > + *  BlueZ - Bluetooth protocol stack for Linux
> > + *
> > + *  Copyright (C) 2020 Google LLC
> > + *
> > + *
> > + *  This program is free software; you can redistribute it and/or
> > + *  modify it under the terms of the GNU Lesser General Public
> > + *  License as published by the Free Software Foundation; either
> > + *  version 2.1 of the License, or (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + *  Lesser General Public License for more details.
> > + *
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#define _GNU_SOURCE
> > +#include <stdint.h>
> > +
> > +#include <glib.h>
> > +#include <dbus/dbus.h>
> > +#include <gdbus/gdbus.h>
> > +
> > +#include "adapter.h"
> > +#include "dbus-common.h"
> > +#include "log.h"
> > +#include "src/shared/mgmt.h"
> > +
> > +#include "adv_monitor.h"
> > +
> > +#define ADV_MONITOR_MGR_INTERFACE      "org.bluez.AdvertisementMonitorManager1"
> > +
> > +struct btd_adv_monitor_manager {
> > +       struct btd_adapter *adapter;
> > +       struct mgmt *mgmt;
> > +       uint16_t adapter_id;
> > +       char *path;
> > +};
> > +
> > +static const GDBusMethodTable adv_monitor_methods[] = {
> > +       { GDBUS_METHOD("RegisterMonitor",
> > +                                       GDBUS_ARGS({ "application", "o" }),
> > +                                       NULL, NULL) },
> > +       { GDBUS_ASYNC_METHOD("UnregisterMonitor",
> > +                                       GDBUS_ARGS({ "application", "o" }),
> > +                                       NULL, NULL) },
> > +       { }
> > +};
> > +
> > +static const GDBusPropertyTable adv_monitor_properties[] = {
> > +       {"SupportedMonitorTypes", "as", NULL, NULL, NULL},
> > +       {"SupportedFeatures", "as", NULL, NULL, NULL},
> > +       { }
> > +};
> > +
> > +/* Allocates a manager object */
> > +static struct btd_adv_monitor_manager *manager_new(
> > +                                               struct btd_adapter *adapter,
> > +                                               struct mgmt *mgmt)
> > +{
> > +       struct btd_adv_monitor_manager *manager;
> > +
> > +       if (!adapter || !mgmt)
> > +               return NULL;
> > +
> > +       manager = g_new0(struct btd_adv_monitor_manager, 1);
>
> Use new0.
>
> > +       if (!manager)
> > +               return NULL;
> > +
> > +       manager->adapter = adapter;
> > +       manager->mgmt = mgmt_ref(mgmt);
> > +       manager->adapter_id = btd_adapter_get_index(adapter);
> > +       manager->path = g_strdup(adapter_get_path(manager->adapter));
>
> If we are doing to reference the adapter we don't really need the
> duplicate its path since we can just use adapter_get_path.
As a part of adapter bring-down, the adv monitor manager would be
destroyed too, and we should avoid accessing adapter's resource(s)
during the bring-down to avoid incorrect values. By making a copy of
the path, the code would be less error-prone since the adv monitor
manager does not depend on the unknown state of the adapter.
>
> > +
> > +       return manager;
> > +}
> > +
> > +/* Frees a manager object */
> > +static void manager_free(struct btd_adv_monitor_manager *manager)
> > +{
> > +       manager->adapter = NULL;
>
> No need to assign NULL if you are going to free the whole object at the end.
Done.
>
> > +       mgmt_unref(manager->mgmt);
> > +       manager->mgmt = NULL;
>
> Ditto.
Done.
>
> > +       g_free(manager->path);
> > +       manager->path = NULL;
>
> Ditto.
Done.
>
> > +
> > +       g_free(manager);
> > +}
> > +
> > +/* Destroys a manager object and unregisters its D-Bus interface */
> > +static void manager_destroy(struct btd_adv_monitor_manager *manager)
> > +{
> > +       if (!manager)
> > +               return;
> > +
> > +       g_dbus_unregister_interface(btd_get_dbus_connection(),
> > +                                       manager->path,
> > +                                       ADV_MONITOR_MGR_INTERFACE);
> > +
> > +       manager_free(manager);
> > +}
> > +
> > +/* Creates a manager and registers its D-Bus interface */
> > +struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> > +                                               struct btd_adapter *adapter,
> > +                                               struct mgmt *mgmt)
> > +{
> > +       struct btd_adv_monitor_manager *manager;
> > +
> > +       manager = manager_new(adapter, mgmt);
> > +       if (!manager)
> > +               return NULL;
> > +
> > +       if (g_dbus_register_interface(btd_get_dbus_connection(), manager->path,
> > +                                       ADV_MONITOR_MGR_INTERFACE,
> > +                                       adv_monitor_methods, NULL,
> > +                                       adv_monitor_properties, manager, NULL)
> > +               == FALSE) {
>
> We haven't been consistent with boolean checks but lately we start
> using more the ! form instead of == FALSE.
Done.

Thanks,
Miao


>
> > +               btd_error(manager->adapter_id,
> > +                               "Failed to register "
> > +                               ADV_MONITOR_MGR_INTERFACE);
> > +               manager_free(manager);
> > +               return NULL;
> > +       }
> > +
> > +       btd_info(manager->adapter_id,
> > +                       "Adv Monitor Manager created for adapter %s",
> > +                       adapter_get_path(manager->adapter));
> > +
> > +       return manager;
> > +}
> > +
> > +/* Destroys a manager and unregisters its D-Bus interface */
> > +void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
> > +{
> > +       if (!manager)
> > +               return;
> > +
> > +       btd_info(manager->adapter_id, "Destroy Adv Monitor Manager");
> > +
> > +       manager_destroy(manager);
> > +}
> > diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> > new file mode 100644
> > index 000000000..69ea348f8
> > --- /dev/null
> > +++ b/src/adv_monitor.h
> > @@ -0,0 +1,32 @@
> > +/*
> > + *
> > + *  BlueZ - Bluetooth protocol stack for Linux
> > + *
> > + *  Copyright (C) 2020 Google LLC
> > + *
> > + *
> > + *  This program is free software; you can redistribute it and/or
> > + *  modify it under the terms of the GNU Lesser General Public
> > + *  License as published by the Free Software Foundation; either
> > + *  version 2.1 of the License, or (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + *  Lesser General Public License for more details.
> > + *
> > + */
> > +
> > +#ifndef __ADV_MONITOR_H
> > +#define __ADV_MONITOR_H
> > +
> > +struct mgmt;
> > +struct btd_adapter;
> > +struct btd_adv_monitor_manager;
> > +
> > +struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> > +                                               struct btd_adapter *adapter,
> > +                                               struct mgmt *mgmt);
> > +void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
> > +
> > +#endif /* __ADV_MONITOR_H */
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz