| Message ID | 20210304122005.Bluez.v2.1.I1411482bfff45aecdec1bc8c895fc7148ee3f50c@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: Fix scannable broadcast advertising on extended APIs | expand |
Hi Daniel, On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > This change moves the advertising data generation to the beginning of > the registration pipeline. This is necessary for the following patch, > which will need to know whether the scan response data is existent so > that the parameter request can be populated correctly. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > --- > > Changes in v2: None > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > 1 file changed, 42 insertions(+), 37 deletions(-) > > diff --git a/src/advertising.c b/src/advertising.c > index 15a343e52..f3dc357a1 100644 > --- a/src/advertising.c > +++ b/src/advertising.c > @@ -80,6 +80,10 @@ struct btd_adv_client { > uint32_t flags; > struct bt_ad *data; > struct bt_ad *scan; > + uint8_t *adv_data; > + uint8_t *scan_rsp; > + size_t adv_data_len; > + size_t scan_rsp_len; I'm debating if we should really just encode it early or we could just introduce something like bt_ad_length so we don't have to have copies of the same data in 2 different formats since bt_ad already represents that. > uint8_t instance; > uint32_t min_interval; > uint32_t max_interval; > @@ -141,6 +145,16 @@ static void client_free(void *data) > bt_ad_unref(client->data); > bt_ad_unref(client->scan); > > + if (client->adv_data) { > + free(client->adv_data); > + client->adv_data = NULL; > + } > + > + if (client->scan_rsp) { > + free(client->scan_rsp); > + client->scan_rsp = NULL; > + } > + > g_dbus_proxy_unref(client->proxy); > > if (client->owner) > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > flags |= MGMT_ADV_PARAM_TX_POWER; > } > > + client->adv_data = generate_adv_data(client, &flags, > + &client->adv_data_len); > + if (!client->adv_data || > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > + error("Advertising data too long or couldn't be generated."); > + return -EINVAL; > + } > + > + client->scan_rsp = generate_scan_rsp(client, &flags, > + &client->scan_rsp_len); > + if (!client->scan_rsp && client->scan_rsp_len) { > + error("Scan data couldn't be generated."); > + free(client->adv_data); > + return -EINVAL; > + } > + > cp.flags = htobl(flags); > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > const struct mgmt_rp_add_ext_adv_params *rp = param; > struct mgmt_cp_add_ext_adv_data *cp = NULL; > uint8_t param_len; > - uint8_t *adv_data = NULL; > - size_t adv_data_len; > - uint8_t *scan_rsp = NULL; > - size_t scan_rsp_len = -1; > - uint32_t flags = 0; > unsigned int mgmt_ret; > dbus_int16_t tx_power; > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > client->instance = rp->instance; > > - flags = get_adv_flags(client); > - > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > - error("Advertising data too long or couldn't be generated."); > - goto fail; > - } > - > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > - if ((!scan_rsp && scan_rsp_len) || > - scan_rsp_len > rp->max_scan_rsp_len) { > - error("Scan data couldn't be generated."); > - goto fail; > - } > - > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > - scan_rsp_len; > + param_len = sizeof(struct mgmt_cp_add_advertising) + > + client->adv_data_len + client->scan_rsp_len; > > cp = malloc0(param_len); > if (!cp) { > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > } > > cp->instance = client->instance; > - cp->adv_data_len = adv_data_len; > - cp->scan_rsp_len = scan_rsp_len; > - memcpy(cp->data, adv_data, adv_data_len); > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > - > - free(adv_data); > - free(scan_rsp); > - adv_data = NULL; > - scan_rsp = NULL; > + cp->adv_data_len = client->adv_data_len; > + cp->scan_rsp_len = client->scan_rsp_len; > + memcpy(cp->data, client->adv_data, client->adv_data_len); > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > + client->scan_rsp_len); > > /* Submit request to update instance data */ > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > return; > > fail: > - if (adv_data) > - free(adv_data); > - > - if (scan_rsp) > - free(scan_rsp); > - > if (cp) > free(cp); > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > if (!client->scan) > goto fail; > > + client->adv_data = NULL; > + client->scan_rsp = NULL; > + client->adv_data_len = 0; > + client->scan_rsp_len = 0; > + > client->manager = manager; > client->appearance = UINT16_MAX; > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > -- > 2.30.1.766.gb4fecdf3b7-goog >
Hi Luiz, Can you please clarify your suggestion? The issue here is that some properties (local name, for instance) aren't incorporated into the bt_ad object until generate_scan_rsp is called. I decided to move the generation of data/scan response to be earlier because otherwise the logic to determine if the scan response was empty would require some repetitive logic that already exists in generate_scan_rsp. Or are you suggesting that we not store adv_data_len and scan_rsp_len in the btd_adv_client, but instead store it in the bt_ad object? This seems possible to me, but it might require a bit more effort to keep the length property in sync. I'll wait for your clarification. Thanks! Daniel On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > This change moves the advertising data generation to the beginning of > > the registration pipeline. This is necessary for the following patch, > > which will need to know whether the scan response data is existent so > > that the parameter request can be populated correctly. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > > > --- > > > > Changes in v2: None > > > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > diff --git a/src/advertising.c b/src/advertising.c > > index 15a343e52..f3dc357a1 100644 > > --- a/src/advertising.c > > +++ b/src/advertising.c > > @@ -80,6 +80,10 @@ struct btd_adv_client { > > uint32_t flags; > > struct bt_ad *data; > > struct bt_ad *scan; > > + uint8_t *adv_data; > > + uint8_t *scan_rsp; > > + size_t adv_data_len; > > + size_t scan_rsp_len; > > I'm debating if we should really just encode it early or we could just > introduce something like bt_ad_length so we don't have to have copies > of the same data in 2 different formats since bt_ad already represents > that. > > > uint8_t instance; > > uint32_t min_interval; > > uint32_t max_interval; > > @@ -141,6 +145,16 @@ static void client_free(void *data) > > bt_ad_unref(client->data); > > bt_ad_unref(client->scan); > > > > + if (client->adv_data) { > > + free(client->adv_data); > > + client->adv_data = NULL; > > + } > > + > > + if (client->scan_rsp) { > > + free(client->scan_rsp); > > + client->scan_rsp = NULL; > > + } > > + > > g_dbus_proxy_unref(client->proxy); > > > > if (client->owner) > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > > flags |= MGMT_ADV_PARAM_TX_POWER; > > } > > > > + client->adv_data = generate_adv_data(client, &flags, > > + &client->adv_data_len); > > + if (!client->adv_data || > > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > > + error("Advertising data too long or couldn't be generated."); > > + return -EINVAL; > > + } > > + > > + client->scan_rsp = generate_scan_rsp(client, &flags, > > + &client->scan_rsp_len); > > + if (!client->scan_rsp && client->scan_rsp_len) { > > + error("Scan data couldn't be generated."); > > + free(client->adv_data); > > + return -EINVAL; > > + } > > + > > cp.flags = htobl(flags); > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > const struct mgmt_rp_add_ext_adv_params *rp = param; > > struct mgmt_cp_add_ext_adv_data *cp = NULL; > > uint8_t param_len; > > - uint8_t *adv_data = NULL; > > - size_t adv_data_len; > > - uint8_t *scan_rsp = NULL; > > - size_t scan_rsp_len = -1; > > - uint32_t flags = 0; > > unsigned int mgmt_ret; > > dbus_int16_t tx_power; > > > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > client->instance = rp->instance; > > > > - flags = get_adv_flags(client); > > - > > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > > - error("Advertising data too long or couldn't be generated."); > > - goto fail; > > - } > > - > > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > > - if ((!scan_rsp && scan_rsp_len) || > > - scan_rsp_len > rp->max_scan_rsp_len) { > > - error("Scan data couldn't be generated."); > > - goto fail; > > - } > > - > > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > > - scan_rsp_len; > > + param_len = sizeof(struct mgmt_cp_add_advertising) + > > + client->adv_data_len + client->scan_rsp_len; > > > > cp = malloc0(param_len); > > if (!cp) { > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > } > > > > cp->instance = client->instance; > > - cp->adv_data_len = adv_data_len; > > - cp->scan_rsp_len = scan_rsp_len; > > - memcpy(cp->data, adv_data, adv_data_len); > > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > > - > > - free(adv_data); > > - free(scan_rsp); > > - adv_data = NULL; > > - scan_rsp = NULL; > > + cp->adv_data_len = client->adv_data_len; > > + cp->scan_rsp_len = client->scan_rsp_len; > > + memcpy(cp->data, client->adv_data, client->adv_data_len); > > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > > + client->scan_rsp_len); > > > > /* Submit request to update instance data */ > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > return; > > > > fail: > > - if (adv_data) > > - free(adv_data); > > - > > - if (scan_rsp) > > - free(scan_rsp); > > - > > if (cp) > > free(cp); > > > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > > if (!client->scan) > > goto fail; > > > > + client->adv_data = NULL; > > + client->scan_rsp = NULL; > > + client->adv_data_len = 0; > > + client->scan_rsp_len = 0; > > + > > client->manager = manager; > > client->appearance = UINT16_MAX; > > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > > -- > > 2.30.1.766.gb4fecdf3b7-goog > > > > > -- > Luiz Augusto von Dentz
Hi Daniel, On Thu, Mar 4, 2021 at 3:27 PM Daniel Winkler <danielwinkler@google.com> wrote: > > Hi Luiz, > > Can you please clarify your suggestion? The issue here is that some > properties (local name, for instance) aren't incorporated into the > bt_ad object until generate_scan_rsp is called. I decided to move the > generation of data/scan response to be earlier because otherwise the > logic to determine if the scan response was empty would require some > repetitive logic that already exists in generate_scan_rsp. > > Or are you suggesting that we not store adv_data_len and scan_rsp_len > in the btd_adv_client, but instead store it in the bt_ad object? This > seems possible to me, but it might require a bit more effort to keep > the length property in sync. I'll wait for your clarification. Yep, the included flags may have to be calculated separately, but I thought the whole point here is to be able to tell if there is any scan_rsp to be added so perhaps just return a bool would be enough to then set the flag so we can continue to generate the data itself at later stage. > Thanks! > Daniel > > On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Daniel, > > > > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > This change moves the advertising data generation to the beginning of > > > the registration pipeline. This is necessary for the following patch, > > > which will need to know whether the scan response data is existent so > > > that the parameter request can be populated correctly. > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > > > > > --- > > > > > > Changes in v2: None > > > > > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > > > diff --git a/src/advertising.c b/src/advertising.c > > > index 15a343e52..f3dc357a1 100644 > > > --- a/src/advertising.c > > > +++ b/src/advertising.c > > > @@ -80,6 +80,10 @@ struct btd_adv_client { > > > uint32_t flags; > > > struct bt_ad *data; > > > struct bt_ad *scan; > > > + uint8_t *adv_data; > > > + uint8_t *scan_rsp; > > > + size_t adv_data_len; > > > + size_t scan_rsp_len; > > > > I'm debating if we should really just encode it early or we could just > > introduce something like bt_ad_length so we don't have to have copies > > of the same data in 2 different formats since bt_ad already represents > > that. > > > > > uint8_t instance; > > > uint32_t min_interval; > > > uint32_t max_interval; > > > @@ -141,6 +145,16 @@ static void client_free(void *data) > > > bt_ad_unref(client->data); > > > bt_ad_unref(client->scan); > > > > > > + if (client->adv_data) { > > > + free(client->adv_data); > > > + client->adv_data = NULL; > > > + } > > > + > > > + if (client->scan_rsp) { > > > + free(client->scan_rsp); > > > + client->scan_rsp = NULL; > > > + } > > > + > > > g_dbus_proxy_unref(client->proxy); > > > > > > if (client->owner) > > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > > > flags |= MGMT_ADV_PARAM_TX_POWER; > > > } > > > > > > + client->adv_data = generate_adv_data(client, &flags, > > > + &client->adv_data_len); > > > + if (!client->adv_data || > > > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > > > + error("Advertising data too long or couldn't be generated."); > > > + return -EINVAL; > > > + } > > > + > > > + client->scan_rsp = generate_scan_rsp(client, &flags, > > > + &client->scan_rsp_len); > > > + if (!client->scan_rsp && client->scan_rsp_len) { > > > + error("Scan data couldn't be generated."); > > > + free(client->adv_data); > > > + return -EINVAL; > > > + } > > > + > > > cp.flags = htobl(flags); > > > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > const struct mgmt_rp_add_ext_adv_params *rp = param; > > > struct mgmt_cp_add_ext_adv_data *cp = NULL; > > > uint8_t param_len; > > > - uint8_t *adv_data = NULL; > > > - size_t adv_data_len; > > > - uint8_t *scan_rsp = NULL; > > > - size_t scan_rsp_len = -1; > > > - uint32_t flags = 0; > > > unsigned int mgmt_ret; > > > dbus_int16_t tx_power; > > > > > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > > > client->instance = rp->instance; > > > > > > - flags = get_adv_flags(client); > > > - > > > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > > > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > > > - error("Advertising data too long or couldn't be generated."); > > > - goto fail; > > > - } > > > - > > > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > > > - if ((!scan_rsp && scan_rsp_len) || > > > - scan_rsp_len > rp->max_scan_rsp_len) { > > > - error("Scan data couldn't be generated."); > > > - goto fail; > > > - } > > > - > > > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > > > - scan_rsp_len; > > > + param_len = sizeof(struct mgmt_cp_add_advertising) + > > > + client->adv_data_len + client->scan_rsp_len; > > > > > > cp = malloc0(param_len); > > > if (!cp) { > > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > } > > > > > > cp->instance = client->instance; > > > - cp->adv_data_len = adv_data_len; > > > - cp->scan_rsp_len = scan_rsp_len; > > > - memcpy(cp->data, adv_data, adv_data_len); > > > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > > > - > > > - free(adv_data); > > > - free(scan_rsp); > > > - adv_data = NULL; > > > - scan_rsp = NULL; > > > + cp->adv_data_len = client->adv_data_len; > > > + cp->scan_rsp_len = client->scan_rsp_len; > > > + memcpy(cp->data, client->adv_data, client->adv_data_len); > > > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > > > + client->scan_rsp_len); > > > > > > /* Submit request to update instance data */ > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > return; > > > > > > fail: > > > - if (adv_data) > > > - free(adv_data); > > > - > > > - if (scan_rsp) > > > - free(scan_rsp); > > > - > > > if (cp) > > > free(cp); > > > > > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > > > if (!client->scan) > > > goto fail; > > > > > > + client->adv_data = NULL; > > > + client->scan_rsp = NULL; > > > + client->adv_data_len = 0; > > > + client->scan_rsp_len = 0; > > > + > > > client->manager = manager; > > > client->appearance = UINT16_MAX; > > > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > > > -- > > > 2.30.1.766.gb4fecdf3b7-goog > > > > > > > > > -- > > Luiz Augusto von Dentz
Hi Luiz, Sorry for the delay. I think I understand your reservations now, and have just uploaded a V3 that uses helpers to determine if the instance will be scannable, rather than storing a duplicated copy of adv data and scan response. Please take a look at your convenience. Best, Daniel On Thu, Mar 4, 2021 at 3:43 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Thu, Mar 4, 2021 at 3:27 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > Hi Luiz, > > > > Can you please clarify your suggestion? The issue here is that some > > properties (local name, for instance) aren't incorporated into the > > bt_ad object until generate_scan_rsp is called. I decided to move the > > generation of data/scan response to be earlier because otherwise the > > logic to determine if the scan response was empty would require some > > repetitive logic that already exists in generate_scan_rsp. > > > > Or are you suggesting that we not store adv_data_len and scan_rsp_len > > in the btd_adv_client, but instead store it in the bt_ad object? This > > seems possible to me, but it might require a bit more effort to keep > > the length property in sync. I'll wait for your clarification. > > Yep, the included flags may have to be calculated separately, but I > thought the whole point here is to be able to tell if there is any > scan_rsp to be added so perhaps just return a bool would be enough to > then set the flag so we can continue to generate the data itself at > later stage. > > > Thanks! > > Daniel > > > > On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Daniel, > > > > > > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > > > This change moves the advertising data generation to the beginning of > > > > the registration pipeline. This is necessary for the following patch, > > > > which will need to know whether the scan response data is existent so > > > > that the parameter request can be populated correctly. > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > > > > > > > --- > > > > > > > > Changes in v2: None > > > > > > > > src/advertising.c | 79 +++++++++++++++++++++++++---------------------- > > > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > > > > > diff --git a/src/advertising.c b/src/advertising.c > > > > index 15a343e52..f3dc357a1 100644 > > > > --- a/src/advertising.c > > > > +++ b/src/advertising.c > > > > @@ -80,6 +80,10 @@ struct btd_adv_client { > > > > uint32_t flags; > > > > struct bt_ad *data; > > > > struct bt_ad *scan; > > > > + uint8_t *adv_data; > > > > + uint8_t *scan_rsp; > > > > + size_t adv_data_len; > > > > + size_t scan_rsp_len; > > > > > > I'm debating if we should really just encode it early or we could just > > > introduce something like bt_ad_length so we don't have to have copies > > > of the same data in 2 different formats since bt_ad already represents > > > that. > > > > > > > uint8_t instance; > > > > uint32_t min_interval; > > > > uint32_t max_interval; > > > > @@ -141,6 +145,16 @@ static void client_free(void *data) > > > > bt_ad_unref(client->data); > > > > bt_ad_unref(client->scan); > > > > > > > > + if (client->adv_data) { > > > > + free(client->adv_data); > > > > + client->adv_data = NULL; > > > > + } > > > > + > > > > + if (client->scan_rsp) { > > > > + free(client->scan_rsp); > > > > + client->scan_rsp = NULL; > > > > + } > > > > + > > > > g_dbus_proxy_unref(client->proxy); > > > > > > > > if (client->owner) > > > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, > > > > flags |= MGMT_ADV_PARAM_TX_POWER; > > > > } > > > > > > > > + client->adv_data = generate_adv_data(client, &flags, > > > > + &client->adv_data_len); > > > > + if (!client->adv_data || > > > > + (client->adv_data_len > calc_max_adv_len(client, flags))) { > > > > + error("Advertising data too long or couldn't be generated."); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + client->scan_rsp = generate_scan_rsp(client, &flags, > > > > + &client->scan_rsp_len); > > > > + if (!client->scan_rsp && client->scan_rsp_len) { > > > > + error("Scan data couldn't be generated."); > > > > + free(client->adv_data); > > > > + return -EINVAL; > > > > + } > > > > + > > > > cp.flags = htobl(flags); > > > > > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, > > > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > const struct mgmt_rp_add_ext_adv_params *rp = param; > > > > struct mgmt_cp_add_ext_adv_data *cp = NULL; > > > > uint8_t param_len; > > > > - uint8_t *adv_data = NULL; > > > > - size_t adv_data_len; > > > > - uint8_t *scan_rsp = NULL; > > > > - size_t scan_rsp_len = -1; > > > > - uint32_t flags = 0; > > > > unsigned int mgmt_ret; > > > > dbus_int16_t tx_power; > > > > > > > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > > > > > client->instance = rp->instance; > > > > > > > > - flags = get_adv_flags(client); > > > > - > > > > - adv_data = generate_adv_data(client, &flags, &adv_data_len); > > > > - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { > > > > - error("Advertising data too long or couldn't be generated."); > > > > - goto fail; > > > > - } > > > > - > > > > - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); > > > > - if ((!scan_rsp && scan_rsp_len) || > > > > - scan_rsp_len > rp->max_scan_rsp_len) { > > > > - error("Scan data couldn't be generated."); > > > > - goto fail; > > > > - } > > > > - > > > > - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + > > > > - scan_rsp_len; > > > > + param_len = sizeof(struct mgmt_cp_add_advertising) + > > > > + client->adv_data_len + client->scan_rsp_len; > > > > > > > > cp = malloc0(param_len); > > > > if (!cp) { > > > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > } > > > > > > > > cp->instance = client->instance; > > > > - cp->adv_data_len = adv_data_len; > > > > - cp->scan_rsp_len = scan_rsp_len; > > > > - memcpy(cp->data, adv_data, adv_data_len); > > > > - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); > > > > - > > > > - free(adv_data); > > > > - free(scan_rsp); > > > > - adv_data = NULL; > > > > - scan_rsp = NULL; > > > > + cp->adv_data_len = client->adv_data_len; > > > > + cp->scan_rsp_len = client->scan_rsp_len; > > > > + memcpy(cp->data, client->adv_data, client->adv_data_len); > > > > + memcpy(cp->data + client->adv_data_len, client->scan_rsp, > > > > + client->scan_rsp_len); > > > > > > > > /* Submit request to update instance data */ > > > > mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, > > > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, > > > > return; > > > > > > > > fail: > > > > - if (adv_data) > > > > - free(adv_data); > > > > - > > > > - if (scan_rsp) > > > > - free(scan_rsp); > > > > - > > > > if (cp) > > > > free(cp); > > > > > > > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > > > > if (!client->scan) > > > > goto fail; > > > > > > > > + client->adv_data = NULL; > > > > + client->scan_rsp = NULL; > > > > + client->adv_data_len = 0; > > > > + client->scan_rsp_len = 0; > > > > + > > > > client->manager = manager; > > > > client->appearance = UINT16_MAX; > > > > client->tx_power = ADV_TX_POWER_NO_PREFERENCE; > > > > -- > > > > 2.30.1.766.gb4fecdf3b7-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz
diff --git a/src/advertising.c b/src/advertising.c index 15a343e52..f3dc357a1 100644 --- a/src/advertising.c +++ b/src/advertising.c @@ -80,6 +80,10 @@ struct btd_adv_client { uint32_t flags; struct bt_ad *data; struct bt_ad *scan; + uint8_t *adv_data; + uint8_t *scan_rsp; + size_t adv_data_len; + size_t scan_rsp_len; uint8_t instance; uint32_t min_interval; uint32_t max_interval; @@ -141,6 +145,16 @@ static void client_free(void *data) bt_ad_unref(client->data); bt_ad_unref(client->scan); + if (client->adv_data) { + free(client->adv_data); + client->adv_data = NULL; + } + + if (client->scan_rsp) { + free(client->scan_rsp); + client->scan_rsp = NULL; + } + g_dbus_proxy_unref(client->proxy); if (client->owner) @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client, flags |= MGMT_ADV_PARAM_TX_POWER; } + client->adv_data = generate_adv_data(client, &flags, + &client->adv_data_len); + if (!client->adv_data || + (client->adv_data_len > calc_max_adv_len(client, flags))) { + error("Advertising data too long or couldn't be generated."); + return -EINVAL; + } + + client->scan_rsp = generate_scan_rsp(client, &flags, + &client->scan_rsp_len); + if (!client->scan_rsp && client->scan_rsp_len) { + error("Scan data couldn't be generated."); + free(client->adv_data); + return -EINVAL; + } + cp.flags = htobl(flags); mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS, @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, const struct mgmt_rp_add_ext_adv_params *rp = param; struct mgmt_cp_add_ext_adv_data *cp = NULL; uint8_t param_len; - uint8_t *adv_data = NULL; - size_t adv_data_len; - uint8_t *scan_rsp = NULL; - size_t scan_rsp_len = -1; - uint32_t flags = 0; unsigned int mgmt_ret; dbus_int16_t tx_power; @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, client->instance = rp->instance; - flags = get_adv_flags(client); - - adv_data = generate_adv_data(client, &flags, &adv_data_len); - if (!adv_data || (adv_data_len > rp->max_adv_data_len)) { - error("Advertising data too long or couldn't be generated."); - goto fail; - } - - scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len); - if ((!scan_rsp && scan_rsp_len) || - scan_rsp_len > rp->max_scan_rsp_len) { - error("Scan data couldn't be generated."); - goto fail; - } - - param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len + - scan_rsp_len; + param_len = sizeof(struct mgmt_cp_add_advertising) + + client->adv_data_len + client->scan_rsp_len; cp = malloc0(param_len); if (!cp) { @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, } cp->instance = client->instance; - cp->adv_data_len = adv_data_len; - cp->scan_rsp_len = scan_rsp_len; - memcpy(cp->data, adv_data, adv_data_len); - memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len); - - free(adv_data); - free(scan_rsp); - adv_data = NULL; - scan_rsp = NULL; + cp->adv_data_len = client->adv_data_len; + cp->scan_rsp_len = client->scan_rsp_len; + memcpy(cp->data, client->adv_data, client->adv_data_len); + memcpy(cp->data + client->adv_data_len, client->scan_rsp, + client->scan_rsp_len); /* Submit request to update instance data */ mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA, @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length, return; fail: - if (adv_data) - free(adv_data); - - if (scan_rsp) - free(scan_rsp); - if (cp) free(cp); @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, if (!client->scan) goto fail; + client->adv_data = NULL; + client->scan_rsp = NULL; + client->adv_data_len = 0; + client->scan_rsp_len = 0; + client->manager = manager; client->appearance = UINT16_MAX; client->tx_power = ADV_TX_POWER_NO_PREFERENCE;