| Message ID | 20201006171333.BlueZ.v6.2.I578ae5e76fcf7243206a27d4f5a25783662a5f14@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi Miao, On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > This implements create an entry point in adapter to start the matching of > Adv based on all monitors and invoke the RSSI tracking for Adv reporting. > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org> > --- > > Changes in v6: > - Fix the termination condition of AD data paring and remove unnecessary > length check > > Changes in v5: > - Remove unittest helper functions > > Changes in v3: > - Remove unused variables > - Fix signature of queue_find() > > src/adapter.c | 35 ++++++-- > src/adv_monitor.c | 219 ++++++++++++++++++++++++++++++++++++++++------ > src/adv_monitor.h | 12 +++ > 3 files changed, 234 insertions(+), 32 deletions(-) > > diff --git a/src/adapter.c b/src/adapter.c > index 6d0114a6b..fdd9b3808 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -6601,6 +6601,15 @@ static void update_found_devices(struct btd_adapter *adapter, > bool name_known, discoverable; > char addr[18]; > bool duplicate = false; > + GSList *matched_monitors; > + > + /* During the background scanning, update the device only when the data > + * match at least one Adv monitor > + */ > + matched_monitors = btd_adv_monitor_content_filter( > + adapter->adv_monitor_manager, data, data_len); > + if (!adapter->discovering && !matched_monitors) > + return; > > memset(&eir_data, 0, sizeof(eir_data)); > eir_parse(&eir_data, data, data_len); > @@ -6646,18 +6655,22 @@ static void update_found_devices(struct btd_adapter *adapter, > device_store_cached_name(dev, eir_data.name); > > /* > - * Only skip devices that are not connected, are temporary and there > - * is no active discovery session ongoing. > + * Only skip devices that are not connected, are temporary, and there > + * is no active discovery session ongoing and no matched Adv monitors > */ > - if (!btd_device_is_connected(dev) && (device_is_temporary(dev) && > - !adapter->discovery_list)) { > + if (!btd_device_is_connected(dev) && > + (device_is_temporary(dev) && !adapter->discovery_list) && > + !matched_monitors) { > eir_data_free(&eir_data); > return; > } > > - /* Don't continue if not discoverable or if filter don't match */ > - if (!discoverable || (adapter->filtered_discovery && > - !is_filter_match(adapter->discovery_list, &eir_data, rssi))) { > + /* If there is no matched Adv monitors, don't continue if not > + * discoverable or if active discovery filter don't match. > + */ > + if (!matched_monitors && (!discoverable || > + (adapter->filtered_discovery && !is_filter_match( > + adapter->discovery_list, &eir_data, rssi)))) { > eir_data_free(&eir_data); > return; > } > @@ -6714,6 +6727,14 @@ static void update_found_devices(struct btd_adapter *adapter, > > eir_data_free(&eir_data); > > + /* After the device is updated, notify the matched Adv monitors */ > + if (matched_monitors) { > + btd_adv_monitor_notify_monitors(adapter->adv_monitor_manager, > + dev, rssi, matched_monitors); > + g_slist_free(matched_monitors); > + matched_monitors = NULL; > + } > + > /* > * Only if at least one client has requested discovery, maintain > * list of found devices and name confirming for legacy devices. > diff --git a/src/adv_monitor.c b/src/adv_monitor.c > index 31ed30a00..fcb127cd4 100644 > --- a/src/adv_monitor.c > +++ b/src/adv_monitor.c > @@ -29,15 +29,12 @@ > #include "device.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" > > -static void monitor_device_free(void *data); > - > #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1" > #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1" > > @@ -84,7 +81,7 @@ enum monitor_state { > MONITOR_STATE_HONORED, /* Accepted by kernel */ > }; > > -struct pattern { > +struct btd_adv_monitor_pattern { > uint8_t ad_type; > uint8_t offset; > uint8_t length; > @@ -133,6 +130,23 @@ struct app_match_data { > const char *path; > }; > > +struct adv_content_filter_info { > + uint8_t eir_len; > + const uint8_t *eir; > + > + bool matched; /* Intermediate state per monitor */ > + GSList *matched_monitors; /* List of matched monitors */ Please use struct queue on new code. > +}; > + > +struct adv_rssi_filter_info { > + struct btd_device *device; > + int8_t rssi; > +}; > + > +static void monitor_device_free(void *data); > +static void adv_monitor_filter_rssi(struct adv_monitor *monitor, > + struct btd_device *device, int8_t rssi); > + > const struct adv_monitor_type { > enum monitor_type type; > const char *name; > @@ -155,7 +169,7 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply) > /* Frees a pattern */ > static void pattern_free(void *data) > { > - struct pattern *pattern = data; > + struct btd_adv_monitor_pattern *pattern = data; > > if (!pattern) > return; > @@ -435,6 +449,36 @@ failed: > return false; > } > > +/* Allocates and initiates a pattern with the given content */ > +static struct btd_adv_monitor_pattern *pattern_create( > + uint8_t ad_type, uint8_t offset, uint8_t length, const uint8_t *value) > +{ > + struct btd_adv_monitor_pattern *pattern; > + > + if (offset > BT_AD_MAX_DATA_LEN - 1) > + return NULL; > + > + if ((ad_type > BT_AD_3D_INFO_DATA && > + ad_type != BT_AD_MANUFACTURER_DATA) || > + ad_type < BT_AD_FLAGS) { > + return NULL; > + } > + > + if (!value || !length || offset + length > BT_AD_MAX_DATA_LEN) > + return NULL; > + > + pattern = new0(struct btd_adv_monitor_pattern, 1); > + if (!pattern) > + return NULL; > + > + pattern->ad_type = ad_type; > + pattern->offset = offset; > + pattern->length = length; > + memcpy(pattern->value, value, pattern->length); I wonder why you didn't add pattern matching into bt_ad directly, that should make it simpler to unit test such feature and enable using in other tools as well. > + return pattern; > +} > + > /* Retrieves Patterns from the remote Adv Monitor object, verifies the values > * and update the local Adv Monitor > */ > @@ -464,7 +508,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path) > int value_len; > uint8_t *value; > uint8_t offset, ad_type; > - struct pattern *pattern; > + struct btd_adv_monitor_pattern *pattern; > DBusMessageIter struct_iter, value_iter; > > dbus_message_iter_recurse(&array_iter, &struct_iter); > @@ -496,28 +540,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path) > 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 = new0(struct pattern, 1); > + pattern = pattern_create(ad_type, offset, value_len, value); > if (!pattern) > goto failed; > > - pattern->ad_type = ad_type; > - pattern->offset = offset; > - pattern->length = value_len; > - memcpy(pattern->value, value, pattern->length); > - > queue_push_tail(monitor->patterns, pattern); > > dbus_message_iter_next(&array_iter); > @@ -952,6 +978,149 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager) > manager_destroy(manager); > } > > +static bool pattern_match(const uint8_t *eir, uint8_t eir_len, > + const struct btd_adv_monitor_pattern *pattern) > +{ > + const uint8_t *data; > + uint8_t idx = 0; > + uint8_t field_len, data_len, data_type; > + > + while (idx < eir_len - 1) { > + field_len = eir[0]; > + > + /* Check for the end of EIR */ > + if (field_len == 0) > + break; > + > + idx += field_len + 1; > + > + /* Do not continue filtering if got incorrect length */ > + if (idx > eir_len) > + break; > + > + data = &eir[2]; > + data_type = eir[1]; > + data_len = field_len - 1; > + > + eir += field_len + 1; > + > + if (data_type != pattern->ad_type) > + continue; > + > + if (data_len < pattern->offset + pattern->length) > + continue; > + > + if (!memcmp(data + pattern->offset, pattern->value, > + pattern->length)) > + return true; > + } Perhaps something like: struct bt_ad *bt_ad_new_with_data(uint8_t *data, uint8_t len); and bool bt_ad_has_pattern(struct bt_ad *ad, const struct bt_ad_pattern *pattern); > + return false; > +} > + > +/* Processes the content matching based on a pattern */ > +static void adv_match_per_pattern(void *data, void *user_data) > +{ > + struct btd_adv_monitor_pattern *pattern = data; > + struct adv_content_filter_info *info = user_data; > + > + if (!pattern || info->matched) > + return; > + > + info->matched = pattern_match(info->eir, info->eir_len, pattern); > +} > + > +/* Processes the content matching based pattern(s) of a monitor */ > +static void adv_match_per_monitor(void *data, void *user_data) > +{ > + struct adv_monitor *monitor = data; > + struct adv_content_filter_info *info = user_data; > + > + if (!monitor && monitor->state != MONITOR_STATE_HONORED) > + return; > + > + /* Reset the intermediate matched status */ > + info->matched = false; > + > + if (monitor->type == MONITOR_TYPE_OR_PATTERNS) { > + queue_foreach(monitor->patterns, adv_match_per_pattern, info); > + if (info->matched) > + goto matched; > + } > + > + return; > + > +matched: > + info->matched_monitors = g_slist_prepend(info->matched_monitors, > + monitor); > +} > + > +/* Processes the content matching for the monitor(s) of an app */ > +static void adv_match_per_app(void *data, void *user_data) > +{ > + struct adv_monitor_app *app = data; > + > + if (!app) > + return; > + > + queue_foreach(app->monitors, adv_match_per_monitor, user_data); > +} > + > +/* Processes the content matching for every app without RSSI filtering and > + * notifying monitors. The caller is responsible of releasing the memory of the > + * list but not the data. > + * Returns the list of monitors whose content match eir. > + */ > +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager, > + const uint8_t *eir, uint8_t eir_len) > +{ > + struct adv_content_filter_info info; > + > + if (!manager || !eir || !eir_len) > + return NULL; > + > + info.eir_len = eir_len; > + info.eir = eir; > + info.matched_monitors = NULL; > + > + queue_foreach(manager->apps, adv_match_per_app, &info); > + > + return info.matched_monitors; > +} > + > +/* Wraps adv_monitor_filter_rssi() to processes the content-matched monitor with > + * RSSI filtering and notifies it on device found/lost event > + */ > +static void monitor_filter_rssi(gpointer a, gpointer b) > +{ > + struct adv_monitor *monitor = a; > + struct adv_rssi_filter_info *info = b; > + > + if (!monitor || !info) > + return; > + > + adv_monitor_filter_rssi(monitor, info->device, info->rssi); > +} > + > +/* Processes every content-matched monitor with RSSI filtering and notifies on > + * device found/lost event. The caller is responsible of releasing the memory > + * of matched_monitors list but not its data. > + */ > +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager, > + struct btd_device *device, int8_t rssi, > + GSList *matched_monitors) > +{ > + struct adv_rssi_filter_info info; > + > + if (!manager || !device || !matched_monitors) > + return; > + > + info.device = device; > + info.rssi = rssi; > + > + g_slist_foreach(matched_monitors, monitor_filter_rssi, &info); > +} > + > /* Matches a device based on btd_device object */ > static bool monitor_device_match(const void *a, const void *b) > { > diff --git a/src/adv_monitor.h b/src/adv_monitor.h > index 13d5d7282..e2482e11e 100644 > --- a/src/adv_monitor.h > +++ b/src/adv_monitor.h > @@ -11,16 +11,28 @@ > #ifndef __ADV_MONITOR_H > #define __ADV_MONITOR_H > > +#include <glib.h> > + > +#include "src/shared/ad.h" > + > struct mgmt; > struct btd_device; > struct btd_adapter; > struct btd_adv_monitor_manager; > +struct btd_adv_monitor_pattern; > > 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); > > +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager, > + const uint8_t *eir, uint8_t eir_len); > + > +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager, > + struct btd_device *device, int8_t rssi, > + GSList *matched_monitors); > + > void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager, > struct btd_device *device); > > -- > 2.26.2 > -- Luiz Augusto von Dentz
Hi Luiz, I did think of adding pattern to bt_ad at the beginning, but here are reasons why I ended up with hosting the definition of pattern in adv_monitor. (1) Pattern is specific to monitoring purpose. An advertisement should not include patterns as its fields due to that fact that a pattern hosts an offset. So I wasn't sure about its justification to be placed in shared/ad. But if you foresee that it would be reused, then I am more than happy to add it to shared/ad. (2) Introducing helpers as you suggested below indeed make it more unittestable. However, it also implied that EIR data (this is in fact AD data) needs to be parsed into a new bt_ad for pattern comparison, and I didn't see an obvious benefit of converting EIR data into a bt_ad just for the comparison. Maybe we can add a struct bt_ad_pattern in shared/ad.h and introduce the following two functions. What do you think? struct bt_ad_pattern *bt_ad_pattern_new(uint8_t type, size_t offset, size_t len, const void *data); /* |data| is one single AD data field so that we can avoid converting EIR data to bt_ad */ bool bt_ad_pattern bt_ad_pattern_match(struct bt_ad_pattern *pattern, void *data, size_t len); On Tue, Oct 6, 2020 at 11:21 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Miao, > > On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > > > This implements create an entry point in adapter to start the matching of > > Adv based on all monitors and invoke the RSSI tracking for Adv reporting. > > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Manish Mandlik <mmandlik@chromium.org> > > --- > > > > Changes in v6: > > - Fix the termination condition of AD data paring and remove unnecessary > > length check > > > > Changes in v5: > > - Remove unittest helper functions > > > > Changes in v3: > > - Remove unused variables > > - Fix signature of queue_find() > > > > src/adapter.c | 35 ++++++-- > > src/adv_monitor.c | 219 ++++++++++++++++++++++++++++++++++++++++------ > > src/adv_monitor.h | 12 +++ > > 3 files changed, 234 insertions(+), 32 deletions(-) > > > > diff --git a/src/adapter.c b/src/adapter.c > > index 6d0114a6b..fdd9b3808 100644 > > --- a/src/adapter.c > > +++ b/src/adapter.c > > @@ -6601,6 +6601,15 @@ static void update_found_devices(struct btd_adapter *adapter, > > bool name_known, discoverable; > > char addr[18]; > > bool duplicate = false; > > + GSList *matched_monitors; > > + > > + /* During the background scanning, update the device only when the data > > + * match at least one Adv monitor > > + */ > > + matched_monitors = btd_adv_monitor_content_filter( > > + adapter->adv_monitor_manager, data, data_len); > > + if (!adapter->discovering && !matched_monitors) > > + return; > > > > memset(&eir_data, 0, sizeof(eir_data)); > > eir_parse(&eir_data, data, data_len); > > @@ -6646,18 +6655,22 @@ static void update_found_devices(struct btd_adapter *adapter, > > device_store_cached_name(dev, eir_data.name); > > > > /* > > - * Only skip devices that are not connected, are temporary and there > > - * is no active discovery session ongoing. > > + * Only skip devices that are not connected, are temporary, and there > > + * is no active discovery session ongoing and no matched Adv monitors > > */ > > - if (!btd_device_is_connected(dev) && (device_is_temporary(dev) && > > - !adapter->discovery_list)) { > > + if (!btd_device_is_connected(dev) && > > + (device_is_temporary(dev) && !adapter->discovery_list) && > > + !matched_monitors) { > > eir_data_free(&eir_data); > > return; > > } > > > > - /* Don't continue if not discoverable or if filter don't match */ > > - if (!discoverable || (adapter->filtered_discovery && > > - !is_filter_match(adapter->discovery_list, &eir_data, rssi))) { > > + /* If there is no matched Adv monitors, don't continue if not > > + * discoverable or if active discovery filter don't match. > > + */ > > + if (!matched_monitors && (!discoverable || > > + (adapter->filtered_discovery && !is_filter_match( > > + adapter->discovery_list, &eir_data, rssi)))) { > > eir_data_free(&eir_data); > > return; > > } > > @@ -6714,6 +6727,14 @@ static void update_found_devices(struct btd_adapter *adapter, > > > > eir_data_free(&eir_data); > > > > + /* After the device is updated, notify the matched Adv monitors */ > > + if (matched_monitors) { > > + btd_adv_monitor_notify_monitors(adapter->adv_monitor_manager, > > + dev, rssi, matched_monitors); > > + g_slist_free(matched_monitors); > > + matched_monitors = NULL; > > + } > > + > > /* > > * Only if at least one client has requested discovery, maintain > > * list of found devices and name confirming for legacy devices. > > diff --git a/src/adv_monitor.c b/src/adv_monitor.c > > index 31ed30a00..fcb127cd4 100644 > > --- a/src/adv_monitor.c > > +++ b/src/adv_monitor.c > > @@ -29,15 +29,12 @@ > > #include "device.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" > > > > -static void monitor_device_free(void *data); > > - > > #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1" > > #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1" > > > > @@ -84,7 +81,7 @@ enum monitor_state { > > MONITOR_STATE_HONORED, /* Accepted by kernel */ > > }; > > > > -struct pattern { > > +struct btd_adv_monitor_pattern { > > uint8_t ad_type; > > uint8_t offset; > > uint8_t length; > > @@ -133,6 +130,23 @@ struct app_match_data { > > const char *path; > > }; > > > > +struct adv_content_filter_info { > > + uint8_t eir_len; > > + const uint8_t *eir; > > + > > + bool matched; /* Intermediate state per monitor */ > > + GSList *matched_monitors; /* List of matched monitors */ > > Please use struct queue on new code. Addressed in my local v7. > > > +}; > > + > > +struct adv_rssi_filter_info { > > + struct btd_device *device; > > + int8_t rssi; > > +}; > > + > > +static void monitor_device_free(void *data); > > +static void adv_monitor_filter_rssi(struct adv_monitor *monitor, > > + struct btd_device *device, int8_t rssi); > > + > > const struct adv_monitor_type { > > enum monitor_type type; > > const char *name; > > @@ -155,7 +169,7 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply) > > /* Frees a pattern */ > > static void pattern_free(void *data) > > { > > - struct pattern *pattern = data; > > + struct btd_adv_monitor_pattern *pattern = data; > > > > if (!pattern) > > return; > > @@ -435,6 +449,36 @@ failed: > > return false; > > } > > > > +/* Allocates and initiates a pattern with the given content */ > > +static struct btd_adv_monitor_pattern *pattern_create( > > + uint8_t ad_type, uint8_t offset, uint8_t length, const uint8_t *value) > > +{ > > + struct btd_adv_monitor_pattern *pattern; > > + > > + if (offset > BT_AD_MAX_DATA_LEN - 1) > > + return NULL; > > + > > + if ((ad_type > BT_AD_3D_INFO_DATA && > > + ad_type != BT_AD_MANUFACTURER_DATA) || > > + ad_type < BT_AD_FLAGS) { > > + return NULL; > > + } > > + > > + if (!value || !length || offset + length > BT_AD_MAX_DATA_LEN) > > + return NULL; > > + > > + pattern = new0(struct btd_adv_monitor_pattern, 1); > > + if (!pattern) > > + return NULL; > > + > > + pattern->ad_type = ad_type; > > + pattern->offset = offset; > > + pattern->length = length; > > + memcpy(pattern->value, value, pattern->length); > > I wonder why you didn't add pattern matching into bt_ad directly, that > should make it simpler to unit test such feature and enable using in > other tools as well. > Please see my reply above. > > + return pattern; > > +} > > + > > /* Retrieves Patterns from the remote Adv Monitor object, verifies the values > > * and update the local Adv Monitor > > */ > > @@ -464,7 +508,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path) > > int value_len; > > uint8_t *value; > > uint8_t offset, ad_type; > > - struct pattern *pattern; > > + struct btd_adv_monitor_pattern *pattern; > > DBusMessageIter struct_iter, value_iter; > > > > dbus_message_iter_recurse(&array_iter, &struct_iter); > > @@ -496,28 +540,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path) > > 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 = new0(struct pattern, 1); > > + pattern = pattern_create(ad_type, offset, value_len, value); > > if (!pattern) > > goto failed; > > > > - pattern->ad_type = ad_type; > > - pattern->offset = offset; > > - pattern->length = value_len; > > - memcpy(pattern->value, value, pattern->length); > > - > > queue_push_tail(monitor->patterns, pattern); > > > > dbus_message_iter_next(&array_iter); > > @@ -952,6 +978,149 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager) > > manager_destroy(manager); > > } > > > > +static bool pattern_match(const uint8_t *eir, uint8_t eir_len, > > + const struct btd_adv_monitor_pattern *pattern) > > +{ > > + const uint8_t *data; > > + uint8_t idx = 0; > > + uint8_t field_len, data_len, data_type; > > + > > + while (idx < eir_len - 1) { > > + field_len = eir[0]; > > + > > + /* Check for the end of EIR */ > > + if (field_len == 0) > > + break; > > + > > + idx += field_len + 1; > > + > > + /* Do not continue filtering if got incorrect length */ > > + if (idx > eir_len) > > + break; > > + > > + data = &eir[2]; > > + data_type = eir[1]; > > + data_len = field_len - 1; > > + > > + eir += field_len + 1; > > + > > + if (data_type != pattern->ad_type) > > + continue; > > + > > + if (data_len < pattern->offset + pattern->length) > > + continue; > > + > > + if (!memcmp(data + pattern->offset, pattern->value, > > + pattern->length)) > > + return true; > > + } > > Perhaps something like: > > struct bt_ad *bt_ad_new_with_data(uint8_t *data, uint8_t len); > > and > > bool bt_ad_has_pattern(struct bt_ad *ad, const struct bt_ad_pattern *pattern); > Please see my reply above. > > + return false; > > +} > > + > > +/* Processes the content matching based on a pattern */ > > +static void adv_match_per_pattern(void *data, void *user_data) > > +{ > > + struct btd_adv_monitor_pattern *pattern = data; > > + struct adv_content_filter_info *info = user_data; > > + > > + if (!pattern || info->matched) > > + return; > > + > > + info->matched = pattern_match(info->eir, info->eir_len, pattern); > > +} > > + > > +/* Processes the content matching based pattern(s) of a monitor */ > > +static void adv_match_per_monitor(void *data, void *user_data) > > +{ > > + struct adv_monitor *monitor = data; > > + struct adv_content_filter_info *info = user_data; > > + > > + if (!monitor && monitor->state != MONITOR_STATE_HONORED) > > + return; > > + > > + /* Reset the intermediate matched status */ > > + info->matched = false; > > + > > + if (monitor->type == MONITOR_TYPE_OR_PATTERNS) { > > + queue_foreach(monitor->patterns, adv_match_per_pattern, info); > > + if (info->matched) > > + goto matched; > > + } > > + > > + return; > > + > > +matched: > > + info->matched_monitors = g_slist_prepend(info->matched_monitors, > > + monitor); > > +} > > + > > +/* Processes the content matching for the monitor(s) of an app */ > > +static void adv_match_per_app(void *data, void *user_data) > > +{ > > + struct adv_monitor_app *app = data; > > + > > + if (!app) > > + return; > > + > > + queue_foreach(app->monitors, adv_match_per_monitor, user_data); > > +} > > + > > +/* Processes the content matching for every app without RSSI filtering and > > + * notifying monitors. The caller is responsible of releasing the memory of the > > + * list but not the data. > > + * Returns the list of monitors whose content match eir. > > + */ > > +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager, > > + const uint8_t *eir, uint8_t eir_len) > > +{ > > + struct adv_content_filter_info info; > > + > > + if (!manager || !eir || !eir_len) > > + return NULL; > > + > > + info.eir_len = eir_len; > > + info.eir = eir; > > + info.matched_monitors = NULL; > > + > > + queue_foreach(manager->apps, adv_match_per_app, &info); > > + > > + return info.matched_monitors; > > +} > > + > > +/* Wraps adv_monitor_filter_rssi() to processes the content-matched monitor with > > + * RSSI filtering and notifies it on device found/lost event > > + */ > > +static void monitor_filter_rssi(gpointer a, gpointer b) > > +{ > > + struct adv_monitor *monitor = a; > > + struct adv_rssi_filter_info *info = b; > > + > > + if (!monitor || !info) > > + return; > > + > > + adv_monitor_filter_rssi(monitor, info->device, info->rssi); > > +} > > + > > +/* Processes every content-matched monitor with RSSI filtering and notifies on > > + * device found/lost event. The caller is responsible of releasing the memory > > + * of matched_monitors list but not its data. > > + */ > > +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager, > > + struct btd_device *device, int8_t rssi, > > + GSList *matched_monitors) > > +{ > > + struct adv_rssi_filter_info info; > > + > > + if (!manager || !device || !matched_monitors) > > + return; > > + > > + info.device = device; > > + info.rssi = rssi; > > + > > + g_slist_foreach(matched_monitors, monitor_filter_rssi, &info); > > +} > > + > > /* Matches a device based on btd_device object */ > > static bool monitor_device_match(const void *a, const void *b) > > { > > diff --git a/src/adv_monitor.h b/src/adv_monitor.h > > index 13d5d7282..e2482e11e 100644 > > --- a/src/adv_monitor.h > > +++ b/src/adv_monitor.h > > @@ -11,16 +11,28 @@ > > #ifndef __ADV_MONITOR_H > > #define __ADV_MONITOR_H > > > > +#include <glib.h> > > + > > +#include "src/shared/ad.h" > > + > > struct mgmt; > > struct btd_device; > > struct btd_adapter; > > struct btd_adv_monitor_manager; > > +struct btd_adv_monitor_pattern; > > > > 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); > > > > +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager, > > + const uint8_t *eir, uint8_t eir_len); > > + > > +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager, > > + struct btd_device *device, int8_t rssi, > > + GSList *matched_monitors); > > + > > void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager, > > struct btd_device *device); > > > > -- > > 2.26.2 > > > > > -- > Luiz Augusto von Dentz Thanks, Miao
Hi Miao-chen, On Mon, Oct 12, 2020 at 2:21 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > Hi Luiz, > > I did think of adding pattern to bt_ad at the beginning, but here are > reasons why I ended up with hosting the definition of pattern in > adv_monitor. > (1) Pattern is specific to monitoring purpose. An advertisement > should not include patterns as its fields due to that fact that a > pattern hosts an offset. So I wasn't sure about its justification to > be placed in shared/ad. But if you foresee that it would be reused, > then I am more than happy to add it to shared/ad. > (2) Introducing helpers as you suggested below indeed make it more > unittestable. However, it also implied that EIR data (this is in fact > AD data) needs to be parsed into a new bt_ad for pattern comparison, > and I didn't see an obvious benefit of converting EIR data into a > bt_ad just for the comparison. Regarding (1) like I said you will need another struct to store the offset and length for doing the pattern matching but otherwise it is pretty similar, regarding (2) there is actually a real benefit that we could in the future drop the eir_ helpers and unit test just the bt_ad including pattern matching as well. > Maybe we can add a struct bt_ad_pattern in shared/ad.h and introduce > the following two functions. What do you think? > > struct bt_ad_pattern *bt_ad_pattern_new(uint8_t type, size_t offset, > size_t len, const void *data); Not sure I follow you here, that doesn't seem to be about the parsing of the AD but the creation of an object for matching, not sure we need that since you can probably just pass a stack variable directly, in any case you would need a free as well. > /* |data| is one single AD data field so that we can avoid converting > EIR data to bt_ad */ > bool bt_ad_pattern bt_ad_pattern_match(struct bt_ad_pattern *pattern, > void *data, size_t len); You may have more than one entry of the same type, anyway if we do have something like bt_ad_new_with_data that means we can pass it around and not have to reparse it when doing custom filtering. We might as well have a callback that does get called every time there is a match, also it may be possible to pass all the pattern at once in which case we can pass the pattern which matched along with the data thus having the call it multiple times for each monitor.
diff --git a/src/adapter.c b/src/adapter.c index 6d0114a6b..fdd9b3808 100644 --- a/src/adapter.c +++ b/src/adapter.c @@ -6601,6 +6601,15 @@ static void update_found_devices(struct btd_adapter *adapter, bool name_known, discoverable; char addr[18]; bool duplicate = false; + GSList *matched_monitors; + + /* During the background scanning, update the device only when the data + * match at least one Adv monitor + */ + matched_monitors = btd_adv_monitor_content_filter( + adapter->adv_monitor_manager, data, data_len); + if (!adapter->discovering && !matched_monitors) + return; memset(&eir_data, 0, sizeof(eir_data)); eir_parse(&eir_data, data, data_len); @@ -6646,18 +6655,22 @@ static void update_found_devices(struct btd_adapter *adapter, device_store_cached_name(dev, eir_data.name); /* - * Only skip devices that are not connected, are temporary and there - * is no active discovery session ongoing. + * Only skip devices that are not connected, are temporary, and there + * is no active discovery session ongoing and no matched Adv monitors */ - if (!btd_device_is_connected(dev) && (device_is_temporary(dev) && - !adapter->discovery_list)) { + if (!btd_device_is_connected(dev) && + (device_is_temporary(dev) && !adapter->discovery_list) && + !matched_monitors) { eir_data_free(&eir_data); return; } - /* Don't continue if not discoverable or if filter don't match */ - if (!discoverable || (adapter->filtered_discovery && - !is_filter_match(adapter->discovery_list, &eir_data, rssi))) { + /* If there is no matched Adv monitors, don't continue if not + * discoverable or if active discovery filter don't match. + */ + if (!matched_monitors && (!discoverable || + (adapter->filtered_discovery && !is_filter_match( + adapter->discovery_list, &eir_data, rssi)))) { eir_data_free(&eir_data); return; } @@ -6714,6 +6727,14 @@ static void update_found_devices(struct btd_adapter *adapter, eir_data_free(&eir_data); + /* After the device is updated, notify the matched Adv monitors */ + if (matched_monitors) { + btd_adv_monitor_notify_monitors(adapter->adv_monitor_manager, + dev, rssi, matched_monitors); + g_slist_free(matched_monitors); + matched_monitors = NULL; + } + /* * Only if at least one client has requested discovery, maintain * list of found devices and name confirming for legacy devices. diff --git a/src/adv_monitor.c b/src/adv_monitor.c index 31ed30a00..fcb127cd4 100644 --- a/src/adv_monitor.c +++ b/src/adv_monitor.c @@ -29,15 +29,12 @@ #include "device.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" -static void monitor_device_free(void *data); - #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1" #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1" @@ -84,7 +81,7 @@ enum monitor_state { MONITOR_STATE_HONORED, /* Accepted by kernel */ }; -struct pattern { +struct btd_adv_monitor_pattern { uint8_t ad_type; uint8_t offset; uint8_t length; @@ -133,6 +130,23 @@ struct app_match_data { const char *path; }; +struct adv_content_filter_info { + uint8_t eir_len; + const uint8_t *eir; + + bool matched; /* Intermediate state per monitor */ + GSList *matched_monitors; /* List of matched monitors */ +}; + +struct adv_rssi_filter_info { + struct btd_device *device; + int8_t rssi; +}; + +static void monitor_device_free(void *data); +static void adv_monitor_filter_rssi(struct adv_monitor *monitor, + struct btd_device *device, int8_t rssi); + const struct adv_monitor_type { enum monitor_type type; const char *name; @@ -155,7 +169,7 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply) /* Frees a pattern */ static void pattern_free(void *data) { - struct pattern *pattern = data; + struct btd_adv_monitor_pattern *pattern = data; if (!pattern) return; @@ -435,6 +449,36 @@ failed: return false; } +/* Allocates and initiates a pattern with the given content */ +static struct btd_adv_monitor_pattern *pattern_create( + uint8_t ad_type, uint8_t offset, uint8_t length, const uint8_t *value) +{ + struct btd_adv_monitor_pattern *pattern; + + if (offset > BT_AD_MAX_DATA_LEN - 1) + return NULL; + + if ((ad_type > BT_AD_3D_INFO_DATA && + ad_type != BT_AD_MANUFACTURER_DATA) || + ad_type < BT_AD_FLAGS) { + return NULL; + } + + if (!value || !length || offset + length > BT_AD_MAX_DATA_LEN) + return NULL; + + pattern = new0(struct btd_adv_monitor_pattern, 1); + if (!pattern) + return NULL; + + pattern->ad_type = ad_type; + pattern->offset = offset; + pattern->length = length; + memcpy(pattern->value, value, pattern->length); + + return pattern; +} + /* Retrieves Patterns from the remote Adv Monitor object, verifies the values * and update the local Adv Monitor */ @@ -464,7 +508,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path) int value_len; uint8_t *value; uint8_t offset, ad_type; - struct pattern *pattern; + struct btd_adv_monitor_pattern *pattern; DBusMessageIter struct_iter, value_iter; dbus_message_iter_recurse(&array_iter, &struct_iter); @@ -496,28 +540,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path) 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 = new0(struct pattern, 1); + pattern = pattern_create(ad_type, offset, value_len, value); if (!pattern) goto failed; - pattern->ad_type = ad_type; - pattern->offset = offset; - pattern->length = value_len; - memcpy(pattern->value, value, pattern->length); - queue_push_tail(monitor->patterns, pattern); dbus_message_iter_next(&array_iter); @@ -952,6 +978,149 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager) manager_destroy(manager); } +static bool pattern_match(const uint8_t *eir, uint8_t eir_len, + const struct btd_adv_monitor_pattern *pattern) +{ + const uint8_t *data; + uint8_t idx = 0; + uint8_t field_len, data_len, data_type; + + while (idx < eir_len - 1) { + field_len = eir[0]; + + /* Check for the end of EIR */ + if (field_len == 0) + break; + + idx += field_len + 1; + + /* Do not continue filtering if got incorrect length */ + if (idx > eir_len) + break; + + data = &eir[2]; + data_type = eir[1]; + data_len = field_len - 1; + + eir += field_len + 1; + + if (data_type != pattern->ad_type) + continue; + + if (data_len < pattern->offset + pattern->length) + continue; + + if (!memcmp(data + pattern->offset, pattern->value, + pattern->length)) + return true; + } + + return false; +} + +/* Processes the content matching based on a pattern */ +static void adv_match_per_pattern(void *data, void *user_data) +{ + struct btd_adv_monitor_pattern *pattern = data; + struct adv_content_filter_info *info = user_data; + + if (!pattern || info->matched) + return; + + info->matched = pattern_match(info->eir, info->eir_len, pattern); +} + +/* Processes the content matching based pattern(s) of a monitor */ +static void adv_match_per_monitor(void *data, void *user_data) +{ + struct adv_monitor *monitor = data; + struct adv_content_filter_info *info = user_data; + + if (!monitor && monitor->state != MONITOR_STATE_HONORED) + return; + + /* Reset the intermediate matched status */ + info->matched = false; + + if (monitor->type == MONITOR_TYPE_OR_PATTERNS) { + queue_foreach(monitor->patterns, adv_match_per_pattern, info); + if (info->matched) + goto matched; + } + + return; + +matched: + info->matched_monitors = g_slist_prepend(info->matched_monitors, + monitor); +} + +/* Processes the content matching for the monitor(s) of an app */ +static void adv_match_per_app(void *data, void *user_data) +{ + struct adv_monitor_app *app = data; + + if (!app) + return; + + queue_foreach(app->monitors, adv_match_per_monitor, user_data); +} + +/* Processes the content matching for every app without RSSI filtering and + * notifying monitors. The caller is responsible of releasing the memory of the + * list but not the data. + * Returns the list of monitors whose content match eir. + */ +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager, + const uint8_t *eir, uint8_t eir_len) +{ + struct adv_content_filter_info info; + + if (!manager || !eir || !eir_len) + return NULL; + + info.eir_len = eir_len; + info.eir = eir; + info.matched_monitors = NULL; + + queue_foreach(manager->apps, adv_match_per_app, &info); + + return info.matched_monitors; +} + +/* Wraps adv_monitor_filter_rssi() to processes the content-matched monitor with + * RSSI filtering and notifies it on device found/lost event + */ +static void monitor_filter_rssi(gpointer a, gpointer b) +{ + struct adv_monitor *monitor = a; + struct adv_rssi_filter_info *info = b; + + if (!monitor || !info) + return; + + adv_monitor_filter_rssi(monitor, info->device, info->rssi); +} + +/* Processes every content-matched monitor with RSSI filtering and notifies on + * device found/lost event. The caller is responsible of releasing the memory + * of matched_monitors list but not its data. + */ +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager, + struct btd_device *device, int8_t rssi, + GSList *matched_monitors) +{ + struct adv_rssi_filter_info info; + + if (!manager || !device || !matched_monitors) + return; + + info.device = device; + info.rssi = rssi; + + g_slist_foreach(matched_monitors, monitor_filter_rssi, &info); +} + /* Matches a device based on btd_device object */ static bool monitor_device_match(const void *a, const void *b) { diff --git a/src/adv_monitor.h b/src/adv_monitor.h index 13d5d7282..e2482e11e 100644 --- a/src/adv_monitor.h +++ b/src/adv_monitor.h @@ -11,16 +11,28 @@ #ifndef __ADV_MONITOR_H #define __ADV_MONITOR_H +#include <glib.h> + +#include "src/shared/ad.h" + struct mgmt; struct btd_device; struct btd_adapter; struct btd_adv_monitor_manager; +struct btd_adv_monitor_pattern; 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); +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager, + const uint8_t *eir, uint8_t eir_len); + +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager, + struct btd_device *device, int8_t rssi, + GSList *matched_monitors); + void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager, struct btd_device *device);