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