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

Commit Message

Miao-chen Chou Aug. 18, 2020, 10:26 p.m. UTC

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 <[e-mail]>
Reviewed-by: Manish Mandlik <[e-mail]>
Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
---

 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

Comments

[e-mail] Aug. 18, 2020, 10:50 p.m. UTC | #1

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.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkpatch Failed

Outputs:
WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#99: FILE: src/adv_monitor.c:1:
+/*

WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#254: FILE: src/adv_monitor.h:1:
+/*

- total: 0 errors, 2 warnings, 237 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.



---
Regards,
Linux Bluetooth

[e-mail] Aug. 18, 2020, 10:53 p.m. UTC | #2

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.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkbuild Failed

Outputs:
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
src/adv_monitor.c:98:16: error: ‘BT_AD_MAX_DATA_LEN’ undeclared here (not in a function)
   98 |  uint8_t value[BT_AD_MAX_DATA_LEN];
      |                ^~~~~~~~~~~~~~~~~~
src/adv_monitor.c: In function ‘monitor_match’:
src/adv_monitor.c:284:35: error: passing argument 1 of ‘g_dbus_proxy_get_path’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
  284 |  if (strcmp(g_dbus_proxy_get_path(proxy), monitor->path) != 0)
      |                                   ^~~~~
In file included from src/adv_monitor.c:31:
./gdbus/gdbus.h:336:13: note: expected ‘GDBusProxy *’ {aka ‘struct GDBusProxy *’} but argument is of type ‘const GDBusProxy *’ {aka ‘const struct GDBusProxy *’}
  336 | const char *g_dbus_proxy_get_path(GDBusProxy *proxy);
      |             ^~~~~~~~~~~~~~~~~~~~~
src/adv_monitor.c: In function ‘parse_patterns’:
src/adv_monitor.c:493:36: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]
  493 |   if (ad_type > BT_AD_3D_INFO_DATA &&
src/adv_monitor.c:433:11: error: unused variable ‘num_patterns’ [-Werror=unused-variable]
  433 |  uint16_t num_patterns = 0;
      |           ^~~~~~~~~~~~
src/adv_monitor.c: In function ‘get_supported_monitor_types’:
src/adv_monitor.c:761:34: error: unused variable ‘manager’ [-Werror=unused-variable]
  761 |  struct btd_adv_monitor_manager *manager = data;
      |                                  ^~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:9272: src/bluetoothd-adv_monitor.o] Error 1
make: *** [Makefile:4014: all] Error 2



---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz Sept. 8, 2020, 5:19 p.m. UTC | #3

Hi Miao,

On Tue, Aug 18, 2020 at 3:30 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Manish Mandlik <[e-mail]>
> Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> ---
>
>  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}

>  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.

>  #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.

> +
> +       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.

> +       mgmt_unref(manager->mgmt);
> +       manager->mgmt = NULL;

Ditto.

> +       g_free(manager->path);
> +       manager->path = NULL;

Ditto.

> +
> +       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.

> +               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
>

Miao-chen Chou Sept. 10, 2020, 4:52 a.m. UTC | #4

Hi Luiz,

On Tue, Sep 8, 2020 at 10:19 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:30 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> > Reviewed-by: Manish Mandlik <[e-mail]>
> > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > ---
> >
> >  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

Luiz Augusto von Dentz Sept. 10, 2020, 5:31 p.m. UTC | #5

Hi Miao,

On Wed, Sep 9, 2020 at 9:52 PM Miao-chen Chou <[e-mail]> wrote:
>
> Hi Luiz,
>
> On Tue, Sep 8, 2020 at 10:19 AM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Miao,
> >
> > On Tue, Aug 18, 2020 at 3:30 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> > > Reviewed-by: Manish Mandlik <[e-mail]>
> > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > ---
> > >
> > >  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".

Actually btmon is a tool though, but perhaps you are saying it would
conflict with commit tagging/prefixing since we usually use monitor:
for btmon changes, that is a fair point. We could perhaps just with
advertising.c, but Im fine with adv_monitor as well.

> >
> > >  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.

Then let's have a separate patch and clean up the includes so we don't
have includes duplicates everywhere.

> > >  #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.

Normally we do that by having the instance attached to the adapter, so
when the adapter needs to be freed it would cleanup all objects
depending on it so we can guarantee its resources are not released
ahead of time. Note that while we are not focusing too hard in
reducing the footprint of the stack I believe that is a good practice
in the long run, specially when that involve heap allocation.

> >
> > > +
> > > +       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

Miao-chen Chou Sept. 10, 2020, 11:18 p.m. UTC | #6

Hi Luiz,

On Thu, Sep 10, 2020 at 10:31 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Miao,
>
> On Wed, Sep 9, 2020 at 9:52 PM Miao-chen Chou <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, Sep 8, 2020 at 10:19 AM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Miao,
> > >
> > > On Tue, Aug 18, 2020 at 3:30 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> > > > Reviewed-by: Manish Mandlik <[e-mail]>
> > > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > ---
> > > >
> > > >  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".
>
> Actually btmon is a tool though, but perhaps you are saying it would
> conflict with commit tagging/prefixing since we usually use monitor:
> for btmon changes, that is a fair point. We could perhaps just with
> advertising.c, but Im fine with adv_monitor as well.
>
Yes, that's what I meant here. Then let's keep it as 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.
>
> Then let's have a separate patch and clean up the includes so we don't
> have includes duplicates everywhere.
>
I believe these headers are the only two where adapter.h refers to the
symbols. And I will split the changes as a separate one.
> > > >  #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.
>
> Normally we do that by having the instance attached to the adapter, so
> when the adapter needs to be freed it would cleanup all objects
> depending on it so we can guarantee its resources are not released
> ahead of time. Note that while we are not focusing too hard in
> reducing the footprint of the stack I believe that is a good practice
> in the long run, specially when that involve heap allocation.
>
Agree and I also check the procedure of adapter bring-down, the path
would still be valid when invoking btd_adv_monitor_manager_destroy(),
so it should be safe.

> > >
> > > > +
> > > > +       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
>
>
>
> --
> Luiz Augusto von Dentz

Regards,
Miao

Patch

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
 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"
+
 #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);
+	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));
+
+	return manager;
+}
+
+/* Frees a manager object */
+static void manager_free(struct btd_adv_monitor_manager *manager)
+{
+	manager->adapter = NULL;
+	mgmt_unref(manager->mgmt);
+	manager->mgmt = NULL;
+	g_free(manager->path);
+	manager->path = NULL;
+
+	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) {
+		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 */