* [BlueZ PATCH v1 2/7] adv_monitor: Implement Get functions of ADV monitor manager properties
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
@ 2020-08-18 22:26 ` Miao-chen Chou
2020-08-18 22:26 ` [BlueZ PATCH v1 3/7] adv_monitor: Implement RegisterMonitor() Miao-chen Chou
` (7 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-08-18 22:26 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Alain Michaud, Manish Mandlik, Luiz Augusto von Dentz,
Howard Chung, Miao-chen Chou, Abhishek Pandit-Subedi
This implements the Get functions of SupportedMonitorTypes and
SupportedFeatures.
The following test was performed.
- Issue dbus-send to read SupportedMonitorTypes and SupportedFeatures.
Reviewed-by: Yun-Hao Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
src/adv_monitor.c | 130 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 125 insertions(+), 5 deletions(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 7044d3cca..4d02237e8 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -28,10 +28,14 @@
#include <dbus/dbus.h>
#include <gdbus/gdbus.h>
+#include "lib/bluetooth.h"
+#include "lib/mgmt.h"
+
#include "adapter.h"
#include "dbus-common.h"
#include "log.h"
#include "src/shared/mgmt.h"
+#include "src/shared/util.h"
#include "adv_monitor.h"
@@ -42,6 +46,12 @@ struct btd_adv_monitor_manager {
struct mgmt *mgmt;
uint16_t adapter_id;
char *path;
+
+ uint32_t supported_features; /* MGMT_ADV_MONITOR_FEATURE_MASK_* */
+ uint32_t enabled_features; /* MGMT_ADV_MONITOR_FEATURE_MASK_* */
+ uint16_t max_num_monitors;
+ uint8_t max_num_patterns;
+
};
static const GDBusMethodTable adv_monitor_methods[] = {
@@ -54,9 +64,78 @@ static const GDBusMethodTable adv_monitor_methods[] = {
{ }
};
+enum monitor_type {
+ MONITOR_TYPE_OR_PATTERNS,
+};
+
+const struct adv_monitor_type {
+ enum monitor_type type;
+ const char *name;
+} supported_types[] = {
+ { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
+ { },
+};
+
+/* Gets SupportedMonitorTypes property */
+static gboolean get_supported_monitor_types(const GDBusPropertyTable *property,
+ DBusMessageIter *iter,
+ void *data)
+{
+ DBusMessageIter entry;
+ const struct adv_monitor_type *t;
+ struct btd_adv_monitor_manager *manager = data;
+
+ dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+ DBUS_TYPE_STRING_AS_STRING,
+ &entry);
+
+ for (t = supported_types; t->name; t++) {
+ dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
+ &t->name);
+ }
+
+ dbus_message_iter_close_container(iter, &entry);
+
+ return TRUE;
+}
+
+const struct adv_monitor_feature {
+ uint32_t mask;
+ const char *name;
+} supported_features[] = {
+ { MGMT_ADV_MONITOR_FEATURE_MASK_OR_PATTERNS, "controller-patterns" },
+ { }
+};
+
+/* Gets SupportedFeatures property */
+static gboolean get_supported_features(const GDBusPropertyTable *property,
+ DBusMessageIter *iter,
+ void *data)
+{
+ DBusMessageIter entry;
+ const struct adv_monitor_feature *f;
+ struct btd_adv_monitor_manager *manager = data;
+
+ dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+ DBUS_TYPE_STRING_AS_STRING,
+ &entry);
+
+ for (f = supported_features; f->name; f++) {
+ if (manager->supported_features & f->mask) {
+ dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
+ &f->name);
+ }
+ }
+
+ dbus_message_iter_close_container(iter, &entry);
+
+ return TRUE;
+}
+
static const GDBusPropertyTable adv_monitor_properties[] = {
- {"SupportedMonitorTypes", "as", NULL, NULL, NULL},
- {"SupportedFeatures", "as", NULL, NULL, NULL},
+ {"SupportedMonitorTypes", "as", get_supported_monitor_types, NULL,
+ NULL},
+ {"SupportedFeatures", "as", get_supported_features, NULL, NULL},
{ }
};
@@ -107,6 +186,42 @@ static void manager_destroy(struct btd_adv_monitor_manager *manager)
manager_free(manager);
}
+/* Initiates manager's members based on the return of
+ * MGMT_OP_READ_ADV_MONITOR_FEATURES
+ */
+static void read_adv_monitor_features_cb(uint8_t status, uint16_t length,
+ const void *param,
+ void *user_data)
+{
+ const struct mgmt_rp_read_adv_monitor_features *rp = param;
+ struct btd_adv_monitor_manager *manager = user_data;
+
+ if (status != MGMT_STATUS_SUCCESS || !param) {
+ btd_error(manager->adapter_id, "Failed to Read Adv Monitor "
+ "Features with status 0x%02x", status);
+ return;
+ }
+
+ if (length < sizeof(*rp)) {
+ btd_error(manager->adapter_id,
+ "Wrong size of Read Adv Monitor Features "
+ "response");
+ return;
+ }
+
+ manager->supported_features = le32_to_cpu(rp->supported_features);
+ manager->enabled_features = le32_to_cpu(rp->enabled_features);
+ manager->max_num_monitors = le16_to_cpu(rp->max_num_handles);
+ manager->max_num_patterns = rp->max_num_patterns;
+
+ btd_info(manager->adapter_id, "Adv Monitor Manager created with "
+ "supported features:0x%08x, enabled features:0x%08x, "
+ "max number of supported monitors:%d, "
+ "max number of supported patterns:%d",
+ manager->supported_features, manager->enabled_features,
+ manager->max_num_monitors, manager->max_num_patterns);
+}
+
/* Creates a manager and registers its D-Bus interface */
struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
struct btd_adapter *adapter,
@@ -130,9 +245,14 @@ struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
return NULL;
}
- btd_info(manager->adapter_id,
- "Adv Monitor Manager created for adapter %s",
- adapter_get_path(manager->adapter));
+ if (!mgmt_send(manager->mgmt, MGMT_OP_READ_ADV_MONITOR_FEATURES,
+ manager->adapter_id, 0, NULL,
+ read_adv_monitor_features_cb, manager, NULL)) {
+ btd_error(manager->adapter_id,
+ "Failed to send Read Adv Monitor Features");
+ manager_destroy(manager);
+ return NULL;
+ }
return manager;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [BlueZ PATCH v1 3/7] adv_monitor: Implement RegisterMonitor()
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
2020-08-18 22:26 ` [BlueZ PATCH v1 2/7] adv_monitor: Implement Get functions of ADV monitor manager properties Miao-chen Chou
@ 2020-08-18 22:26 ` Miao-chen Chou
2020-09-08 17:24 ` Luiz Augusto von Dentz
2020-08-18 22:26 ` [BlueZ PATCH v1 4/7] adv_monitor: Implement UnregisterMonitor() Miao-chen Chou
` (6 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Miao-chen Chou @ 2020-08-18 22:26 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Alain Michaud, Manish Mandlik, Luiz Augusto von Dentz,
Howard Chung, Miao-chen Chou, Abhishek Pandit-Subedi
This implements the RegisterMonitor() method handler of ADV monitor
manager interface.
The following tests were performed.
- Issue a RegisterMonitor() call with a valid path and expect a
success as return.
- Issue a RegisterMonitor() call with an invalid path and expect
org.bluez.Error.InvalidArguments as return.
- Issue two Registermonitor() calls with the same path and expect
org.bluez.Error.AlreadyExists.
- Verify the values of the registered paths with logging.
- Verify D-Bus disconnection callback was triggered when the client detach
from D-Bus.
Reviewed-by: Yun-Hao Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
src/adv_monitor.c | 167 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 166 insertions(+), 1 deletion(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 4d02237e8..3d27ad18b 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -22,7 +22,9 @@
#endif
#define _GNU_SOURCE
+#include <errno.h>
#include <stdint.h>
+#include <string.h>
#include <glib.h>
#include <dbus/dbus.h>
@@ -34,7 +36,9 @@
#include "adapter.h"
#include "dbus-common.h"
#include "log.h"
+#include "src/error.h"
#include "src/shared/mgmt.h"
+#include "src/shared/queue.h"
#include "src/shared/util.h"
#include "adv_monitor.h"
@@ -52,12 +56,170 @@ struct btd_adv_monitor_manager {
uint16_t max_num_monitors;
uint8_t max_num_patterns;
+ struct queue *apps; /* apps who registered for Adv monitoring */
};
+struct adv_monitor_app {
+ struct btd_adv_monitor_manager *manager;
+ char *owner;
+ char *path;
+
+ DBusMessage *reg;
+ GDBusClient *client;
+};
+
+struct app_match_data {
+ const char *owner;
+ const char *path;
+};
+
+/* Replies to an app's D-Bus message and unref it */
+static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
+{
+ if (!app || !app->reg || !reply)
+ return;
+
+ g_dbus_send_message(btd_get_dbus_connection(), reply);
+ dbus_message_unref(app->reg);
+ app->reg = NULL;
+}
+
+/* Destroys an app object along with related D-Bus handlers */
+static void app_destroy(void *data)
+{
+ struct adv_monitor_app *app = data;
+
+ if (!app)
+ return;
+
+ DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path);
+
+ if (app->reg) {
+ app_reply_msg(app, btd_error_failed(app->reg,
+ "Adv Monitor app destroyed"));
+ }
+
+ if (app->client) {
+ g_dbus_client_set_disconnect_watch(app->client, NULL, NULL);
+ g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL,
+ NULL);
+ g_dbus_client_set_ready_watch(app->client, NULL, NULL);
+ g_dbus_client_unref(app->client);
+ app->client = NULL;
+ }
+
+ g_free(app->owner);
+ app->owner = NULL;
+ g_free(app->path);
+ app->path = NULL;
+
+ g_free(app);
+}
+
+/* Handles a D-Bus disconnection event of an app */
+static void app_disconnect_cb(DBusConnection *conn, void *user_data)
+{
+ struct adv_monitor_app *app = user_data;
+
+ btd_info(app->manager->adapter_id, "Adv Monitor app %s disconnected "
+ "from D-Bus", app->owner);
+ if (app && queue_remove(app->manager->apps, app))
+ app_destroy(app);
+}
+
+/* Creates an app object, initiates it and sets D-Bus event handlers */
+static struct adv_monitor_app *app_create(DBusConnection *conn,
+ const char *sender, const char *path,
+ struct btd_adv_monitor_manager *manager)
+{
+ struct adv_monitor_app *app;
+
+ if (!path || !sender || !manager)
+ return NULL;
+
+ app = g_new0(struct adv_monitor_app, 1);
+ if (!app)
+ return NULL;
+
+ app->owner = g_strdup(sender);
+ app->path = g_strdup(path);
+ app->manager = manager;
+ app->reg = NULL;
+
+ app->client = g_dbus_client_new(conn, sender, path);
+ if (!app->client) {
+ app_destroy(app);
+ return NULL;
+ }
+
+ g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
+ g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
+ g_dbus_client_set_ready_watch(app->client, NULL, NULL);
+
+ return app;
+}
+
+/* Matches an app based on its owner and path */
+static bool app_match(const void *a, const void *b)
+{
+ const struct adv_monitor_app *app = a;
+ const struct app_match_data *match = b;
+
+ if (match->owner && strcmp(app->owner, match->owner))
+ return false;
+
+ if (match->path && strcmp(app->path, match->path))
+ return false;
+
+ return true;
+}
+
+/* Handles a RegisterMonitor D-Bus call */
+static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg,
+ void *user_data)
+{
+ DBusMessageIter args;
+ struct app_match_data match;
+ struct adv_monitor_app *app;
+ struct btd_adv_monitor_manager *manager = user_data;
+
+ if (!dbus_message_iter_init(msg, &args))
+ return btd_error_invalid_args(msg);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+ return btd_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&args, &match.path);
+
+ if (!strlen(match.path) || !g_str_has_prefix(match.path, "/"))
+ return btd_error_invalid_args(msg);
+
+ match.owner = dbus_message_get_sender(msg);
+
+ if (queue_find(manager->apps, app_match, &match))
+ return btd_error_already_exists(msg);
+
+ app = app_create(conn, match.owner, match.path, manager);
+ if (!app) {
+ btd_error(manager->adapter_id,
+ "Failed to reserve %s for Adv Monitor app %s",
+ match.path, match.owner);
+ return btd_error_failed(msg,
+ "Failed to create Adv Monitor app");
+ }
+
+ queue_push_tail(manager->apps, app);
+
+ btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s",
+ match.path, match.owner);
+
+ return dbus_message_new_method_return(msg);
+}
+
static const GDBusMethodTable adv_monitor_methods[] = {
{ GDBUS_METHOD("RegisterMonitor",
GDBUS_ARGS({ "application", "o" }),
- NULL, NULL) },
+ NULL, register_monitor) },
{ GDBUS_ASYNC_METHOD("UnregisterMonitor",
GDBUS_ARGS({ "application", "o" }),
NULL, NULL) },
@@ -157,6 +319,7 @@ static struct btd_adv_monitor_manager *manager_new(
manager->mgmt = mgmt_ref(mgmt);
manager->adapter_id = btd_adapter_get_index(adapter);
manager->path = g_strdup(adapter_get_path(manager->adapter));
+ manager->apps = queue_new();
return manager;
}
@@ -170,6 +333,8 @@ static void manager_free(struct btd_adv_monitor_manager *manager)
g_free(manager->path);
manager->path = NULL;
+ queue_destroy(manager->apps, app_destroy);
+
g_free(manager);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 3/7] adv_monitor: Implement RegisterMonitor()
2020-08-18 22:26 ` [BlueZ PATCH v1 3/7] adv_monitor: Implement RegisterMonitor() Miao-chen Chou
@ 2020-09-08 17:24 ` Luiz Augusto von Dentz
2020-09-10 4:52 ` Miao-chen Chou
0 siblings, 1 reply; 22+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-08 17:24 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung, Abhishek Pandit-Subedi
Hi Miao,
On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> This implements the RegisterMonitor() method handler of ADV monitor
> manager interface.
>
> The following tests were performed.
> - Issue a RegisterMonitor() call with a valid path and expect a
> success as return.
> - Issue a RegisterMonitor() call with an invalid path and expect
> org.bluez.Error.InvalidArguments as return.
> - Issue two Registermonitor() calls with the same path and expect
> org.bluez.Error.AlreadyExists.
> - Verify the values of the registered paths with logging.
> - Verify D-Bus disconnection callback was triggered when the client detach
> from D-Bus.
>
> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> src/adv_monitor.c | 167 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 166 insertions(+), 1 deletion(-)
>
> diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> index 4d02237e8..3d27ad18b 100644
> --- a/src/adv_monitor.c
> +++ b/src/adv_monitor.c
> @@ -22,7 +22,9 @@
> #endif
>
> #define _GNU_SOURCE
> +#include <errno.h>
> #include <stdint.h>
> +#include <string.h>
>
> #include <glib.h>
> #include <dbus/dbus.h>
> @@ -34,7 +36,9 @@
> #include "adapter.h"
> #include "dbus-common.h"
> #include "log.h"
> +#include "src/error.h"
> #include "src/shared/mgmt.h"
> +#include "src/shared/queue.h"
> #include "src/shared/util.h"
>
> #include "adv_monitor.h"
> @@ -52,12 +56,170 @@ struct btd_adv_monitor_manager {
> uint16_t max_num_monitors;
> uint8_t max_num_patterns;
>
> + struct queue *apps; /* apps who registered for Adv monitoring */
> };
>
> +struct adv_monitor_app {
> + struct btd_adv_monitor_manager *manager;
> + char *owner;
> + char *path;
> +
> + DBusMessage *reg;
> + GDBusClient *client;
> +};
> +
> +struct app_match_data {
> + const char *owner;
> + const char *path;
> +};
> +
> +/* Replies to an app's D-Bus message and unref it */
> +static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> +{
> + if (!app || !app->reg || !reply)
> + return;
> +
> + g_dbus_send_message(btd_get_dbus_connection(), reply);
> + dbus_message_unref(app->reg);
> + app->reg = NULL;
> +}
> +
> +/* Destroys an app object along with related D-Bus handlers */
> +static void app_destroy(void *data)
> +{
> + struct adv_monitor_app *app = data;
> +
> + if (!app)
> + return;
> +
> + DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path);
> +
> + if (app->reg) {
> + app_reply_msg(app, btd_error_failed(app->reg,
> + "Adv Monitor app destroyed"));
> + }
> +
> + if (app->client) {
> + g_dbus_client_set_disconnect_watch(app->client, NULL, NULL);
> + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL,
> + NULL);
> + g_dbus_client_set_ready_watch(app->client, NULL, NULL);
> + g_dbus_client_unref(app->client);
> + app->client = NULL;
> + }
> +
> + g_free(app->owner);
> + app->owner = NULL;
> + g_free(app->path);
> + app->path = NULL;
Same comment as before, if the whole object would be free then there
is no need to reset each individual field.
> +
> + g_free(app);
> +}
> +
> +/* Handles a D-Bus disconnection event of an app */
> +static void app_disconnect_cb(DBusConnection *conn, void *user_data)
> +{
> + struct adv_monitor_app *app = user_data;
> +
> + btd_info(app->manager->adapter_id, "Adv Monitor app %s disconnected "
> + "from D-Bus", app->owner);
> + if (app && queue_remove(app->manager->apps, app))
> + app_destroy(app);
> +}
> +
> +/* Creates an app object, initiates it and sets D-Bus event handlers */
> +static struct adv_monitor_app *app_create(DBusConnection *conn,
> + const char *sender, const char *path,
> + struct btd_adv_monitor_manager *manager)
> +{
> + struct adv_monitor_app *app;
> +
> + if (!path || !sender || !manager)
> + return NULL;
> +
> + app = g_new0(struct adv_monitor_app, 1);
Please use new0 instead of g_new0 in new code.
> + if (!app)
> + return NULL;
> +
> + app->owner = g_strdup(sender);
> + app->path = g_strdup(path);
> + app->manager = manager;
> + app->reg = NULL;
> +
> + app->client = g_dbus_client_new(conn, sender, path);
> + if (!app->client) {
> + app_destroy(app);
> + return NULL;
> + }
> +
> + g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
> + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
> + g_dbus_client_set_ready_watch(app->client, NULL, NULL);
> +
> + return app;
> +}
> +
> +/* Matches an app based on its owner and path */
> +static bool app_match(const void *a, const void *b)
> +{
> + const struct adv_monitor_app *app = a;
> + const struct app_match_data *match = b;
> +
> + if (match->owner && strcmp(app->owner, match->owner))
> + return false;
> +
> + if (match->path && strcmp(app->path, match->path))
> + return false;
> +
> + return true;
> +}
> +
> +/* Handles a RegisterMonitor D-Bus call */
> +static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg,
> + void *user_data)
> +{
> + DBusMessageIter args;
> + struct app_match_data match;
> + struct adv_monitor_app *app;
> + struct btd_adv_monitor_manager *manager = user_data;
> +
> + if (!dbus_message_iter_init(msg, &args))
> + return btd_error_invalid_args(msg);
> +
> + if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
> + return btd_error_invalid_args(msg);
> +
> + dbus_message_iter_get_basic(&args, &match.path);
> +
> + if (!strlen(match.path) || !g_str_has_prefix(match.path, "/"))
> + return btd_error_invalid_args(msg);
> +
> + match.owner = dbus_message_get_sender(msg);
> +
> + if (queue_find(manager->apps, app_match, &match))
> + return btd_error_already_exists(msg);
> +
> + app = app_create(conn, match.owner, match.path, manager);
> + if (!app) {
> + btd_error(manager->adapter_id,
> + "Failed to reserve %s for Adv Monitor app %s",
> + match.path, match.owner);
> + return btd_error_failed(msg,
> + "Failed to create Adv Monitor app");
> + }
> +
> + queue_push_tail(manager->apps, app);
> +
> + btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s",
> + match.path, match.owner);
> +
> + return dbus_message_new_method_return(msg);
> +}
> +
> static const GDBusMethodTable adv_monitor_methods[] = {
> { GDBUS_METHOD("RegisterMonitor",
> GDBUS_ARGS({ "application", "o" }),
> - NULL, NULL) },
> + NULL, register_monitor) },
> { GDBUS_ASYNC_METHOD("UnregisterMonitor",
> GDBUS_ARGS({ "application", "o" }),
> NULL, NULL) },
> @@ -157,6 +319,7 @@ static struct btd_adv_monitor_manager *manager_new(
> manager->mgmt = mgmt_ref(mgmt);
> manager->adapter_id = btd_adapter_get_index(adapter);
> manager->path = g_strdup(adapter_get_path(manager->adapter));
> + manager->apps = queue_new();
>
> return manager;
> }
> @@ -170,6 +333,8 @@ static void manager_free(struct btd_adv_monitor_manager *manager)
> g_free(manager->path);
> manager->path = NULL;
>
> + queue_destroy(manager->apps, app_destroy);
> +
> g_free(manager);
> }
>
> --
> 2.26.2
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 3/7] adv_monitor: Implement RegisterMonitor()
2020-09-08 17:24 ` Luiz Augusto von Dentz
@ 2020-09-10 4:52 ` Miao-chen Chou
0 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-09-10 4:52 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung, Abhishek Pandit-Subedi
Hi Luiz,
On Tue, Sep 8, 2020 at 10:25 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > This implements the RegisterMonitor() method handler of ADV monitor
> > manager interface.
> >
> > The following tests were performed.
> > - Issue a RegisterMonitor() call with a valid path and expect a
> > success as return.
> > - Issue a RegisterMonitor() call with an invalid path and expect
> > org.bluez.Error.InvalidArguments as return.
> > - Issue two Registermonitor() calls with the same path and expect
> > org.bluez.Error.AlreadyExists.
> > - Verify the values of the registered paths with logging.
> > - Verify D-Bus disconnection callback was triggered when the client detach
> > from D-Bus.
> >
> > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > src/adv_monitor.c | 167 +++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 166 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > index 4d02237e8..3d27ad18b 100644
> > --- a/src/adv_monitor.c
> > +++ b/src/adv_monitor.c
> > @@ -22,7 +22,9 @@
> > #endif
> >
> > #define _GNU_SOURCE
> > +#include <errno.h>
> > #include <stdint.h>
> > +#include <string.h>
> >
> > #include <glib.h>
> > #include <dbus/dbus.h>
> > @@ -34,7 +36,9 @@
> > #include "adapter.h"
> > #include "dbus-common.h"
> > #include "log.h"
> > +#include "src/error.h"
> > #include "src/shared/mgmt.h"
> > +#include "src/shared/queue.h"
> > #include "src/shared/util.h"
> >
> > #include "adv_monitor.h"
> > @@ -52,12 +56,170 @@ struct btd_adv_monitor_manager {
> > uint16_t max_num_monitors;
> > uint8_t max_num_patterns;
> >
> > + struct queue *apps; /* apps who registered for Adv monitoring */
> > };
> >
> > +struct adv_monitor_app {
> > + struct btd_adv_monitor_manager *manager;
> > + char *owner;
> > + char *path;
> > +
> > + DBusMessage *reg;
> > + GDBusClient *client;
> > +};
> > +
> > +struct app_match_data {
> > + const char *owner;
> > + const char *path;
> > +};
> > +
> > +/* Replies to an app's D-Bus message and unref it */
> > +static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> > +{
> > + if (!app || !app->reg || !reply)
> > + return;
> > +
> > + g_dbus_send_message(btd_get_dbus_connection(), reply);
> > + dbus_message_unref(app->reg);
> > + app->reg = NULL;
> > +}
> > +
> > +/* Destroys an app object along with related D-Bus handlers */
> > +static void app_destroy(void *data)
> > +{
> > + struct adv_monitor_app *app = data;
> > +
> > + if (!app)
> > + return;
> > +
> > + DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path);
> > +
> > + if (app->reg) {
> > + app_reply_msg(app, btd_error_failed(app->reg,
> > + "Adv Monitor app destroyed"));
> > + }
> > +
> > + if (app->client) {
> > + g_dbus_client_set_disconnect_watch(app->client, NULL, NULL);
> > + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL,
> > + NULL);
> > + g_dbus_client_set_ready_watch(app->client, NULL, NULL);
> > + g_dbus_client_unref(app->client);
> > + app->client = NULL;
> > + }
> > +
> > + g_free(app->owner);
> > + app->owner = NULL;
> > + g_free(app->path);
> > + app->path = NULL;
>
> Same comment as before, if the whole object would be free then there
> is no need to reset each individual field.
Done.
>
> > +
> > + g_free(app);
> > +}
> > +
> > +/* Handles a D-Bus disconnection event of an app */
> > +static void app_disconnect_cb(DBusConnection *conn, void *user_data)
> > +{
> > + struct adv_monitor_app *app = user_data;
> > +
> > + btd_info(app->manager->adapter_id, "Adv Monitor app %s disconnected "
> > + "from D-Bus", app->owner);
> > + if (app && queue_remove(app->manager->apps, app))
> > + app_destroy(app);
> > +}
> > +
> > +/* Creates an app object, initiates it and sets D-Bus event handlers */
> > +static struct adv_monitor_app *app_create(DBusConnection *conn,
> > + const char *sender, const char *path,
> > + struct btd_adv_monitor_manager *manager)
> > +{
> > + struct adv_monitor_app *app;
> > +
> > + if (!path || !sender || !manager)
> > + return NULL;
> > +
> > + app = g_new0(struct adv_monitor_app, 1);
>
> Please use new0 instead of g_new0 in new code.
Done.
>
> > + if (!app)
> > + return NULL;
> > +
> > + app->owner = g_strdup(sender);
> > + app->path = g_strdup(path);
> > + app->manager = manager;
> > + app->reg = NULL;
> > +
> > + app->client = g_dbus_client_new(conn, sender, path);
> > + if (!app->client) {
> > + app_destroy(app);
> > + return NULL;
> > + }
> > +
> > + g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
> > + g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
> > + g_dbus_client_set_ready_watch(app->client, NULL, NULL);
> > +
> > + return app;
> > +}
> > +
> > +/* Matches an app based on its owner and path */
> > +static bool app_match(const void *a, const void *b)
> > +{
> > + const struct adv_monitor_app *app = a;
> > + const struct app_match_data *match = b;
> > +
> > + if (match->owner && strcmp(app->owner, match->owner))
> > + return false;
> > +
> > + if (match->path && strcmp(app->path, match->path))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/* Handles a RegisterMonitor D-Bus call */
> > +static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg,
> > + void *user_data)
> > +{
> > + DBusMessageIter args;
> > + struct app_match_data match;
> > + struct adv_monitor_app *app;
> > + struct btd_adv_monitor_manager *manager = user_data;
> > +
> > + if (!dbus_message_iter_init(msg, &args))
> > + return btd_error_invalid_args(msg);
> > +
> > + if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
> > + return btd_error_invalid_args(msg);
> > +
> > + dbus_message_iter_get_basic(&args, &match.path);
> > +
> > + if (!strlen(match.path) || !g_str_has_prefix(match.path, "/"))
> > + return btd_error_invalid_args(msg);
> > +
> > + match.owner = dbus_message_get_sender(msg);
> > +
> > + if (queue_find(manager->apps, app_match, &match))
> > + return btd_error_already_exists(msg);
> > +
> > + app = app_create(conn, match.owner, match.path, manager);
> > + if (!app) {
> > + btd_error(manager->adapter_id,
> > + "Failed to reserve %s for Adv Monitor app %s",
> > + match.path, match.owner);
> > + return btd_error_failed(msg,
> > + "Failed to create Adv Monitor app");
> > + }
> > +
> > + queue_push_tail(manager->apps, app);
> > +
> > + btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s",
> > + match.path, match.owner);
> > +
> > + return dbus_message_new_method_return(msg);
> > +}
> > +
> > static const GDBusMethodTable adv_monitor_methods[] = {
> > { GDBUS_METHOD("RegisterMonitor",
> > GDBUS_ARGS({ "application", "o" }),
> > - NULL, NULL) },
> > + NULL, register_monitor) },
> > { GDBUS_ASYNC_METHOD("UnregisterMonitor",
> > GDBUS_ARGS({ "application", "o" }),
> > NULL, NULL) },
> > @@ -157,6 +319,7 @@ static struct btd_adv_monitor_manager *manager_new(
> > manager->mgmt = mgmt_ref(mgmt);
> > manager->adapter_id = btd_adapter_get_index(adapter);
> > manager->path = g_strdup(adapter_get_path(manager->adapter));
> > + manager->apps = queue_new();
> >
> > return manager;
> > }
> > @@ -170,6 +333,8 @@ static void manager_free(struct btd_adv_monitor_manager *manager)
> > g_free(manager->path);
> > manager->path = NULL;
> >
> > + queue_destroy(manager->apps, app_destroy);
> > +
> > g_free(manager);
> > }
> >
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Miao
^ permalink raw reply [flat|nested] 22+ messages in thread
* [BlueZ PATCH v1 4/7] adv_monitor: Implement UnregisterMonitor()
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
2020-08-18 22:26 ` [BlueZ PATCH v1 2/7] adv_monitor: Implement Get functions of ADV monitor manager properties Miao-chen Chou
2020-08-18 22:26 ` [BlueZ PATCH v1 3/7] adv_monitor: Implement RegisterMonitor() Miao-chen Chou
@ 2020-08-18 22:26 ` Miao-chen Chou
2020-08-18 22:26 ` [BlueZ PATCH v1 5/7] adv_monitor: Handle D-Bus client ready events Miao-chen Chou
` (5 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-08-18 22:26 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Alain Michaud, Manish Mandlik, Luiz Augusto von Dentz,
Howard Chung, Miao-chen Chou, Abhishek Pandit-Subedi
This implements the UnregisterMonitor() method handler of ADV monitor
manager interface.
The following tests were performed.
- Issue a UnregisterMonitor() call with a nonexistent path and expect
org.bluez.Error.DoesNotExist as the return.
- Issue a UnregisterMonitor() call with a invalid path and expect
org.bluez.Error.InvalidArguments as the return.
- Issue RegisterMonitor() with a path, issue UnregisterMonitor() and
expect a successful method call return.
Reviewed-by: Yun-Hao Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
src/adv_monitor.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 3d27ad18b..a9e2e4421 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -216,13 +216,48 @@ static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg,
return dbus_message_new_method_return(msg);
}
+/* Handles UnregisterMonitor D-Bus call */
+static DBusMessage *unregister_monitor(DBusConnection *conn,
+ DBusMessage *msg, void *user_data)
+{
+ DBusMessageIter args;
+ struct app_match_data match;
+ struct adv_monitor_app *app;
+ struct btd_adv_monitor_manager *manager = user_data;
+
+ if (!dbus_message_iter_init(msg, &args))
+ return btd_error_invalid_args(msg);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+ return btd_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&args, &match.path);
+
+ if (!strlen(match.path) || !g_str_has_prefix(match.path, "/"))
+ return btd_error_invalid_args(msg);
+
+ match.owner = dbus_message_get_sender(msg);
+
+ app = queue_find(manager->apps, app_match, &match);
+ if (!app)
+ return btd_error_does_not_exist(msg);
+
+ queue_remove(manager->apps, app);
+ app_destroy(app);
+
+ btd_info(manager->adapter_id, "Path %s removed along with Adv Monitor "
+ "app %s", match.path, match.owner);
+
+ return dbus_message_new_method_return(msg);
+}
+
static const GDBusMethodTable adv_monitor_methods[] = {
{ GDBUS_METHOD("RegisterMonitor",
GDBUS_ARGS({ "application", "o" }),
NULL, register_monitor) },
{ GDBUS_ASYNC_METHOD("UnregisterMonitor",
GDBUS_ARGS({ "application", "o" }),
- NULL, NULL) },
+ NULL, unregister_monitor) },
{ }
};
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [BlueZ PATCH v1 5/7] adv_monitor: Handle D-Bus client ready events
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
` (2 preceding siblings ...)
2020-08-18 22:26 ` [BlueZ PATCH v1 4/7] adv_monitor: Implement UnregisterMonitor() Miao-chen Chou
@ 2020-08-18 22:26 ` Miao-chen Chou
2020-08-18 22:26 ` [BlueZ PATCH v1 6/7] adv_monitor: Handle D-Bus proxy event of an ADV monitor Miao-chen Chou
` (4 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-08-18 22:26 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Alain Michaud, Manish Mandlik, Luiz Augusto von Dentz,
Howard Chung, Miao-chen Chou
This adds a handler of client ready events. The handler replies to the
RegisterMonitor() method call.
The following tests were performed.
- Call RegisterMonitor() and expect to receive a return.
Reviewed-by: Yun-Hao Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
src/adv_monitor.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index a9e2e4421..b5ea5ee99 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -127,9 +127,22 @@ static void app_disconnect_cb(DBusConnection *conn, void *user_data)
app_destroy(app);
}
+/* Handles the ready signal of Adv Monitor app */
+static void app_ready_cb(GDBusClient *client, void *user_data)
+{
+ struct adv_monitor_app *app = user_data;
+ uint16_t adapter_id = app->manager->adapter_id;
+
+ btd_info(adapter_id, "Path %s reserved for Adv Monitor app %s",
+ app->path, app->owner);
+
+ app_reply_msg(app, dbus_message_new_method_return(app->reg));
+}
+
/* Creates an app object, initiates it and sets D-Bus event handlers */
static struct adv_monitor_app *app_create(DBusConnection *conn,
- const char *sender, const char *path,
+ DBusMessage *msg, const char *sender,
+ const char *path,
struct btd_adv_monitor_manager *manager)
{
struct adv_monitor_app *app;
@@ -154,7 +167,9 @@ static struct adv_monitor_app *app_create(DBusConnection *conn,
g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
- g_dbus_client_set_ready_watch(app->client, NULL, NULL);
+ g_dbus_client_set_ready_watch(app->client, app_ready_cb, app);
+
+ app->reg = dbus_message_ref(msg);
return app;
}
@@ -199,7 +214,7 @@ static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg,
if (queue_find(manager->apps, app_match, &match))
return btd_error_already_exists(msg);
- app = app_create(conn, match.owner, match.path, manager);
+ app = app_create(conn, msg, match.owner, match.path, manager);
if (!app) {
btd_error(manager->adapter_id,
"Failed to reserve %s for Adv Monitor app %s",
@@ -210,10 +225,7 @@ static DBusMessage *register_monitor(DBusConnection *conn, DBusMessage *msg,
queue_push_tail(manager->apps, app);
- btd_info(manager->adapter_id, "Path %s reserved for Adv Monitor app %s",
- match.path, match.owner);
-
- return dbus_message_new_method_return(msg);
+ return NULL;
}
/* Handles UnregisterMonitor D-Bus call */
@@ -252,7 +264,7 @@ static DBusMessage *unregister_monitor(DBusConnection *conn,
}
static const GDBusMethodTable adv_monitor_methods[] = {
- { GDBUS_METHOD("RegisterMonitor",
+ { GDBUS_ASYNC_METHOD("RegisterMonitor",
GDBUS_ARGS({ "application", "o" }),
NULL, register_monitor) },
{ GDBUS_ASYNC_METHOD("UnregisterMonitor",
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [BlueZ PATCH v1 6/7] adv_monitor: Handle D-Bus proxy event of an ADV monitor
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
` (3 preceding siblings ...)
2020-08-18 22:26 ` [BlueZ PATCH v1 5/7] adv_monitor: Handle D-Bus client ready events Miao-chen Chou
@ 2020-08-18 22:26 ` Miao-chen Chou
2020-09-08 17:35 ` Luiz Augusto von Dentz
2020-08-18 22:26 ` [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description Miao-chen Chou
` (3 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Miao-chen Chou @ 2020-08-18 22:26 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Alain Michaud, Manish Mandlik, Luiz Augusto von Dentz,
Howard Chung, Miao-chen Chou
This adds two handlers, one for adding and one for removing, of D-Bus proxy
events. The handler of property changes is set to NULL as intended,
since for simplicity no further changes on monitor's properties would
affect the ongoing monitoring.
The following test steps were performed with bluetoothctl.
- After registering the root path, expose two monitors and verify that
the proxy added event are received.
- Have two monitors added, unexpose the monitors, and verify that the
proxy removed events are received for those two monitors.
- Have two monitors added, unregister the monitors and verify that the
proxy removed events are received for those two monitors.
Reviewed-by: Yun-Hao Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
src/adv_monitor.c | 492 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 479 insertions(+), 13 deletions(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index b5ea5ee99..23fbc2b45 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -37,14 +37,23 @@
#include "dbus-common.h"
#include "log.h"
#include "src/error.h"
+#include "src/shared/ad.h"
#include "src/shared/mgmt.h"
#include "src/shared/queue.h"
#include "src/shared/util.h"
#include "adv_monitor.h"
+#define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
#define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
+#define ADV_MONITOR_UNSET_RSSI 127 /* dBm */
+#define ADV_MONITOR_MAX_RSSI 20 /* dBm */
+#define ADV_MONITOR_MIN_RSSI -127 /* dBm */
+#define ADV_MONITOR_UNSET_TIMER 0 /* second */
+#define ADV_MONITOR_MIN_TIMER 1 /* second */
+#define ADV_MONITOR_MAX_TIMER 300 /* second */
+
struct btd_adv_monitor_manager {
struct btd_adapter *adapter;
struct mgmt *mgmt;
@@ -66,6 +75,43 @@ struct adv_monitor_app {
DBusMessage *reg;
GDBusClient *client;
+
+ struct queue *monitors;
+};
+
+enum monitor_type {
+ MONITOR_TYPE_NONE,
+ MONITOR_TYPE_OR_PATTERNS,
+};
+
+enum monitor_state {
+ MONITOR_STATE_NEW, /* New but not yet init'ed with actual values */
+ MONITOR_STATE_FAILED, /* Failed to be init'ed */
+ MONITOR_STATE_INITED, /* Init'ed but not yet sent to kernel */
+ MONITOR_STATE_HONORED, /* Accepted by kernel */
+};
+
+struct pattern {
+ uint8_t ad_type;
+ uint8_t offset;
+ uint8_t length;
+ uint8_t value[BT_AD_MAX_DATA_LEN];
+};
+
+struct adv_monitor {
+ struct adv_monitor_app *app;
+ GDBusProxy *proxy;
+ char *path;
+
+ enum monitor_state state; /* MONITOR_STATE_* */
+
+ int8_t high_rssi; /* high RSSI threshold */
+ uint16_t high_rssi_timeout; /* high RSSI threshold timeout */
+ int8_t low_rssi; /* low RSSI threshold */
+ uint16_t low_rssi_timeout; /* low RSSI threshold timeout */
+
+ enum monitor_type type; /* MONITOR_TYPE_* */
+ struct queue *patterns;
};
struct app_match_data {
@@ -73,6 +119,14 @@ struct app_match_data {
const char *path;
};
+const struct adv_monitor_type {
+ enum monitor_type type;
+ const char *name;
+} supported_types[] = {
+ { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
+ { },
+};
+
/* Replies to an app's D-Bus message and unref it */
static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
{
@@ -84,6 +138,52 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
app->reg = NULL;
}
+/* Frees a pattern */
+static void pattern_free(void *data)
+{
+ struct pattern *pattern = data;
+
+ if (!pattern)
+ return;
+
+ g_free(pattern);
+}
+
+/* Frees a monitor object */
+static void monitor_free(void *data)
+{
+ struct adv_monitor *monitor = data;
+
+ if (!monitor)
+ return;
+
+ monitor->app = NULL;
+ g_dbus_proxy_unref(monitor->proxy);
+ monitor->proxy = NULL;
+ g_free(monitor->path);
+ monitor->path = NULL;
+
+ queue_destroy(monitor->patterns, pattern_free);
+ monitor->patterns = NULL;
+
+ g_free(monitor);
+}
+
+/* Calls Release() method of the remote Adv Monitor */
+static void monitor_release(void *data, void *user_data)
+{
+ struct adv_monitor *monitor = data;
+
+ if (!monitor)
+ return;
+
+ DBG("Calling Release() on Adv Monitor of owner %s at path %s",
+ monitor->app->owner, monitor->path);
+
+ g_dbus_proxy_method_call(monitor->proxy, "Release", NULL, NULL, NULL,
+ NULL);
+}
+
/* Destroys an app object along with related D-Bus handlers */
static void app_destroy(void *data)
{
@@ -94,6 +194,9 @@ static void app_destroy(void *data)
DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path);
+ queue_foreach(app->monitors, monitor_release, NULL);
+ queue_destroy(app->monitors, monitor_free);
+
if (app->reg) {
app_reply_msg(app, btd_error_failed(app->reg,
"Adv Monitor app destroyed"));
@@ -139,6 +242,372 @@ static void app_ready_cb(GDBusClient *client, void *user_data)
app_reply_msg(app, dbus_message_new_method_return(app->reg));
}
+/* Allocates an Adv Monitor */
+static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
+ GDBusProxy *proxy)
+{
+ struct adv_monitor *monitor;
+
+ if (!app || !proxy)
+ return NULL;
+
+ monitor = g_new0(struct adv_monitor, 1);
+ if (!monitor)
+ return NULL;
+
+ monitor->app = app;
+ monitor->proxy = g_dbus_proxy_ref(proxy);
+ monitor->path = g_strdup(g_dbus_proxy_get_path(proxy));
+
+ monitor->state = MONITOR_STATE_NEW;
+
+ monitor->high_rssi = ADV_MONITOR_UNSET_RSSI;
+ monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
+ monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
+ monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
+
+ monitor->type = MONITOR_TYPE_NONE;
+ monitor->patterns = NULL;
+
+ return monitor;
+}
+
+/* Matches a monitor based on its D-Bus path */
+static bool monitor_match(const void *a, const void *b)
+{
+ const GDBusProxy *proxy = b;
+ const struct adv_monitor *monitor = a;
+
+ if (!proxy || !monitor)
+ return false;
+
+ if (strcmp(g_dbus_proxy_get_path(proxy), monitor->path) != 0)
+ return false;
+
+ return true;
+}
+
+/* Retrieves Type from the remote Adv Monitor object, verifies the value and
+ * update the local Adv Monitor
+ */
+static bool parse_monitor_type(struct adv_monitor *monitor, const char *path)
+{
+ DBusMessageIter iter;
+ const struct adv_monitor_type *t;
+ const char *type_str;
+ uint16_t adapter_id = monitor->app->manager->adapter_id;
+
+ if (!g_dbus_proxy_get_property(monitor->proxy, "Type", &iter)) {
+ btd_error(adapter_id, "Failed to retrieve property Type from "
+ "the Adv Monitor at path %s", path);
+ return false;
+ }
+
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+ goto failed;
+
+ dbus_message_iter_get_basic(&iter, &type_str);
+
+ for (t = supported_types; t->name; t++) {
+ if (strcmp(t->name, type_str) == 0) {
+ monitor->type = t->type;
+ return true;
+ }
+ }
+
+failed:
+ btd_error(adapter_id, "Invalid argument of property Type of the Adv "
+ "Monitor at path %s", path);
+
+ return false;
+}
+
+/* Retrieves RSSIThresholdsAndTimers from the remote Adv Monitor object,
+ * verifies the values and update the local Adv Monitor
+ */
+static bool parse_rssi_and_timeout(struct adv_monitor *monitor,
+ const char *path)
+{
+ DBusMessageIter prop_struct, iter;
+ int16_t h_rssi, l_rssi;
+ uint16_t h_rssi_timer, l_rssi_timer;
+ uint16_t adapter_id = monitor->app->manager->adapter_id;
+
+ /* Property RSSIThresholdsAndTimers is optional */
+ if (!g_dbus_proxy_get_property(monitor->proxy,
+ "RSSIThresholdsAndTimers",
+ &prop_struct)) {
+ DBG("Adv Monitor at path %s provides no RSSI thresholds and "
+ "timeouts", path);
+ return true;
+ }
+
+ if (dbus_message_iter_get_arg_type(&prop_struct) != DBUS_TYPE_STRUCT)
+ goto failed;
+
+ dbus_message_iter_recurse(&prop_struct, &iter);
+
+ /* Extract HighRSSIThreshold */
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT16)
+ goto failed;
+ dbus_message_iter_get_basic(&iter, &h_rssi);
+ if (!dbus_message_iter_next(&iter))
+ goto failed;
+
+ /* Extract HighRSSIThresholdTimer */
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_UINT16)
+ goto failed;
+ dbus_message_iter_get_basic(&iter, &h_rssi_timer);
+ if (!dbus_message_iter_next(&iter))
+ goto failed;
+
+ /* Extract LowRSSIThreshold */
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT16)
+ goto failed;
+ dbus_message_iter_get_basic(&iter, &l_rssi);
+ if (!dbus_message_iter_next(&iter))
+ goto failed;
+
+ /* Extract LowRSSIThresholdTimer */
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_UINT16)
+ goto failed;
+ dbus_message_iter_get_basic(&iter, &l_rssi_timer);
+
+ /* Verify the values of RSSIs and their timers. For simplicity, we
+ * enforce the all-or-none rule to these fields. In other words, either
+ * all are set to the unset values or all are set within valid ranges.
+ */
+ if (h_rssi == ADV_MONITOR_UNSET_RSSI &&
+ l_rssi == ADV_MONITOR_UNSET_RSSI &&
+ h_rssi_timer == ADV_MONITOR_UNSET_TIMER &&
+ l_rssi_timer == ADV_MONITOR_UNSET_TIMER) {
+ goto done;
+ }
+
+ if (h_rssi < ADV_MONITOR_MIN_RSSI || h_rssi > ADV_MONITOR_MAX_RSSI ||
+ l_rssi < ADV_MONITOR_MIN_RSSI ||
+ l_rssi > ADV_MONITOR_MAX_RSSI || h_rssi <= l_rssi) {
+ goto failed;
+ }
+
+ if (h_rssi_timer < ADV_MONITOR_MIN_TIMER ||
+ h_rssi_timer > ADV_MONITOR_MAX_TIMER ||
+ l_rssi_timer < ADV_MONITOR_MIN_TIMER ||
+ l_rssi_timer > ADV_MONITOR_MAX_TIMER) {
+ goto failed;
+ }
+
+ monitor->high_rssi = h_rssi;
+ monitor->low_rssi = l_rssi;
+ monitor->high_rssi_timeout = h_rssi_timer;
+ monitor->low_rssi_timeout = l_rssi_timer;
+
+done:
+ DBG("Adv Monitor at %s initiated with high RSSI threshold %d, high "
+ "RSSI threshold timeout %d, low RSSI threshold %d, low RSSI "
+ "threshold timeout %d", path, monitor->high_rssi,
+ monitor->high_rssi_timeout, monitor->low_rssi,
+ monitor->low_rssi_timeout);
+
+ return true;
+
+failed:
+ monitor->high_rssi = ADV_MONITOR_UNSET_RSSI;
+ monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
+ monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
+ monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
+
+ btd_error(adapter_id, "Invalid argument of property "
+ "RSSIThresholdsAndTimers of the Adv Monitor at path %s",
+ path);
+
+ return false;
+}
+
+/* Retrieves Patterns from the remote Adv Monitor object, verifies the values
+ * and update the local Adv Monitor
+ */
+static bool parse_patterns(struct adv_monitor *monitor, const char *path)
+{
+ DBusMessageIter array, array_iter;
+ uint16_t num_patterns = 0;
+ uint16_t adapter_id = monitor->app->manager->adapter_id;
+
+ if (!g_dbus_proxy_get_property(monitor->proxy, "Patterns", &array)) {
+ btd_error(adapter_id, "Failed to retrieve property Patterns "
+ "from the Adv Monitor at path %s", path);
+ return false;
+ }
+
+ monitor->patterns = queue_new();
+
+ if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_ARRAY ||
+ dbus_message_iter_get_element_type(&array) !=
+ DBUS_TYPE_STRUCT) {
+ goto failed;
+ }
+
+ dbus_message_iter_recurse(&array, &array_iter);
+
+ while (dbus_message_iter_get_arg_type(&array_iter) ==
+ DBUS_TYPE_STRUCT) {
+ int value_len;
+ uint8_t *value;
+ uint8_t offset, ad_type;
+ struct pattern *pattern;
+ DBusMessageIter struct_iter, value_iter;
+
+ dbus_message_iter_recurse(&array_iter, &struct_iter);
+
+ // Extract start position
+ if (dbus_message_iter_get_arg_type(&struct_iter) !=
+ DBUS_TYPE_BYTE) {
+ goto failed;
+ }
+ dbus_message_iter_get_basic(&struct_iter, &offset);
+ if (!dbus_message_iter_next(&struct_iter))
+ goto failed;
+
+ // Extract AD data type
+ if (dbus_message_iter_get_arg_type(&struct_iter) !=
+ DBUS_TYPE_BYTE) {
+ goto failed;
+ }
+ dbus_message_iter_get_basic(&struct_iter, &ad_type);
+ if (!dbus_message_iter_next(&struct_iter))
+ goto failed;
+
+ // Extract value of a pattern
+ if (dbus_message_iter_get_arg_type(&struct_iter) !=
+ DBUS_TYPE_ARRAY) {
+ goto failed;
+ }
+ dbus_message_iter_recurse(&struct_iter, &value_iter);
+ dbus_message_iter_get_fixed_array(&value_iter, &value,
+ &value_len);
+
+ // Verify the values
+ if (offset > BT_AD_MAX_DATA_LEN - 1)
+ goto failed;
+
+ if (ad_type > BT_AD_3D_INFO_DATA &&
+ ad_type != BT_AD_MANUFACTURER_DATA
+ || ad_type < BT_AD_FLAGS) {
+ goto failed;
+ }
+
+ if (!value || value_len <= 0 || value_len > BT_AD_MAX_DATA_LEN)
+ goto failed;
+
+ pattern = g_new0(struct pattern, 1);
+ if (!pattern)
+ goto failed;
+
+ pattern->ad_type = ad_type;
+ pattern->offset = offset;
+ pattern->length = value_len;
+ g_memmove(pattern->value, value, pattern->length);
+
+ queue_push_tail(monitor->patterns, pattern);
+
+ dbus_message_iter_next(&array_iter);
+ }
+
+ /* There must be at least one pattern. */
+ if (queue_isempty(monitor->patterns))
+ goto failed;
+
+ return true;
+
+failed:
+ queue_destroy(monitor->patterns, pattern_free);
+ monitor->patterns = NULL;
+
+ btd_error(adapter_id, "Invalid argument of property Patterns of the "
+ "Adv Monitor at path %s", path);
+
+ return false;
+}
+
+/* Processes the content of the remote Adv Monitor */
+static bool monitor_process(struct adv_monitor *monitor,
+ struct adv_monitor_app *app)
+{
+ const char *path = g_dbus_proxy_get_path(monitor->proxy);
+
+ monitor->state = MONITOR_STATE_FAILED;
+
+ if (!parse_monitor_type(monitor, path))
+ goto done;
+
+ if (!parse_rssi_and_timeout(monitor, path))
+ goto done;
+
+ if (monitor->type == MONITOR_TYPE_OR_PATTERNS &&
+ parse_patterns(monitor, path)) {
+ monitor->state = MONITOR_STATE_INITED;
+ }
+
+done:
+ return monitor->state != MONITOR_STATE_FAILED;
+}
+
+/* Handles an Adv Monitor D-Bus proxy added event */
+static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
+{
+ struct adv_monitor *monitor;
+ struct adv_monitor_app *app = user_data;
+ uint16_t adapter_id = app->manager->adapter_id;
+ const char *path = g_dbus_proxy_get_path(proxy);
+ const char *iface = g_dbus_proxy_get_interface(proxy);
+
+ if (strcmp(iface, ADV_MONITOR_INTERFACE) != 0 ||
+ !g_str_has_prefix(path, app->path)) {
+ return;
+ }
+
+ if (queue_find(app->monitors, monitor_match, proxy)) {
+ btd_error(adapter_id, "Adv Monitor proxy already exists with "
+ "path %s", path);
+ return;
+ }
+
+ monitor = monitor_new(app, proxy);
+ if (!monitor) {
+ btd_error(adapter_id, "Failed to allocate an Adv Monitor for "
+ "the object at %s", path);
+ return;
+ }
+
+ if (!monitor_process(monitor, app)) {
+ monitor_release(monitor, NULL);
+ monitor_free(monitor);
+ DBG("Adv Monitor at path %s released due to invalid content",
+ path);
+ return;
+ }
+
+ queue_push_tail(app->monitors, monitor);
+
+ DBG("Adv Monitor allocated for the object at path %s", path);
+}
+
+/* Handles the removal of an Adv Monitor D-Bus proxy */
+static void monitor_proxy_removed_cb(GDBusProxy *proxy, void *user_data)
+{
+ struct adv_monitor *monitor;
+ struct adv_monitor_app *app = user_data;
+
+ monitor = queue_remove_if(app->monitors, monitor_match, proxy);
+ if (monitor) {
+ DBG("Adv Monitor removed for the object at path %s",
+ monitor->path);
+
+ /* The object was gone, so we don't need to call Release() */
+ monitor_free(monitor);
+ }
+}
+
/* Creates an app object, initiates it and sets D-Bus event handlers */
static struct adv_monitor_app *app_create(DBusConnection *conn,
DBusMessage *msg, const char *sender,
@@ -165,8 +634,17 @@ static struct adv_monitor_app *app_create(DBusConnection *conn,
return NULL;
}
+ app->monitors = queue_new();
+
g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
- g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
+
+ /* Note that any property changes on a monitor object would not affect
+ * the content of the corresponding monitor.
+ */
+ g_dbus_client_set_proxy_handlers(app->client, monitor_proxy_added_cb,
+ monitor_proxy_removed_cb, NULL,
+ app);
+
g_dbus_client_set_ready_watch(app->client, app_ready_cb, app);
app->reg = dbus_message_ref(msg);
@@ -273,18 +751,6 @@ static const GDBusMethodTable adv_monitor_methods[] = {
{ }
};
-enum monitor_type {
- MONITOR_TYPE_OR_PATTERNS,
-};
-
-const struct adv_monitor_type {
- enum monitor_type type;
- const char *name;
-} supported_types[] = {
- { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
- { },
-};
-
/* Gets SupportedMonitorTypes property */
static gboolean get_supported_monitor_types(const GDBusPropertyTable *property,
DBusMessageIter *iter,
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 6/7] adv_monitor: Handle D-Bus proxy event of an ADV monitor
2020-08-18 22:26 ` [BlueZ PATCH v1 6/7] adv_monitor: Handle D-Bus proxy event of an ADV monitor Miao-chen Chou
@ 2020-09-08 17:35 ` Luiz Augusto von Dentz
2020-09-10 4:52 ` Miao-chen Chou
0 siblings, 1 reply; 22+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-08 17:35 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung
Hi Miao,
On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> This adds two handlers, one for adding and one for removing, of D-Bus proxy
> events. The handler of property changes is set to NULL as intended,
> since for simplicity no further changes on monitor's properties would
> affect the ongoing monitoring.
>
> The following test steps were performed with bluetoothctl.
> - After registering the root path, expose two monitors and verify that
> the proxy added event are received.
> - Have two monitors added, unexpose the monitors, and verify that the
> proxy removed events are received for those two monitors.
> - Have two monitors added, unregister the monitors and verify that the
> proxy removed events are received for those two monitors.
>
> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> ---
>
> src/adv_monitor.c | 492 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 479 insertions(+), 13 deletions(-)
>
> diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> index b5ea5ee99..23fbc2b45 100644
> --- a/src/adv_monitor.c
> +++ b/src/adv_monitor.c
> @@ -37,14 +37,23 @@
> #include "dbus-common.h"
> #include "log.h"
> #include "src/error.h"
> +#include "src/shared/ad.h"
> #include "src/shared/mgmt.h"
> #include "src/shared/queue.h"
> #include "src/shared/util.h"
>
> #include "adv_monitor.h"
>
> +#define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
>
> +#define ADV_MONITOR_UNSET_RSSI 127 /* dBm */
> +#define ADV_MONITOR_MAX_RSSI 20 /* dBm */
> +#define ADV_MONITOR_MIN_RSSI -127 /* dBm */
> +#define ADV_MONITOR_UNSET_TIMER 0 /* second */
> +#define ADV_MONITOR_MIN_TIMER 1 /* second */
> +#define ADV_MONITOR_MAX_TIMER 300 /* second */
> +
> struct btd_adv_monitor_manager {
> struct btd_adapter *adapter;
> struct mgmt *mgmt;
> @@ -66,6 +75,43 @@ struct adv_monitor_app {
>
> DBusMessage *reg;
> GDBusClient *client;
> +
> + struct queue *monitors;
> +};
> +
> +enum monitor_type {
> + MONITOR_TYPE_NONE,
> + MONITOR_TYPE_OR_PATTERNS,
> +};
> +
> +enum monitor_state {
> + MONITOR_STATE_NEW, /* New but not yet init'ed with actual values */
> + MONITOR_STATE_FAILED, /* Failed to be init'ed */
> + MONITOR_STATE_INITED, /* Init'ed but not yet sent to kernel */
> + MONITOR_STATE_HONORED, /* Accepted by kernel */
> +};
> +
> +struct pattern {
> + uint8_t ad_type;
> + uint8_t offset;
> + uint8_t length;
> + uint8_t value[BT_AD_MAX_DATA_LEN];
> +};
> +
> +struct adv_monitor {
> + struct adv_monitor_app *app;
> + GDBusProxy *proxy;
> + char *path;
> +
> + enum monitor_state state; /* MONITOR_STATE_* */
> +
> + int8_t high_rssi; /* high RSSI threshold */
> + uint16_t high_rssi_timeout; /* high RSSI threshold timeout */
> + int8_t low_rssi; /* low RSSI threshold */
> + uint16_t low_rssi_timeout; /* low RSSI threshold timeout */
> +
> + enum monitor_type type; /* MONITOR_TYPE_* */
> + struct queue *patterns;
> };
>
> struct app_match_data {
> @@ -73,6 +119,14 @@ struct app_match_data {
> const char *path;
> };
>
> +const struct adv_monitor_type {
> + enum monitor_type type;
> + const char *name;
> +} supported_types[] = {
> + { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
> + { },
> +};
> +
> /* Replies to an app's D-Bus message and unref it */
> static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> {
> @@ -84,6 +138,52 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> app->reg = NULL;
> }
>
> +/* Frees a pattern */
> +static void pattern_free(void *data)
> +{
> + struct pattern *pattern = data;
> +
> + if (!pattern)
> + return;
> +
> + g_free(pattern);
> +}
> +
> +/* Frees a monitor object */
> +static void monitor_free(void *data)
> +{
> + struct adv_monitor *monitor = data;
> +
> + if (!monitor)
> + return;
> +
> + monitor->app = NULL;
> + g_dbus_proxy_unref(monitor->proxy);
> + monitor->proxy = NULL;
> + g_free(monitor->path);
> + monitor->path = NULL;
> +
> + queue_destroy(monitor->patterns, pattern_free);
> + monitor->patterns = NULL;
> +
> + g_free(monitor);
> +}
> +
> +/* Calls Release() method of the remote Adv Monitor */
> +static void monitor_release(void *data, void *user_data)
> +{
> + struct adv_monitor *monitor = data;
> +
> + if (!monitor)
> + return;
> +
> + DBG("Calling Release() on Adv Monitor of owner %s at path %s",
> + monitor->app->owner, monitor->path);
> +
> + g_dbus_proxy_method_call(monitor->proxy, "Release", NULL, NULL, NULL,
> + NULL);
> +}
> +
> /* Destroys an app object along with related D-Bus handlers */
> static void app_destroy(void *data)
> {
> @@ -94,6 +194,9 @@ static void app_destroy(void *data)
>
> DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path);
>
> + queue_foreach(app->monitors, monitor_release, NULL);
> + queue_destroy(app->monitors, monitor_free);
> +
> if (app->reg) {
> app_reply_msg(app, btd_error_failed(app->reg,
> "Adv Monitor app destroyed"));
> @@ -139,6 +242,372 @@ static void app_ready_cb(GDBusClient *client, void *user_data)
> app_reply_msg(app, dbus_message_new_method_return(app->reg));
> }
>
> +/* Allocates an Adv Monitor */
> +static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
> + GDBusProxy *proxy)
> +{
> + struct adv_monitor *monitor;
> +
> + if (!app || !proxy)
> + return NULL;
> +
> + monitor = g_new0(struct adv_monitor, 1);
new0
> + if (!monitor)
> + return NULL;
> +
> + monitor->app = app;
> + monitor->proxy = g_dbus_proxy_ref(proxy);
> + monitor->path = g_strdup(g_dbus_proxy_get_path(proxy));
> +
> + monitor->state = MONITOR_STATE_NEW;
> +
> + monitor->high_rssi = ADV_MONITOR_UNSET_RSSI;
> + monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> + monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> + monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> +
> + monitor->type = MONITOR_TYPE_NONE;
> + monitor->patterns = NULL;
> +
> + return monitor;
> +}
> +
> +/* Matches a monitor based on its D-Bus path */
> +static bool monitor_match(const void *a, const void *b)
> +{
> + const GDBusProxy *proxy = b;
> + const struct adv_monitor *monitor = a;
> +
> + if (!proxy || !monitor)
> + return false;
> +
> + if (strcmp(g_dbus_proxy_get_path(proxy), monitor->path) != 0)
> + return false;
> +
> + return true;
> +}
> +
> +/* Retrieves Type from the remote Adv Monitor object, verifies the value and
> + * update the local Adv Monitor
> + */
> +static bool parse_monitor_type(struct adv_monitor *monitor, const char *path)
> +{
> + DBusMessageIter iter;
> + const struct adv_monitor_type *t;
> + const char *type_str;
> + uint16_t adapter_id = monitor->app->manager->adapter_id;
> +
> + if (!g_dbus_proxy_get_property(monitor->proxy, "Type", &iter)) {
> + btd_error(adapter_id, "Failed to retrieve property Type from "
> + "the Adv Monitor at path %s", path);
> + return false;
> + }
> +
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> + goto failed;
> +
> + dbus_message_iter_get_basic(&iter, &type_str);
> +
> + for (t = supported_types; t->name; t++) {
> + if (strcmp(t->name, type_str) == 0) {
> + monitor->type = t->type;
> + return true;
> + }
> + }
> +
> +failed:
> + btd_error(adapter_id, "Invalid argument of property Type of the Adv "
> + "Monitor at path %s", path);
> +
> + return false;
> +}
> +
> +/* Retrieves RSSIThresholdsAndTimers from the remote Adv Monitor object,
> + * verifies the values and update the local Adv Monitor
> + */
> +static bool parse_rssi_and_timeout(struct adv_monitor *monitor,
> + const char *path)
> +{
> + DBusMessageIter prop_struct, iter;
> + int16_t h_rssi, l_rssi;
> + uint16_t h_rssi_timer, l_rssi_timer;
> + uint16_t adapter_id = monitor->app->manager->adapter_id;
> +
> + /* Property RSSIThresholdsAndTimers is optional */
> + if (!g_dbus_proxy_get_property(monitor->proxy,
> + "RSSIThresholdsAndTimers",
> + &prop_struct)) {
> + DBG("Adv Monitor at path %s provides no RSSI thresholds and "
> + "timeouts", path);
> + return true;
> + }
> +
> + if (dbus_message_iter_get_arg_type(&prop_struct) != DBUS_TYPE_STRUCT)
> + goto failed;
> +
> + dbus_message_iter_recurse(&prop_struct, &iter);
> +
> + /* Extract HighRSSIThreshold */
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT16)
> + goto failed;
> + dbus_message_iter_get_basic(&iter, &h_rssi);
> + if (!dbus_message_iter_next(&iter))
> + goto failed;
> +
> + /* Extract HighRSSIThresholdTimer */
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_UINT16)
> + goto failed;
> + dbus_message_iter_get_basic(&iter, &h_rssi_timer);
> + if (!dbus_message_iter_next(&iter))
> + goto failed;
> +
> + /* Extract LowRSSIThreshold */
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT16)
> + goto failed;
> + dbus_message_iter_get_basic(&iter, &l_rssi);
> + if (!dbus_message_iter_next(&iter))
> + goto failed;
> +
> + /* Extract LowRSSIThresholdTimer */
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_UINT16)
> + goto failed;
> + dbus_message_iter_get_basic(&iter, &l_rssi_timer);
> +
> + /* Verify the values of RSSIs and their timers. For simplicity, we
> + * enforce the all-or-none rule to these fields. In other words, either
> + * all are set to the unset values or all are set within valid ranges.
> + */
> + if (h_rssi == ADV_MONITOR_UNSET_RSSI &&
> + l_rssi == ADV_MONITOR_UNSET_RSSI &&
> + h_rssi_timer == ADV_MONITOR_UNSET_TIMER &&
> + l_rssi_timer == ADV_MONITOR_UNSET_TIMER) {
> + goto done;
> + }
> +
> + if (h_rssi < ADV_MONITOR_MIN_RSSI || h_rssi > ADV_MONITOR_MAX_RSSI ||
> + l_rssi < ADV_MONITOR_MIN_RSSI ||
> + l_rssi > ADV_MONITOR_MAX_RSSI || h_rssi <= l_rssi) {
> + goto failed;
> + }
> +
> + if (h_rssi_timer < ADV_MONITOR_MIN_TIMER ||
> + h_rssi_timer > ADV_MONITOR_MAX_TIMER ||
> + l_rssi_timer < ADV_MONITOR_MIN_TIMER ||
> + l_rssi_timer > ADV_MONITOR_MAX_TIMER) {
> + goto failed;
> + }
> +
> + monitor->high_rssi = h_rssi;
> + monitor->low_rssi = l_rssi;
> + monitor->high_rssi_timeout = h_rssi_timer;
> + monitor->low_rssi_timeout = l_rssi_timer;
> +
> +done:
> + DBG("Adv Monitor at %s initiated with high RSSI threshold %d, high "
> + "RSSI threshold timeout %d, low RSSI threshold %d, low RSSI "
> + "threshold timeout %d", path, monitor->high_rssi,
> + monitor->high_rssi_timeout, monitor->low_rssi,
> + monitor->low_rssi_timeout);
> +
> + return true;
> +
> +failed:
> + monitor->high_rssi = ADV_MONITOR_UNSET_RSSI;
> + monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> + monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> + monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> +
> + btd_error(adapter_id, "Invalid argument of property "
> + "RSSIThresholdsAndTimers of the Adv Monitor at path %s",
> + path);
> +
> + return false;
> +}
> +
> +/* Retrieves Patterns from the remote Adv Monitor object, verifies the values
> + * and update the local Adv Monitor
> + */
> +static bool parse_patterns(struct adv_monitor *monitor, const char *path)
> +{
> + DBusMessageIter array, array_iter;
> + uint16_t num_patterns = 0;
> + uint16_t adapter_id = monitor->app->manager->adapter_id;
> +
> + if (!g_dbus_proxy_get_property(monitor->proxy, "Patterns", &array)) {
> + btd_error(adapter_id, "Failed to retrieve property Patterns "
> + "from the Adv Monitor at path %s", path);
> + return false;
> + }
> +
> + monitor->patterns = queue_new();
> +
> + if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_ARRAY ||
> + dbus_message_iter_get_element_type(&array) !=
> + DBUS_TYPE_STRUCT) {
> + goto failed;
> + }
> +
> + dbus_message_iter_recurse(&array, &array_iter);
> +
> + while (dbus_message_iter_get_arg_type(&array_iter) ==
> + DBUS_TYPE_STRUCT) {
> + int value_len;
> + uint8_t *value;
> + uint8_t offset, ad_type;
> + struct pattern *pattern;
> + DBusMessageIter struct_iter, value_iter;
> +
> + dbus_message_iter_recurse(&array_iter, &struct_iter);
> +
> + // Extract start position
> + if (dbus_message_iter_get_arg_type(&struct_iter) !=
> + DBUS_TYPE_BYTE) {
> + goto failed;
> + }
> + dbus_message_iter_get_basic(&struct_iter, &offset);
> + if (!dbus_message_iter_next(&struct_iter))
> + goto failed;
> +
> + // Extract AD data type
> + if (dbus_message_iter_get_arg_type(&struct_iter) !=
> + DBUS_TYPE_BYTE) {
> + goto failed;
> + }
> + dbus_message_iter_get_basic(&struct_iter, &ad_type);
> + if (!dbus_message_iter_next(&struct_iter))
> + goto failed;
> +
> + // Extract value of a pattern
> + if (dbus_message_iter_get_arg_type(&struct_iter) !=
> + DBUS_TYPE_ARRAY) {
> + goto failed;
> + }
> + dbus_message_iter_recurse(&struct_iter, &value_iter);
> + dbus_message_iter_get_fixed_array(&value_iter, &value,
> + &value_len);
> +
> + // Verify the values
> + if (offset > BT_AD_MAX_DATA_LEN - 1)
> + goto failed;
> +
> + if (ad_type > BT_AD_3D_INFO_DATA &&
> + ad_type != BT_AD_MANUFACTURER_DATA
> + || ad_type < BT_AD_FLAGS) {
> + goto failed;
> + }
> +
> + if (!value || value_len <= 0 || value_len > BT_AD_MAX_DATA_LEN)
> + goto failed;
> +
> + pattern = g_new0(struct pattern, 1);
new0
> + if (!pattern)
> + goto failed;
> +
> + pattern->ad_type = ad_type;
> + pattern->offset = offset;
> + pattern->length = value_len;
> + g_memmove(pattern->value, value, pattern->length);
This looks wrong, we shouldn't be changing the memory value points to,
this might be equivalent to memcpy so Id just use that instead.
> +
> + queue_push_tail(monitor->patterns, pattern);
> +
> + dbus_message_iter_next(&array_iter);
> + }
> +
> + /* There must be at least one pattern. */
> + if (queue_isempty(monitor->patterns))
> + goto failed;
> +
> + return true;
> +
> +failed:
> + queue_destroy(monitor->patterns, pattern_free);
> + monitor->patterns = NULL;
> +
> + btd_error(adapter_id, "Invalid argument of property Patterns of the "
> + "Adv Monitor at path %s", path);
> +
> + return false;
> +}
> +
> +/* Processes the content of the remote Adv Monitor */
> +static bool monitor_process(struct adv_monitor *monitor,
> + struct adv_monitor_app *app)
> +{
> + const char *path = g_dbus_proxy_get_path(monitor->proxy);
> +
> + monitor->state = MONITOR_STATE_FAILED;
> +
> + if (!parse_monitor_type(monitor, path))
> + goto done;
> +
> + if (!parse_rssi_and_timeout(monitor, path))
> + goto done;
> +
> + if (monitor->type == MONITOR_TYPE_OR_PATTERNS &&
> + parse_patterns(monitor, path)) {
> + monitor->state = MONITOR_STATE_INITED;
> + }
> +
> +done:
> + return monitor->state != MONITOR_STATE_FAILED;
> +}
> +
> +/* Handles an Adv Monitor D-Bus proxy added event */
> +static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
> +{
> + struct adv_monitor *monitor;
> + struct adv_monitor_app *app = user_data;
> + uint16_t adapter_id = app->manager->adapter_id;
> + const char *path = g_dbus_proxy_get_path(proxy);
> + const char *iface = g_dbus_proxy_get_interface(proxy);
> +
> + if (strcmp(iface, ADV_MONITOR_INTERFACE) != 0 ||
> + !g_str_has_prefix(path, app->path)) {
> + return;
> + }
> +
> + if (queue_find(app->monitors, monitor_match, proxy)) {
> + btd_error(adapter_id, "Adv Monitor proxy already exists with "
> + "path %s", path);
> + return;
> + }
> +
> + monitor = monitor_new(app, proxy);
> + if (!monitor) {
> + btd_error(adapter_id, "Failed to allocate an Adv Monitor for "
> + "the object at %s", path);
> + return;
> + }
> +
> + if (!monitor_process(monitor, app)) {
> + monitor_release(monitor, NULL);
> + monitor_free(monitor);
> + DBG("Adv Monitor at path %s released due to invalid content",
> + path);
> + return;
> + }
> +
> + queue_push_tail(app->monitors, monitor);
> +
> + DBG("Adv Monitor allocated for the object at path %s", path);
> +}
> +
> +/* Handles the removal of an Adv Monitor D-Bus proxy */
> +static void monitor_proxy_removed_cb(GDBusProxy *proxy, void *user_data)
> +{
> + struct adv_monitor *monitor;
> + struct adv_monitor_app *app = user_data;
> +
> + monitor = queue_remove_if(app->monitors, monitor_match, proxy);
> + if (monitor) {
> + DBG("Adv Monitor removed for the object at path %s",
> + monitor->path);
> +
> + /* The object was gone, so we don't need to call Release() */
> + monitor_free(monitor);
> + }
> +}
> +
> /* Creates an app object, initiates it and sets D-Bus event handlers */
> static struct adv_monitor_app *app_create(DBusConnection *conn,
> DBusMessage *msg, const char *sender,
> @@ -165,8 +634,17 @@ static struct adv_monitor_app *app_create(DBusConnection *conn,
> return NULL;
> }
>
> + app->monitors = queue_new();
> +
> g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
> - g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
> +
> + /* Note that any property changes on a monitor object would not affect
> + * the content of the corresponding monitor.
> + */
> + g_dbus_client_set_proxy_handlers(app->client, monitor_proxy_added_cb,
> + monitor_proxy_removed_cb, NULL,
> + app);
> +
> g_dbus_client_set_ready_watch(app->client, app_ready_cb, app);
>
> app->reg = dbus_message_ref(msg);
> @@ -273,18 +751,6 @@ static const GDBusMethodTable adv_monitor_methods[] = {
> { }
> };
>
> -enum monitor_type {
> - MONITOR_TYPE_OR_PATTERNS,
> -};
> -
> -const struct adv_monitor_type {
> - enum monitor_type type;
> - const char *name;
> -} supported_types[] = {
> - { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
> - { },
> -};
> -
> /* Gets SupportedMonitorTypes property */
> static gboolean get_supported_monitor_types(const GDBusPropertyTable *property,
> DBusMessageIter *iter,
> --
> 2.26.2
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 6/7] adv_monitor: Handle D-Bus proxy event of an ADV monitor
2020-09-08 17:35 ` Luiz Augusto von Dentz
@ 2020-09-10 4:52 ` Miao-chen Chou
0 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-09-10 4:52 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung
Hi Luiz,
On Tue, Sep 8, 2020 at 10:36 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:31 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > This adds two handlers, one for adding and one for removing, of D-Bus proxy
> > events. The handler of property changes is set to NULL as intended,
> > since for simplicity no further changes on monitor's properties would
> > affect the ongoing monitoring.
> >
> > The following test steps were performed with bluetoothctl.
> > - After registering the root path, expose two monitors and verify that
> > the proxy added event are received.
> > - Have two monitors added, unexpose the monitors, and verify that the
> > proxy removed events are received for those two monitors.
> > - Have two monitors added, unregister the monitors and verify that the
> > proxy removed events are received for those two monitors.
> >
> > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > ---
> >
> > src/adv_monitor.c | 492 ++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 479 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > index b5ea5ee99..23fbc2b45 100644
> > --- a/src/adv_monitor.c
> > +++ b/src/adv_monitor.c
> > @@ -37,14 +37,23 @@
> > #include "dbus-common.h"
> > #include "log.h"
> > #include "src/error.h"
> > +#include "src/shared/ad.h"
> > #include "src/shared/mgmt.h"
> > #include "src/shared/queue.h"
> > #include "src/shared/util.h"
> >
> > #include "adv_monitor.h"
> >
> > +#define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> > #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
> >
> > +#define ADV_MONITOR_UNSET_RSSI 127 /* dBm */
> > +#define ADV_MONITOR_MAX_RSSI 20 /* dBm */
> > +#define ADV_MONITOR_MIN_RSSI -127 /* dBm */
> > +#define ADV_MONITOR_UNSET_TIMER 0 /* second */
> > +#define ADV_MONITOR_MIN_TIMER 1 /* second */
> > +#define ADV_MONITOR_MAX_TIMER 300 /* second */
> > +
> > struct btd_adv_monitor_manager {
> > struct btd_adapter *adapter;
> > struct mgmt *mgmt;
> > @@ -66,6 +75,43 @@ struct adv_monitor_app {
> >
> > DBusMessage *reg;
> > GDBusClient *client;
> > +
> > + struct queue *monitors;
> > +};
> > +
> > +enum monitor_type {
> > + MONITOR_TYPE_NONE,
> > + MONITOR_TYPE_OR_PATTERNS,
> > +};
> > +
> > +enum monitor_state {
> > + MONITOR_STATE_NEW, /* New but not yet init'ed with actual values */
> > + MONITOR_STATE_FAILED, /* Failed to be init'ed */
> > + MONITOR_STATE_INITED, /* Init'ed but not yet sent to kernel */
> > + MONITOR_STATE_HONORED, /* Accepted by kernel */
> > +};
> > +
> > +struct pattern {
> > + uint8_t ad_type;
> > + uint8_t offset;
> > + uint8_t length;
> > + uint8_t value[BT_AD_MAX_DATA_LEN];
> > +};
> > +
> > +struct adv_monitor {
> > + struct adv_monitor_app *app;
> > + GDBusProxy *proxy;
> > + char *path;
> > +
> > + enum monitor_state state; /* MONITOR_STATE_* */
> > +
> > + int8_t high_rssi; /* high RSSI threshold */
> > + uint16_t high_rssi_timeout; /* high RSSI threshold timeout */
> > + int8_t low_rssi; /* low RSSI threshold */
> > + uint16_t low_rssi_timeout; /* low RSSI threshold timeout */
> > +
> > + enum monitor_type type; /* MONITOR_TYPE_* */
> > + struct queue *patterns;
> > };
> >
> > struct app_match_data {
> > @@ -73,6 +119,14 @@ struct app_match_data {
> > const char *path;
> > };
> >
> > +const struct adv_monitor_type {
> > + enum monitor_type type;
> > + const char *name;
> > +} supported_types[] = {
> > + { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
> > + { },
> > +};
> > +
> > /* Replies to an app's D-Bus message and unref it */
> > static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> > {
> > @@ -84,6 +138,52 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> > app->reg = NULL;
> > }
> >
> > +/* Frees a pattern */
> > +static void pattern_free(void *data)
> > +{
> > + struct pattern *pattern = data;
> > +
> > + if (!pattern)
> > + return;
> > +
> > + g_free(pattern);
> > +}
> > +
> > +/* Frees a monitor object */
> > +static void monitor_free(void *data)
> > +{
> > + struct adv_monitor *monitor = data;
> > +
> > + if (!monitor)
> > + return;
> > +
> > + monitor->app = NULL;
> > + g_dbus_proxy_unref(monitor->proxy);
> > + monitor->proxy = NULL;
> > + g_free(monitor->path);
> > + monitor->path = NULL;
> > +
> > + queue_destroy(monitor->patterns, pattern_free);
> > + monitor->patterns = NULL;
> > +
> > + g_free(monitor);
> > +}
> > +
> > +/* Calls Release() method of the remote Adv Monitor */
> > +static void monitor_release(void *data, void *user_data)
> > +{
> > + struct adv_monitor *monitor = data;
> > +
> > + if (!monitor)
> > + return;
> > +
> > + DBG("Calling Release() on Adv Monitor of owner %s at path %s",
> > + monitor->app->owner, monitor->path);
> > +
> > + g_dbus_proxy_method_call(monitor->proxy, "Release", NULL, NULL, NULL,
> > + NULL);
> > +}
> > +
> > /* Destroys an app object along with related D-Bus handlers */
> > static void app_destroy(void *data)
> > {
> > @@ -94,6 +194,9 @@ static void app_destroy(void *data)
> >
> > DBG("Destroy Adv Monitor app %s at path %s", app->owner, app->path);
> >
> > + queue_foreach(app->monitors, monitor_release, NULL);
> > + queue_destroy(app->monitors, monitor_free);
> > +
> > if (app->reg) {
> > app_reply_msg(app, btd_error_failed(app->reg,
> > "Adv Monitor app destroyed"));
> > @@ -139,6 +242,372 @@ static void app_ready_cb(GDBusClient *client, void *user_data)
> > app_reply_msg(app, dbus_message_new_method_return(app->reg));
> > }
> >
> > +/* Allocates an Adv Monitor */
> > +static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
> > + GDBusProxy *proxy)
> > +{
> > + struct adv_monitor *monitor;
> > +
> > + if (!app || !proxy)
> > + return NULL;
> > +
> > + monitor = g_new0(struct adv_monitor, 1);
>
> new0
Done.
>
> > + if (!monitor)
> > + return NULL;
> > +
> > + monitor->app = app;
> > + monitor->proxy = g_dbus_proxy_ref(proxy);
> > + monitor->path = g_strdup(g_dbus_proxy_get_path(proxy));
> > +
> > + monitor->state = MONITOR_STATE_NEW;
> > +
> > + monitor->high_rssi = ADV_MONITOR_UNSET_RSSI;
> > + monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > + monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> > + monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > +
> > + monitor->type = MONITOR_TYPE_NONE;
> > + monitor->patterns = NULL;
> > +
> > + return monitor;
> > +}
> > +
> > +/* Matches a monitor based on its D-Bus path */
> > +static bool monitor_match(const void *a, const void *b)
> > +{
> > + const GDBusProxy *proxy = b;
> > + const struct adv_monitor *monitor = a;
> > +
> > + if (!proxy || !monitor)
> > + return false;
> > +
> > + if (strcmp(g_dbus_proxy_get_path(proxy), monitor->path) != 0)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/* Retrieves Type from the remote Adv Monitor object, verifies the value and
> > + * update the local Adv Monitor
> > + */
> > +static bool parse_monitor_type(struct adv_monitor *monitor, const char *path)
> > +{
> > + DBusMessageIter iter;
> > + const struct adv_monitor_type *t;
> > + const char *type_str;
> > + uint16_t adapter_id = monitor->app->manager->adapter_id;
> > +
> > + if (!g_dbus_proxy_get_property(monitor->proxy, "Type", &iter)) {
> > + btd_error(adapter_id, "Failed to retrieve property Type from "
> > + "the Adv Monitor at path %s", path);
> > + return false;
> > + }
> > +
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> > + goto failed;
> > +
> > + dbus_message_iter_get_basic(&iter, &type_str);
> > +
> > + for (t = supported_types; t->name; t++) {
> > + if (strcmp(t->name, type_str) == 0) {
> > + monitor->type = t->type;
> > + return true;
> > + }
> > + }
> > +
> > +failed:
> > + btd_error(adapter_id, "Invalid argument of property Type of the Adv "
> > + "Monitor at path %s", path);
> > +
> > + return false;
> > +}
> > +
> > +/* Retrieves RSSIThresholdsAndTimers from the remote Adv Monitor object,
> > + * verifies the values and update the local Adv Monitor
> > + */
> > +static bool parse_rssi_and_timeout(struct adv_monitor *monitor,
> > + const char *path)
> > +{
> > + DBusMessageIter prop_struct, iter;
> > + int16_t h_rssi, l_rssi;
> > + uint16_t h_rssi_timer, l_rssi_timer;
> > + uint16_t adapter_id = monitor->app->manager->adapter_id;
> > +
> > + /* Property RSSIThresholdsAndTimers is optional */
> > + if (!g_dbus_proxy_get_property(monitor->proxy,
> > + "RSSIThresholdsAndTimers",
> > + &prop_struct)) {
> > + DBG("Adv Monitor at path %s provides no RSSI thresholds and "
> > + "timeouts", path);
> > + return true;
> > + }
> > +
> > + if (dbus_message_iter_get_arg_type(&prop_struct) != DBUS_TYPE_STRUCT)
> > + goto failed;
> > +
> > + dbus_message_iter_recurse(&prop_struct, &iter);
> > +
> > + /* Extract HighRSSIThreshold */
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT16)
> > + goto failed;
> > + dbus_message_iter_get_basic(&iter, &h_rssi);
> > + if (!dbus_message_iter_next(&iter))
> > + goto failed;
> > +
> > + /* Extract HighRSSIThresholdTimer */
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_UINT16)
> > + goto failed;
> > + dbus_message_iter_get_basic(&iter, &h_rssi_timer);
> > + if (!dbus_message_iter_next(&iter))
> > + goto failed;
> > +
> > + /* Extract LowRSSIThreshold */
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_INT16)
> > + goto failed;
> > + dbus_message_iter_get_basic(&iter, &l_rssi);
> > + if (!dbus_message_iter_next(&iter))
> > + goto failed;
> > +
> > + /* Extract LowRSSIThresholdTimer */
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_UINT16)
> > + goto failed;
> > + dbus_message_iter_get_basic(&iter, &l_rssi_timer);
> > +
> > + /* Verify the values of RSSIs and their timers. For simplicity, we
> > + * enforce the all-or-none rule to these fields. In other words, either
> > + * all are set to the unset values or all are set within valid ranges.
> > + */
> > + if (h_rssi == ADV_MONITOR_UNSET_RSSI &&
> > + l_rssi == ADV_MONITOR_UNSET_RSSI &&
> > + h_rssi_timer == ADV_MONITOR_UNSET_TIMER &&
> > + l_rssi_timer == ADV_MONITOR_UNSET_TIMER) {
> > + goto done;
> > + }
> > +
> > + if (h_rssi < ADV_MONITOR_MIN_RSSI || h_rssi > ADV_MONITOR_MAX_RSSI ||
> > + l_rssi < ADV_MONITOR_MIN_RSSI ||
> > + l_rssi > ADV_MONITOR_MAX_RSSI || h_rssi <= l_rssi) {
> > + goto failed;
> > + }
> > +
> > + if (h_rssi_timer < ADV_MONITOR_MIN_TIMER ||
> > + h_rssi_timer > ADV_MONITOR_MAX_TIMER ||
> > + l_rssi_timer < ADV_MONITOR_MIN_TIMER ||
> > + l_rssi_timer > ADV_MONITOR_MAX_TIMER) {
> > + goto failed;
> > + }
> > +
> > + monitor->high_rssi = h_rssi;
> > + monitor->low_rssi = l_rssi;
> > + monitor->high_rssi_timeout = h_rssi_timer;
> > + monitor->low_rssi_timeout = l_rssi_timer;
> > +
> > +done:
> > + DBG("Adv Monitor at %s initiated with high RSSI threshold %d, high "
> > + "RSSI threshold timeout %d, low RSSI threshold %d, low RSSI "
> > + "threshold timeout %d", path, monitor->high_rssi,
> > + monitor->high_rssi_timeout, monitor->low_rssi,
> > + monitor->low_rssi_timeout);
> > +
> > + return true;
> > +
> > +failed:
> > + monitor->high_rssi = ADV_MONITOR_UNSET_RSSI;
> > + monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> > + monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > + monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > +
> > + btd_error(adapter_id, "Invalid argument of property "
> > + "RSSIThresholdsAndTimers of the Adv Monitor at path %s",
> > + path);
> > +
> > + return false;
> > +}
> > +
> > +/* Retrieves Patterns from the remote Adv Monitor object, verifies the values
> > + * and update the local Adv Monitor
> > + */
> > +static bool parse_patterns(struct adv_monitor *monitor, const char *path)
> > +{
> > + DBusMessageIter array, array_iter;
> > + uint16_t num_patterns = 0;
> > + uint16_t adapter_id = monitor->app->manager->adapter_id;
> > +
> > + if (!g_dbus_proxy_get_property(monitor->proxy, "Patterns", &array)) {
> > + btd_error(adapter_id, "Failed to retrieve property Patterns "
> > + "from the Adv Monitor at path %s", path);
> > + return false;
> > + }
> > +
> > + monitor->patterns = queue_new();
> > +
> > + if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_ARRAY ||
> > + dbus_message_iter_get_element_type(&array) !=
> > + DBUS_TYPE_STRUCT) {
> > + goto failed;
> > + }
> > +
> > + dbus_message_iter_recurse(&array, &array_iter);
> > +
> > + while (dbus_message_iter_get_arg_type(&array_iter) ==
> > + DBUS_TYPE_STRUCT) {
> > + int value_len;
> > + uint8_t *value;
> > + uint8_t offset, ad_type;
> > + struct pattern *pattern;
> > + DBusMessageIter struct_iter, value_iter;
> > +
> > + dbus_message_iter_recurse(&array_iter, &struct_iter);
> > +
> > + // Extract start position
> > + if (dbus_message_iter_get_arg_type(&struct_iter) !=
> > + DBUS_TYPE_BYTE) {
> > + goto failed;
> > + }
> > + dbus_message_iter_get_basic(&struct_iter, &offset);
> > + if (!dbus_message_iter_next(&struct_iter))
> > + goto failed;
> > +
> > + // Extract AD data type
> > + if (dbus_message_iter_get_arg_type(&struct_iter) !=
> > + DBUS_TYPE_BYTE) {
> > + goto failed;
> > + }
> > + dbus_message_iter_get_basic(&struct_iter, &ad_type);
> > + if (!dbus_message_iter_next(&struct_iter))
> > + goto failed;
> > +
> > + // Extract value of a pattern
> > + if (dbus_message_iter_get_arg_type(&struct_iter) !=
> > + DBUS_TYPE_ARRAY) {
> > + goto failed;
> > + }
> > + dbus_message_iter_recurse(&struct_iter, &value_iter);
> > + dbus_message_iter_get_fixed_array(&value_iter, &value,
> > + &value_len);
> > +
> > + // Verify the values
> > + if (offset > BT_AD_MAX_DATA_LEN - 1)
> > + goto failed;
> > +
> > + if (ad_type > BT_AD_3D_INFO_DATA &&
> > + ad_type != BT_AD_MANUFACTURER_DATA
> > + || ad_type < BT_AD_FLAGS) {
> > + goto failed;
> > + }
> > +
> > + if (!value || value_len <= 0 || value_len > BT_AD_MAX_DATA_LEN)
> > + goto failed;
> > +
> > + pattern = g_new0(struct pattern, 1);
>
> new0
Done.
>
> > + if (!pattern)
> > + goto failed;
> > +
> > + pattern->ad_type = ad_type;
> > + pattern->offset = offset;
> > + pattern->length = value_len;
> > + g_memmove(pattern->value, value, pattern->length);
>
> This looks wrong, we shouldn't be changing the memory value points to,
> this might be equivalent to memcpy so Id just use that instead.
Done.
>
> > +
> > + queue_push_tail(monitor->patterns, pattern);
> > +
> > + dbus_message_iter_next(&array_iter);
> > + }
> > +
> > + /* There must be at least one pattern. */
> > + if (queue_isempty(monitor->patterns))
> > + goto failed;
> > +
> > + return true;
> > +
> > +failed:
> > + queue_destroy(monitor->patterns, pattern_free);
> > + monitor->patterns = NULL;
> > +
> > + btd_error(adapter_id, "Invalid argument of property Patterns of the "
> > + "Adv Monitor at path %s", path);
> > +
> > + return false;
> > +}
> > +
> > +/* Processes the content of the remote Adv Monitor */
> > +static bool monitor_process(struct adv_monitor *monitor,
> > + struct adv_monitor_app *app)
> > +{
> > + const char *path = g_dbus_proxy_get_path(monitor->proxy);
> > +
> > + monitor->state = MONITOR_STATE_FAILED;
> > +
> > + if (!parse_monitor_type(monitor, path))
> > + goto done;
> > +
> > + if (!parse_rssi_and_timeout(monitor, path))
> > + goto done;
> > +
> > + if (monitor->type == MONITOR_TYPE_OR_PATTERNS &&
> > + parse_patterns(monitor, path)) {
> > + monitor->state = MONITOR_STATE_INITED;
> > + }
> > +
> > +done:
> > + return monitor->state != MONITOR_STATE_FAILED;
> > +}
> > +
> > +/* Handles an Adv Monitor D-Bus proxy added event */
> > +static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
> > +{
> > + struct adv_monitor *monitor;
> > + struct adv_monitor_app *app = user_data;
> > + uint16_t adapter_id = app->manager->adapter_id;
> > + const char *path = g_dbus_proxy_get_path(proxy);
> > + const char *iface = g_dbus_proxy_get_interface(proxy);
> > +
> > + if (strcmp(iface, ADV_MONITOR_INTERFACE) != 0 ||
> > + !g_str_has_prefix(path, app->path)) {
> > + return;
> > + }
> > +
> > + if (queue_find(app->monitors, monitor_match, proxy)) {
> > + btd_error(adapter_id, "Adv Monitor proxy already exists with "
> > + "path %s", path);
> > + return;
> > + }
> > +
> > + monitor = monitor_new(app, proxy);
> > + if (!monitor) {
> > + btd_error(adapter_id, "Failed to allocate an Adv Monitor for "
> > + "the object at %s", path);
> > + return;
> > + }
> > +
> > + if (!monitor_process(monitor, app)) {
> > + monitor_release(monitor, NULL);
> > + monitor_free(monitor);
> > + DBG("Adv Monitor at path %s released due to invalid content",
> > + path);
> > + return;
> > + }
> > +
> > + queue_push_tail(app->monitors, monitor);
> > +
> > + DBG("Adv Monitor allocated for the object at path %s", path);
> > +}
> > +
> > +/* Handles the removal of an Adv Monitor D-Bus proxy */
> > +static void monitor_proxy_removed_cb(GDBusProxy *proxy, void *user_data)
> > +{
> > + struct adv_monitor *monitor;
> > + struct adv_monitor_app *app = user_data;
> > +
> > + monitor = queue_remove_if(app->monitors, monitor_match, proxy);
> > + if (monitor) {
> > + DBG("Adv Monitor removed for the object at path %s",
> > + monitor->path);
> > +
> > + /* The object was gone, so we don't need to call Release() */
> > + monitor_free(monitor);
> > + }
> > +}
> > +
> > /* Creates an app object, initiates it and sets D-Bus event handlers */
> > static struct adv_monitor_app *app_create(DBusConnection *conn,
> > DBusMessage *msg, const char *sender,
> > @@ -165,8 +634,17 @@ static struct adv_monitor_app *app_create(DBusConnection *conn,
> > return NULL;
> > }
> >
> > + app->monitors = queue_new();
> > +
> > g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
> > - g_dbus_client_set_proxy_handlers(app->client, NULL, NULL, NULL, NULL);
> > +
> > + /* Note that any property changes on a monitor object would not affect
> > + * the content of the corresponding monitor.
> > + */
> > + g_dbus_client_set_proxy_handlers(app->client, monitor_proxy_added_cb,
> > + monitor_proxy_removed_cb, NULL,
> > + app);
> > +
> > g_dbus_client_set_ready_watch(app->client, app_ready_cb, app);
> >
> > app->reg = dbus_message_ref(msg);
> > @@ -273,18 +751,6 @@ static const GDBusMethodTable adv_monitor_methods[] = {
> > { }
> > };
> >
> > -enum monitor_type {
> > - MONITOR_TYPE_OR_PATTERNS,
> > -};
> > -
> > -const struct adv_monitor_type {
> > - enum monitor_type type;
> > - const char *name;
> > -} supported_types[] = {
> > - { MONITOR_TYPE_OR_PATTERNS, "or_patterns" },
> > - { },
> > -};
> > -
> > /* Gets SupportedMonitorTypes property */
> > static gboolean get_supported_monitor_types(const GDBusPropertyTable *property,
> > DBusMessageIter *iter,
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Miao
^ permalink raw reply [flat|nested] 22+ messages in thread
* [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
` (4 preceding siblings ...)
2020-08-18 22:26 ` [BlueZ PATCH v1 6/7] adv_monitor: Handle D-Bus proxy event of an ADV monitor Miao-chen Chou
@ 2020-08-18 22:26 ` Miao-chen Chou
2020-08-18 22:50 ` [BlueZ,v1,7/7] " bluez.test.bot
2020-09-08 17:39 ` [BlueZ PATCH v1 7/7] " Luiz Augusto von Dentz
2020-08-18 22:50 ` [BlueZ,v1,1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface bluez.test.bot
` (2 subsequent siblings)
8 siblings, 2 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-08-18 22:26 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Alain Michaud, Manish Mandlik, Luiz Augusto von Dentz,
Howard Chung, Miao-chen Chou
This modifies the following description to Advertisement Monitor API.
- Add org.bluez.Error.Failed to RegisterMonitor() method.
- Add more description about the usage of RegisterMonitor() and
UnregisterMonitor() methods.
- Add description about the ranges for the fields in property
RSSIThresholdsAndTimers.
Reviewed-by: Yun-Hao Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
doc/advertisement-monitor-api.txt | 34 +++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
index 74adbfae9..e09b6fd25 100644
--- a/doc/advertisement-monitor-api.txt
+++ b/doc/advertisement-monitor-api.txt
@@ -49,7 +49,7 @@ Properties string Type [read-only]
org.bluez.AdvertisementMonitorManager1 for the available
options.
- (Int16, Uint16, Int16, Uint16) RSSIThreshholdsAndTimers [read-only, optional]
+ (Int16, Uint16, Int16, Uint16) RSSIThresholdsAndTimers [read-only, optional]
This contains HighRSSIThreshold, HighRSSIThresholdTimer,
LowRSSIThreshold, LowRSSIThresholdTimer in order. The
@@ -66,7 +66,11 @@ Properties string Type [read-only]
RSSIs of the received advertisement(s) during
LowRSSIThresholdTimer do not reach LowRSSIThreshold.
- array{(uint8, uint8, string)} Patterns [read-only, optional]
+ The valid range of a RSSI is -127 to +20 dBm while 127
+ dBm indicates unset. The valid range of a timer is 1 to
+ 300 seconds while 0 indicates unset.
+
+ array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
If Type is set to 0x01, this must exist and has at least
one entry in the array.
@@ -80,8 +84,9 @@ Properties string Type [read-only]
See https://www.bluetooth.com/specifications/
assigned-numbers/generic-access-profile/ for
the possible allowed value.
- string content_of_pattern
- This is the value of the pattern.
+ array{byte} content_of_pattern
+ This is the value of the pattern. The maximum
+ length of the bytes is 31.
Advertisement Monitor Manager hierarchy
=======================================
@@ -91,20 +96,31 @@ Object path /org/bluez/{hci0,hci1,...}
Methods void RegisterMonitor(object application)
- This registers a hierarchy of advertisement monitors.
+ This registers the root path of a hierarchy of
+ advertisement monitors.
The application object path together with the D-Bus
system bus connection ID define the identification of
the application registering advertisement monitors.
+ Once a root path is registered by a client via this
+ method, the client can freely expose/unexpose
+ advertisement monitors without re-registering the root
+ path again. After use, the client should call
+ UnregisterMonitor() method to invalidate the
+ advertisement monitors.
Possible errors: org.bluez.Error.InvalidArguments
org.bluez.Error.AlreadyExists
+ org.bluez.Error.Failed
void UnregisterMonitor(object application)
- This unregisters advertisement monitors that have been
- previously registered. The object path parameter must
- match the same value that has been used on
- registration.
+ This unregisters a hierarchy of advertisement monitors
+ that has been previously registered. The object path
+ parameter must match the same value that has been used
+ on registration. Upon unregistration, the advertisement
+ monitor(s) should expect to receive Release() method as
+ the signal that the advertisement monitor(s) has been
+ deactivated.
Possible errors: org.bluez.Error.InvalidArguments
org.bluez.Error.DoesNotExist
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* RE: [BlueZ,v1,7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description
2020-08-18 22:26 ` [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description Miao-chen Chou
@ 2020-08-18 22:50 ` bluez.test.bot
2020-09-08 17:39 ` [BlueZ PATCH v1 7/7] " Luiz Augusto von Dentz
1 sibling, 0 replies; 22+ messages in thread
From: bluez.test.bot @ 2020-08-18 22:50 UTC (permalink / raw)
To: linux-bluetooth, mcchou
[-- Attachment #1: Type: text/plain, Size: 438 bytes --]
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:
checkgitlint Failed
Outputs:
1: T1 Title exceeds max length (75>72): "doc/advertisement-monitor-api: Update Advertisement Monitor API description"
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description
2020-08-18 22:26 ` [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description Miao-chen Chou
2020-08-18 22:50 ` [BlueZ,v1,7/7] " bluez.test.bot
@ 2020-09-08 17:39 ` Luiz Augusto von Dentz
2020-09-10 4:53 ` Miao-chen Chou
1 sibling, 1 reply; 22+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-08 17:39 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung
Hi Miao,
On Tue, Aug 18, 2020 at 3:34 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> This modifies the following description to Advertisement Monitor API.
> - Add org.bluez.Error.Failed to RegisterMonitor() method.
> - Add more description about the usage of RegisterMonitor() and
> UnregisterMonitor() methods.
> - Add description about the ranges for the fields in property
> RSSIThresholdsAndTimers.
>
> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> ---
>
> doc/advertisement-monitor-api.txt | 34 +++++++++++++++++++++++--------
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> index 74adbfae9..e09b6fd25 100644
> --- a/doc/advertisement-monitor-api.txt
> +++ b/doc/advertisement-monitor-api.txt
> @@ -49,7 +49,7 @@ Properties string Type [read-only]
> org.bluez.AdvertisementMonitorManager1 for the available
> options.
>
> - (Int16, Uint16, Int16, Uint16) RSSIThreshholdsAndTimers [read-only, optional]
> + (Int16, Uint16, Int16, Uint16) RSSIThresholdsAndTimers [read-only, optional]
>
> This contains HighRSSIThreshold, HighRSSIThresholdTimer,
> LowRSSIThreshold, LowRSSIThresholdTimer in order. The
> @@ -66,7 +66,11 @@ Properties string Type [read-only]
> RSSIs of the received advertisement(s) during
> LowRSSIThresholdTimer do not reach LowRSSIThreshold.
>
> - array{(uint8, uint8, string)} Patterns [read-only, optional]
> + The valid range of a RSSI is -127 to +20 dBm while 127
> + dBm indicates unset. The valid range of a timer is 1 to
> + 300 seconds while 0 indicates unset.
> +
> + array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
>
> If Type is set to 0x01, this must exist and has at least
> one entry in the array.
> @@ -80,8 +84,9 @@ Properties string Type [read-only]
> See https://www.bluetooth.com/specifications/
> assigned-numbers/generic-access-profile/ for
> the possible allowed value.
> - string content_of_pattern
> - This is the value of the pattern.
> + array{byte} content_of_pattern
> + This is the value of the pattern. The maximum
> + length of the bytes is 31.
>
> Advertisement Monitor Manager hierarchy
> =======================================
> @@ -91,20 +96,31 @@ Object path /org/bluez/{hci0,hci1,...}
>
> Methods void RegisterMonitor(object application)
>
> - This registers a hierarchy of advertisement monitors.
> + This registers the root path of a hierarchy of
> + advertisement monitors.
> The application object path together with the D-Bus
> system bus connection ID define the identification of
> the application registering advertisement monitors.
> + Once a root path is registered by a client via this
> + method, the client can freely expose/unexpose
> + advertisement monitors without re-registering the root
> + path again. After use, the client should call
> + UnregisterMonitor() method to invalidate the
> + advertisement monitors.
>
> Possible errors: org.bluez.Error.InvalidArguments
> org.bluez.Error.AlreadyExists
> + org.bluez.Error.Failed
>
> void UnregisterMonitor(object application)
>
> - This unregisters advertisement monitors that have been
> - previously registered. The object path parameter must
> - match the same value that has been used on
> - registration.
> + This unregisters a hierarchy of advertisement monitors
> + that has been previously registered. The object path
> + parameter must match the same value that has been used
> + on registration. Upon unregistration, the advertisement
> + monitor(s) should expect to receive Release() method as
> + the signal that the advertisement monitor(s) has been
> + deactivated.
>
> Possible errors: org.bluez.Error.InvalidArguments
> org.bluez.Error.DoesNotExist
> --
> 2.26.2
These are still experimental so you will need to use EXPERIMENTAL
version when declaring the methods/properties so it only gets enabled
when the experimental flag is passed to bluetoothd.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description
2020-09-08 17:39 ` [BlueZ PATCH v1 7/7] " Luiz Augusto von Dentz
@ 2020-09-10 4:53 ` Miao-chen Chou
2020-09-10 17:43 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 22+ messages in thread
From: Miao-chen Chou @ 2020-09-10 4:53 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung
Hi Luiz,
On Tue, Sep 8, 2020 at 10:40 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Aug 18, 2020 at 3:34 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > This modifies the following description to Advertisement Monitor API.
> > - Add org.bluez.Error.Failed to RegisterMonitor() method.
> > - Add more description about the usage of RegisterMonitor() and
> > UnregisterMonitor() methods.
> > - Add description about the ranges for the fields in property
> > RSSIThresholdsAndTimers.
> >
> > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > ---
> >
> > doc/advertisement-monitor-api.txt | 34 +++++++++++++++++++++++--------
> > 1 file changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> > index 74adbfae9..e09b6fd25 100644
> > --- a/doc/advertisement-monitor-api.txt
> > +++ b/doc/advertisement-monitor-api.txt
> > @@ -49,7 +49,7 @@ Properties string Type [read-only]
> > org.bluez.AdvertisementMonitorManager1 for the available
> > options.
> >
> > - (Int16, Uint16, Int16, Uint16) RSSIThreshholdsAndTimers [read-only, optional]
> > + (Int16, Uint16, Int16, Uint16) RSSIThresholdsAndTimers [read-only, optional]
> >
> > This contains HighRSSIThreshold, HighRSSIThresholdTimer,
> > LowRSSIThreshold, LowRSSIThresholdTimer in order. The
> > @@ -66,7 +66,11 @@ Properties string Type [read-only]
> > RSSIs of the received advertisement(s) during
> > LowRSSIThresholdTimer do not reach LowRSSIThreshold.
> >
> > - array{(uint8, uint8, string)} Patterns [read-only, optional]
> > + The valid range of a RSSI is -127 to +20 dBm while 127
> > + dBm indicates unset. The valid range of a timer is 1 to
> > + 300 seconds while 0 indicates unset.
> > +
> > + array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
> >
> > If Type is set to 0x01, this must exist and has at least
> > one entry in the array.
> > @@ -80,8 +84,9 @@ Properties string Type [read-only]
> > See https://www.bluetooth.com/specifications/
> > assigned-numbers/generic-access-profile/ for
> > the possible allowed value.
> > - string content_of_pattern
> > - This is the value of the pattern.
> > + array{byte} content_of_pattern
> > + This is the value of the pattern. The maximum
> > + length of the bytes is 31.
> >
> > Advertisement Monitor Manager hierarchy
> > =======================================
> > @@ -91,20 +96,31 @@ Object path /org/bluez/{hci0,hci1,...}
> >
> > Methods void RegisterMonitor(object application)
> >
> > - This registers a hierarchy of advertisement monitors.
> > + This registers the root path of a hierarchy of
> > + advertisement monitors.
> > The application object path together with the D-Bus
> > system bus connection ID define the identification of
> > the application registering advertisement monitors.
> > + Once a root path is registered by a client via this
> > + method, the client can freely expose/unexpose
> > + advertisement monitors without re-registering the root
> > + path again. After use, the client should call
> > + UnregisterMonitor() method to invalidate the
> > + advertisement monitors.
> >
> > Possible errors: org.bluez.Error.InvalidArguments
> > org.bluez.Error.AlreadyExists
> > + org.bluez.Error.Failed
> >
> > void UnregisterMonitor(object application)
> >
> > - This unregisters advertisement monitors that have been
> > - previously registered. The object path parameter must
> > - match the same value that has been used on
> > - registration.
> > + This unregisters a hierarchy of advertisement monitors
> > + that has been previously registered. The object path
> > + parameter must match the same value that has been used
> > + on registration. Upon unregistration, the advertisement
> > + monitor(s) should expect to receive Release() method as
> > + the signal that the advertisement monitor(s) has been
> > + deactivated.
> >
> > Possible errors: org.bluez.Error.InvalidArguments
> > org.bluez.Error.DoesNotExist
> > --
> > 2.26.2
>
> These are still experimental so you will need to use EXPERIMENTAL
> version when declaring the methods/properties so it only gets enabled
> when the experimental flag is passed to bluetoothd.
g_dbus_register_interface() does not allow to have all methods and
properties marked as experimental, so at least SupportedFeatures
should be non-experimental.
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Miao
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description
2020-09-10 4:53 ` Miao-chen Chou
@ 2020-09-10 17:43 ` Luiz Augusto von Dentz
2020-09-10 23:19 ` Miao-chen Chou
0 siblings, 1 reply; 22+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-10 17:43 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung
Hi Miao,
On Wed, Sep 9, 2020 at 9:53 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> On Tue, Sep 8, 2020 at 10:40 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Miao,
> >
> > On Tue, Aug 18, 2020 at 3:34 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > >
> > > This modifies the following description to Advertisement Monitor API.
> > > - Add org.bluez.Error.Failed to RegisterMonitor() method.
> > > - Add more description about the usage of RegisterMonitor() and
> > > UnregisterMonitor() methods.
> > > - Add description about the ranges for the fields in property
> > > RSSIThresholdsAndTimers.
> > >
> > > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> > > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > > ---
> > >
> > > doc/advertisement-monitor-api.txt | 34 +++++++++++++++++++++++--------
> > > 1 file changed, 25 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> > > index 74adbfae9..e09b6fd25 100644
> > > --- a/doc/advertisement-monitor-api.txt
> > > +++ b/doc/advertisement-monitor-api.txt
> > > @@ -49,7 +49,7 @@ Properties string Type [read-only]
> > > org.bluez.AdvertisementMonitorManager1 for the available
> > > options.
> > >
> > > - (Int16, Uint16, Int16, Uint16) RSSIThreshholdsAndTimers [read-only, optional]
> > > + (Int16, Uint16, Int16, Uint16) RSSIThresholdsAndTimers [read-only, optional]
> > >
> > > This contains HighRSSIThreshold, HighRSSIThresholdTimer,
> > > LowRSSIThreshold, LowRSSIThresholdTimer in order. The
> > > @@ -66,7 +66,11 @@ Properties string Type [read-only]
> > > RSSIs of the received advertisement(s) during
> > > LowRSSIThresholdTimer do not reach LowRSSIThreshold.
> > >
> > > - array{(uint8, uint8, string)} Patterns [read-only, optional]
> > > + The valid range of a RSSI is -127 to +20 dBm while 127
> > > + dBm indicates unset. The valid range of a timer is 1 to
> > > + 300 seconds while 0 indicates unset.
> > > +
> > > + array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
> > >
> > > If Type is set to 0x01, this must exist and has at least
> > > one entry in the array.
> > > @@ -80,8 +84,9 @@ Properties string Type [read-only]
> > > See https://www.bluetooth.com/specifications/
> > > assigned-numbers/generic-access-profile/ for
> > > the possible allowed value.
> > > - string content_of_pattern
> > > - This is the value of the pattern.
> > > + array{byte} content_of_pattern
> > > + This is the value of the pattern. The maximum
> > > + length of the bytes is 31.
> > >
> > > Advertisement Monitor Manager hierarchy
> > > =======================================
> > > @@ -91,20 +96,31 @@ Object path /org/bluez/{hci0,hci1,...}
> > >
> > > Methods void RegisterMonitor(object application)
> > >
> > > - This registers a hierarchy of advertisement monitors.
> > > + This registers the root path of a hierarchy of
> > > + advertisement monitors.
> > > The application object path together with the D-Bus
> > > system bus connection ID define the identification of
> > > the application registering advertisement monitors.
> > > + Once a root path is registered by a client via this
> > > + method, the client can freely expose/unexpose
> > > + advertisement monitors without re-registering the root
> > > + path again. After use, the client should call
> > > + UnregisterMonitor() method to invalidate the
> > > + advertisement monitors.
> > >
> > > Possible errors: org.bluez.Error.InvalidArguments
> > > org.bluez.Error.AlreadyExists
> > > + org.bluez.Error.Failed
> > >
> > > void UnregisterMonitor(object application)
> > >
> > > - This unregisters advertisement monitors that have been
> > > - previously registered. The object path parameter must
> > > - match the same value that has been used on
> > > - registration.
> > > + This unregisters a hierarchy of advertisement monitors
> > > + that has been previously registered. The object path
> > > + parameter must match the same value that has been used
> > > + on registration. Upon unregistration, the advertisement
> > > + monitor(s) should expect to receive Release() method as
> > > + the signal that the advertisement monitor(s) has been
> > > + deactivated.
> > >
> > > Possible errors: org.bluez.Error.InvalidArguments
> > > org.bluez.Error.DoesNotExist
> > > --
> > > 2.26.2
> >
> > These are still experimental so you will need to use EXPERIMENTAL
> > version when declaring the methods/properties so it only gets enabled
> > when the experimental flag is passed to bluetoothd.
> g_dbus_register_interface() does not allow to have all methods and
> properties marked as experimental, so at least SupportedFeatures
> should be non-experimental.
You will need to do something like the following:
if (g_dbus_get_flags() & G_DBUS_FLAG_ENABLE_EXPERIMENTAL)
You can see how it was used in adv_manager:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=d6e9539e31c6bb5afd39ec6f09c518d232e6345d
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Miao
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description
2020-09-10 17:43 ` Luiz Augusto von Dentz
@ 2020-09-10 23:19 ` Miao-chen Chou
0 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-09-10 23:19 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung
Hi Luiz,
On Thu, Sep 10, 2020 at 10:43 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Wed, Sep 9, 2020 at 9:53 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, Sep 8, 2020 at 10:40 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Miao,
> > >
> > > On Tue, Aug 18, 2020 at 3:34 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > > >
> > > > This modifies the following description to Advertisement Monitor API.
> > > > - Add org.bluez.Error.Failed to RegisterMonitor() method.
> > > > - Add more description about the usage of RegisterMonitor() and
> > > > UnregisterMonitor() methods.
> > > > - Add description about the ranges for the fields in property
> > > > RSSIThresholdsAndTimers.
> > > >
> > > > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> > > > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > > > ---
> > > >
> > > > doc/advertisement-monitor-api.txt | 34 +++++++++++++++++++++++--------
> > > > 1 file changed, 25 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> > > > index 74adbfae9..e09b6fd25 100644
> > > > --- a/doc/advertisement-monitor-api.txt
> > > > +++ b/doc/advertisement-monitor-api.txt
> > > > @@ -49,7 +49,7 @@ Properties string Type [read-only]
> > > > org.bluez.AdvertisementMonitorManager1 for the available
> > > > options.
> > > >
> > > > - (Int16, Uint16, Int16, Uint16) RSSIThreshholdsAndTimers [read-only, optional]
> > > > + (Int16, Uint16, Int16, Uint16) RSSIThresholdsAndTimers [read-only, optional]
> > > >
> > > > This contains HighRSSIThreshold, HighRSSIThresholdTimer,
> > > > LowRSSIThreshold, LowRSSIThresholdTimer in order. The
> > > > @@ -66,7 +66,11 @@ Properties string Type [read-only]
> > > > RSSIs of the received advertisement(s) during
> > > > LowRSSIThresholdTimer do not reach LowRSSIThreshold.
> > > >
> > > > - array{(uint8, uint8, string)} Patterns [read-only, optional]
> > > > + The valid range of a RSSI is -127 to +20 dBm while 127
> > > > + dBm indicates unset. The valid range of a timer is 1 to
> > > > + 300 seconds while 0 indicates unset.
> > > > +
> > > > + array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
> > > >
> > > > If Type is set to 0x01, this must exist and has at least
> > > > one entry in the array.
> > > > @@ -80,8 +84,9 @@ Properties string Type [read-only]
> > > > See https://www.bluetooth.com/specifications/
> > > > assigned-numbers/generic-access-profile/ for
> > > > the possible allowed value.
> > > > - string content_of_pattern
> > > > - This is the value of the pattern.
> > > > + array{byte} content_of_pattern
> > > > + This is the value of the pattern. The maximum
> > > > + length of the bytes is 31.
> > > >
> > > > Advertisement Monitor Manager hierarchy
> > > > =======================================
> > > > @@ -91,20 +96,31 @@ Object path /org/bluez/{hci0,hci1,...}
> > > >
> > > > Methods void RegisterMonitor(object application)
> > > >
> > > > - This registers a hierarchy of advertisement monitors.
> > > > + This registers the root path of a hierarchy of
> > > > + advertisement monitors.
> > > > The application object path together with the D-Bus
> > > > system bus connection ID define the identification of
> > > > the application registering advertisement monitors.
> > > > + Once a root path is registered by a client via this
> > > > + method, the client can freely expose/unexpose
> > > > + advertisement monitors without re-registering the root
> > > > + path again. After use, the client should call
> > > > + UnregisterMonitor() method to invalidate the
> > > > + advertisement monitors.
> > > >
> > > > Possible errors: org.bluez.Error.InvalidArguments
> > > > org.bluez.Error.AlreadyExists
> > > > + org.bluez.Error.Failed
> > > >
> > > > void UnregisterMonitor(object application)
> > > >
> > > > - This unregisters advertisement monitors that have been
> > > > - previously registered. The object path parameter must
> > > > - match the same value that has been used on
> > > > - registration.
> > > > + This unregisters a hierarchy of advertisement monitors
> > > > + that has been previously registered. The object path
> > > > + parameter must match the same value that has been used
> > > > + on registration. Upon unregistration, the advertisement
> > > > + monitor(s) should expect to receive Release() method as
> > > > + the signal that the advertisement monitor(s) has been
> > > > + deactivated.
> > > >
> > > > Possible errors: org.bluez.Error.InvalidArguments
> > > > org.bluez.Error.DoesNotExist
> > > > --
> > > > 2.26.2
> > >
> > > These are still experimental so you will need to use EXPERIMENTAL
> > > version when declaring the methods/properties so it only gets enabled
> > > when the experimental flag is passed to bluetoothd.
> > g_dbus_register_interface() does not allow to have all methods and
> > properties marked as experimental, so at least SupportedFeatures
> > should be non-experimental.
>
> You will need to do something like the following:
>
> if (g_dbus_get_flags() & G_DBUS_FLAG_ENABLE_EXPERIMENTAL)
>
> You can see how it was used in adv_manager:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=d6e9539e31c6bb5afd39ec6f09c518d232e6345d
>
Thanks for the pointer. Please see v4 for the changes.
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Miao
>
>
>
> --
> Luiz Augusto von Dentz
Regards,
Miao
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [BlueZ,v1,1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
` (5 preceding siblings ...)
2020-08-18 22:26 ` [BlueZ PATCH v1 7/7] doc/advertisement-monitor-api: Update Advertisement Monitor API description Miao-chen Chou
@ 2020-08-18 22:50 ` bluez.test.bot
2020-08-18 22:53 ` bluez.test.bot
2020-09-08 17:19 ` [BlueZ PATCH v1 1/7] " Luiz Augusto von Dentz
8 siblings, 0 replies; 22+ messages in thread
From: bluez.test.bot @ 2020-08-18 22:50 UTC (permalink / raw)
To: linux-bluetooth, mcchou
[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]
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
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [BlueZ,v1,1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
` (6 preceding siblings ...)
2020-08-18 22:50 ` [BlueZ,v1,1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface bluez.test.bot
@ 2020-08-18 22:53 ` bluez.test.bot
2020-09-08 17:19 ` [BlueZ PATCH v1 1/7] " Luiz Augusto von Dentz
8 siblings, 0 replies; 22+ messages in thread
From: bluez.test.bot @ 2020-08-18 22:53 UTC (permalink / raw)
To: linux-bluetooth, mcchou
[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface
2020-08-18 22:26 [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface Miao-chen Chou
` (7 preceding siblings ...)
2020-08-18 22:53 ` bluez.test.bot
@ 2020-09-08 17:19 ` Luiz Augusto von Dentz
2020-09-10 4:52 ` Miao-chen Chou
8 siblings, 1 reply; 22+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-08 17:19 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung, Abhishek Pandit-Subedi
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}
> 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
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface
2020-09-08 17:19 ` [BlueZ PATCH v1 1/7] " Luiz Augusto von Dentz
@ 2020-09-10 4:52 ` Miao-chen Chou
2020-09-10 17:31 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 22+ messages in thread
From: Miao-chen Chou @ 2020-09-10 4:52 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung, Abhishek Pandit-Subedi
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface
2020-09-10 4:52 ` Miao-chen Chou
@ 2020-09-10 17:31 ` Luiz Augusto von Dentz
2020-09-10 23:18 ` Miao-chen Chou
0 siblings, 1 reply; 22+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-10 17:31 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung, Abhishek Pandit-Subedi
Hi Miao,
On Wed, Sep 9, 2020 at 9:52 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> 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".
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
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [BlueZ PATCH v1 1/7] adv_monitor: Introduce org.bluez.AdvertisementMonitorManager1 interface
2020-09-10 17:31 ` Luiz Augusto von Dentz
@ 2020-09-10 23:18 ` Miao-chen Chou
0 siblings, 0 replies; 22+ messages in thread
From: Miao-chen Chou @ 2020-09-10 23:18 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Alain Michaud, Manish Mandlik,
Luiz Augusto von Dentz, Howard Chung, Abhishek Pandit-Subedi
Hi Luiz,
On Thu, Sep 10, 2020 at 10:31 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Wed, Sep 9, 2020 at 9:52 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > 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".
>
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread