Hi Luiz,
On Tue, Sep 8, 2020 at 10:19 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:30 PM Miao-chen Chou <mcchou@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Reviewed-by: Manish Mandlik <mmandlik@xxxxxxxxxxxx>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > ---
> >
> > 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
