From: Luiz Augusto von Dentz <[email protected]>
If either manufacturer or service data already exist but the content
is different replace with the new data.
---
src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/shared/ad.c b/src/shared/ad.c
index 00d138d..dfc6b9a 100644
--- a/src/shared/ad.c
+++ b/src/shared/ad.c
@@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
queue_remove_all(ad->service_uuids, NULL, NULL, free);
}
+static bool manufacturer_id_data_match(const void *data, const void *user_data)
+{
+ const struct bt_ad_manufacturer_data *m = data;
+ uint16_t id = PTR_TO_UINT(user_data);
+
+ return m->manufacturer_id == id;
+}
+
bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
void *data, size_t len)
{
@@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
return false;
+ new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
+ UINT_TO_PTR(manufacturer_id));
+ if (new_data) {
+ if (new_data->len == len && !memcmp(new_data->data, data, len))
+ return false;
+ new_data->data = realloc(new_data->data, len);
+ memcpy(new_data->data, data, len);
+ new_data->len = len;
+ return true;
+ }
+
new_data = new0(struct bt_ad_manufacturer_data, 1);
if (!new_data)
return false;
@@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
new_data->len = len;
- if (bt_ad_has_manufacturer_data(ad, new_data)) {
- manuf_destroy(new_data);
- return false;
- }
-
if (queue_push_tail(ad->manufacturer_data, new_data))
return true;
@@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
}
+
+static bool service_uuid_match(const void *data, const void *user_data)
+{
+ const struct bt_ad_service_data *s = data;
+ const bt_uuid_t *uuid = user_data;
+
+ return bt_uuid_cmp(&s->uuid, uuid);
+}
+
bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
size_t len)
{
@@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
return false;
+ new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
+ if (new_data) {
+ if (new_data->len == len && !memcmp(new_data->data, data, len))
+ return false;
+ new_data->data = realloc(new_data->data, len);
+ memcpy(new_data->data, data, len);
+ new_data->len = len;
+ return true;
+ }
+
new_data = new0(struct bt_ad_service_data, 1);
if (!new_data)
return false;
--
2.1.0
Hi,
On Tue, May 19, 2015 at 8:41 PM, Marie Janssen <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, May 19, 2015 at 10:35 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Marie,
>>
>> On Tue, May 19, 2015 at 8:16 PM, Marie Janssen <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> On Mon, May 18, 2015 at 4:14 AM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> Hi Marie,
>>>>
>>>> On Fri, May 15, 2015 at 7:04 PM, Marie Janssen <[email protected]> wrote:
>>>>> On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <[email protected]> wrote:
>>>>>> Hi Luiz,
>>>>>>
>>>>>> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
>>>>>> <[email protected]> wrote:
>>>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>>>
>>>>>>> If either manufacturer or service data already exist but the content
>>>>>>> is different replace with the new data.
>>>>>>> ---
>>>>>>> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>>>>>> 1 file changed, 38 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/shared/ad.c b/src/shared/ad.c
>>>>>>> index 00d138d..dfc6b9a 100644
>>>>>>> --- a/src/shared/ad.c
>>>>>>> +++ b/src/shared/ad.c
>>>>>>> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
>>>>>>> queue_remove_all(ad->service_uuids, NULL, NULL, free);
>>>>>>> }
>>>>>>>
>>>>>>> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
>>>>>>> +{
>>>>>>> + const struct bt_ad_manufacturer_data *m = data;
>>>>>>> + uint16_t id = PTR_TO_UINT(user_data);
>>>>>>> +
>>>>>>> + return m->manufacturer_id == id;
>>>>>>> +}
>>>>>>> +
>>>>>>> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>>> void *data, size_t len)
>>>>>>> {
>>>>>>> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>>> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
>>>>>>> return false;
>>>>>>>
>>>>>>> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
>>>>>>> + UINT_TO_PTR(manufacturer_id));
>>>>>>> + if (new_data) {
>>>>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>>>>> + return false;
>>>>>>
>>>>>> I don't think we want this check. If the data is different but length
>>>>>> the same, we still want to update the data.
>>>>>
>>>>> Sorry I read this wrong. We would want to return true here because
>>>>> the data in the ad is as expected.
>>>>
>>>> This is to prevent emitting duplicated signals since in fact it did
>>>> not change anything.
>>>>
>>>
>>> Previously the false return value meant that some error had occurred,
>>> such as memory allocation failure or the data being too long for the
>>> ad. Having nothing change and the ad be basically intact seems like
>>> not a problem. I guess this is a API semantics question, whether a
>>> true return value means "the data is set correctly" vs "the data is
>>> updated".
>>
>> While this is true there is also the convenience side of things, lets
>> say we return true then I will need to check if the same data exists
>> before calling this function because otherwise I can't tell if
>> anything has changed and that basically means we have 2 checks instead
>> of one. Beside the action here is to some data, if we are not adding
>> or replacing anything Id say returning false is fine, btw if you look
>> at the blocks Im removing that is already the behavior we have.
>>
>
> All right, this makes sense then I don't have any problems with it otherwise.
>
>>>>>>> + new_data->data = realloc(new_data->data, len);
>>>>>>> + memcpy(new_data->data, data, len);
>>>>>>> + new_data->len = len;
>>>>>>> + return true;
>>>>>>> + }
>>>>>>> +
>>>>>>> new_data = new0(struct bt_ad_manufacturer_data, 1);
>>>>>>> if (!new_data)
>>>>>>> return false;
>>>>>>> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>>>
>>>>>>> new_data->len = len;
>>>>>>>
>>>>>>> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
>>>>>>> - manuf_destroy(new_data);
>>>>>>> - return false;
>>>>>>> - }
>>>>>>> -
>>>>>>> if (queue_push_tail(ad->manufacturer_data, new_data))
>>>>>>> return true;
>>>>>>>
>>>>>>> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
>>>>>>> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
>>>>>>> }
>>>>>>>
>>>>>>> +
>>>>>>> +static bool service_uuid_match(const void *data, const void *user_data)
>>>>>>> +{
>>>>>>> + const struct bt_ad_service_data *s = data;
>>>>>>> + const bt_uuid_t *uuid = user_data;
>>>>>>> +
>>>>>>> + return bt_uuid_cmp(&s->uuid, uuid);
>>>>>>> +}
>>>>>>> +
>>>>>>> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>>>>> size_t len)
>>>>>>> {
>>>>>>> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>>>>> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
>>>>>>> return false;
>>>>>>>
>>>>>>> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
>>>>>>> + if (new_data) {
>>>>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>>>>> + return false;
>>>>>>
>>>>>> Similarly here as from above, it would be better to replace the data.
>>>>>>
>>>>>>> + new_data->data = realloc(new_data->data, len);
>>>>>>> + memcpy(new_data->data, data, len);
>>>>>>> + new_data->len = len;
>>>>>>> + return true;
>>>>>>> + }
>>>>>>> +
>>>>>>> new_data = new0(struct bt_ad_service_data, 1);
>>>>>>> if (!new_data)
>>>>>>> return false;
>>>>>>> --
>>>>>>> 2.1.0
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>>>> the body of a message to [email protected]
>>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Marie Janssen
>>>>
>>>>
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>>
>>>
>>>
>>> --
>>> Marie Janssen
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> Marie Janssen
Applied
--
Luiz Augusto von Dentz
Hi Luiz,
On Tue, May 19, 2015 at 10:35 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marie,
>
> On Tue, May 19, 2015 at 8:16 PM, Marie Janssen <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Mon, May 18, 2015 at 4:14 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Marie,
>>>
>>> On Fri, May 15, 2015 at 7:04 PM, Marie Janssen <[email protected]> wrote:
>>>> On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <[email protected]> wrote:
>>>>> Hi Luiz,
>>>>>
>>>>> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
>>>>> <[email protected]> wrote:
>>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>>
>>>>>> If either manufacturer or service data already exist but the content
>>>>>> is different replace with the new data.
>>>>>> ---
>>>>>> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>>>>> 1 file changed, 38 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/src/shared/ad.c b/src/shared/ad.c
>>>>>> index 00d138d..dfc6b9a 100644
>>>>>> --- a/src/shared/ad.c
>>>>>> +++ b/src/shared/ad.c
>>>>>> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
>>>>>> queue_remove_all(ad->service_uuids, NULL, NULL, free);
>>>>>> }
>>>>>>
>>>>>> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
>>>>>> +{
>>>>>> + const struct bt_ad_manufacturer_data *m = data;
>>>>>> + uint16_t id = PTR_TO_UINT(user_data);
>>>>>> +
>>>>>> + return m->manufacturer_id == id;
>>>>>> +}
>>>>>> +
>>>>>> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>> void *data, size_t len)
>>>>>> {
>>>>>> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
>>>>>> return false;
>>>>>>
>>>>>> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
>>>>>> + UINT_TO_PTR(manufacturer_id));
>>>>>> + if (new_data) {
>>>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>>>> + return false;
>>>>>
>>>>> I don't think we want this check. If the data is different but length
>>>>> the same, we still want to update the data.
>>>>
>>>> Sorry I read this wrong. We would want to return true here because
>>>> the data in the ad is as expected.
>>>
>>> This is to prevent emitting duplicated signals since in fact it did
>>> not change anything.
>>>
>>
>> Previously the false return value meant that some error had occurred,
>> such as memory allocation failure or the data being too long for the
>> ad. Having nothing change and the ad be basically intact seems like
>> not a problem. I guess this is a API semantics question, whether a
>> true return value means "the data is set correctly" vs "the data is
>> updated".
>
> While this is true there is also the convenience side of things, lets
> say we return true then I will need to check if the same data exists
> before calling this function because otherwise I can't tell if
> anything has changed and that basically means we have 2 checks instead
> of one. Beside the action here is to some data, if we are not adding
> or replacing anything Id say returning false is fine, btw if you look
> at the blocks Im removing that is already the behavior we have.
>
All right, this makes sense then I don't have any problems with it otherwise.
>>>>>> + new_data->data = realloc(new_data->data, len);
>>>>>> + memcpy(new_data->data, data, len);
>>>>>> + new_data->len = len;
>>>>>> + return true;
>>>>>> + }
>>>>>> +
>>>>>> new_data = new0(struct bt_ad_manufacturer_data, 1);
>>>>>> if (!new_data)
>>>>>> return false;
>>>>>> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>>
>>>>>> new_data->len = len;
>>>>>>
>>>>>> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
>>>>>> - manuf_destroy(new_data);
>>>>>> - return false;
>>>>>> - }
>>>>>> -
>>>>>> if (queue_push_tail(ad->manufacturer_data, new_data))
>>>>>> return true;
>>>>>>
>>>>>> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
>>>>>> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
>>>>>> }
>>>>>>
>>>>>> +
>>>>>> +static bool service_uuid_match(const void *data, const void *user_data)
>>>>>> +{
>>>>>> + const struct bt_ad_service_data *s = data;
>>>>>> + const bt_uuid_t *uuid = user_data;
>>>>>> +
>>>>>> + return bt_uuid_cmp(&s->uuid, uuid);
>>>>>> +}
>>>>>> +
>>>>>> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>>>> size_t len)
>>>>>> {
>>>>>> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>>>> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
>>>>>> return false;
>>>>>>
>>>>>> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
>>>>>> + if (new_data) {
>>>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>>>> + return false;
>>>>>
>>>>> Similarly here as from above, it would be better to replace the data.
>>>>>
>>>>>> + new_data->data = realloc(new_data->data, len);
>>>>>> + memcpy(new_data->data, data, len);
>>>>>> + new_data->len = len;
>>>>>> + return true;
>>>>>> + }
>>>>>> +
>>>>>> new_data = new0(struct bt_ad_service_data, 1);
>>>>>> if (!new_data)
>>>>>> return false;
>>>>>> --
>>>>>> 2.1.0
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>>> the body of a message to [email protected]
>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Marie Janssen
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>>
>>
>> --
>> Marie Janssen
>
>
>
> --
> Luiz Augusto von Dentz
--
Marie Janssen
Hi Marie,
On Tue, May 19, 2015 at 8:16 PM, Marie Janssen <[email protected]> wrote:
> Hi Luiz,
>
> On Mon, May 18, 2015 at 4:14 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Marie,
>>
>> On Fri, May 15, 2015 at 7:04 PM, Marie Janssen <[email protected]> wrote:
>>> On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <[email protected]> wrote:
>>>> Hi Luiz,
>>>>
>>>> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
>>>> <[email protected]> wrote:
>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>
>>>>> If either manufacturer or service data already exist but the content
>>>>> is different replace with the new data.
>>>>> ---
>>>>> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>>>> 1 file changed, 38 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/shared/ad.c b/src/shared/ad.c
>>>>> index 00d138d..dfc6b9a 100644
>>>>> --- a/src/shared/ad.c
>>>>> +++ b/src/shared/ad.c
>>>>> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
>>>>> queue_remove_all(ad->service_uuids, NULL, NULL, free);
>>>>> }
>>>>>
>>>>> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
>>>>> +{
>>>>> + const struct bt_ad_manufacturer_data *m = data;
>>>>> + uint16_t id = PTR_TO_UINT(user_data);
>>>>> +
>>>>> + return m->manufacturer_id == id;
>>>>> +}
>>>>> +
>>>>> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>> void *data, size_t len)
>>>>> {
>>>>> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
>>>>> return false;
>>>>>
>>>>> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
>>>>> + UINT_TO_PTR(manufacturer_id));
>>>>> + if (new_data) {
>>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>>> + return false;
>>>>
>>>> I don't think we want this check. If the data is different but length
>>>> the same, we still want to update the data.
>>>
>>> Sorry I read this wrong. We would want to return true here because
>>> the data in the ad is as expected.
>>
>> This is to prevent emitting duplicated signals since in fact it did
>> not change anything.
>>
>
> Previously the false return value meant that some error had occurred,
> such as memory allocation failure or the data being too long for the
> ad. Having nothing change and the ad be basically intact seems like
> not a problem. I guess this is a API semantics question, whether a
> true return value means "the data is set correctly" vs "the data is
> updated".
While this is true there is also the convenience side of things, lets
say we return true then I will need to check if the same data exists
before calling this function because otherwise I can't tell if
anything has changed and that basically means we have 2 checks instead
of one. Beside the action here is to some data, if we are not adding
or replacing anything Id say returning false is fine, btw if you look
at the blocks Im removing that is already the behavior we have.
>>>>> + new_data->data = realloc(new_data->data, len);
>>>>> + memcpy(new_data->data, data, len);
>>>>> + new_data->len = len;
>>>>> + return true;
>>>>> + }
>>>>> +
>>>>> new_data = new0(struct bt_ad_manufacturer_data, 1);
>>>>> if (!new_data)
>>>>> return false;
>>>>> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>>
>>>>> new_data->len = len;
>>>>>
>>>>> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
>>>>> - manuf_destroy(new_data);
>>>>> - return false;
>>>>> - }
>>>>> -
>>>>> if (queue_push_tail(ad->manufacturer_data, new_data))
>>>>> return true;
>>>>>
>>>>> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
>>>>> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
>>>>> }
>>>>>
>>>>> +
>>>>> +static bool service_uuid_match(const void *data, const void *user_data)
>>>>> +{
>>>>> + const struct bt_ad_service_data *s = data;
>>>>> + const bt_uuid_t *uuid = user_data;
>>>>> +
>>>>> + return bt_uuid_cmp(&s->uuid, uuid);
>>>>> +}
>>>>> +
>>>>> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>>> size_t len)
>>>>> {
>>>>> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>>> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
>>>>> return false;
>>>>>
>>>>> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
>>>>> + if (new_data) {
>>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>>> + return false;
>>>>
>>>> Similarly here as from above, it would be better to replace the data.
>>>>
>>>>> + new_data->data = realloc(new_data->data, len);
>>>>> + memcpy(new_data->data, data, len);
>>>>> + new_data->len = len;
>>>>> + return true;
>>>>> + }
>>>>> +
>>>>> new_data = new0(struct bt_ad_service_data, 1);
>>>>> if (!new_data)
>>>>> return false;
>>>>> --
>>>>> 2.1.0
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>> the body of a message to [email protected]
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>>
>>>> --
>>>> Marie Janssen
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> Marie Janssen
--
Luiz Augusto von Dentz
Hi Luiz,
On Mon, May 18, 2015 at 4:14 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marie,
>
> On Fri, May 15, 2015 at 7:04 PM, Marie Janssen <[email protected]> wrote:
>> On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> If either manufacturer or service data already exist but the content
>>>> is different replace with the new data.
>>>> ---
>>>> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>>> 1 file changed, 38 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/shared/ad.c b/src/shared/ad.c
>>>> index 00d138d..dfc6b9a 100644
>>>> --- a/src/shared/ad.c
>>>> +++ b/src/shared/ad.c
>>>> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
>>>> queue_remove_all(ad->service_uuids, NULL, NULL, free);
>>>> }
>>>>
>>>> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
>>>> +{
>>>> + const struct bt_ad_manufacturer_data *m = data;
>>>> + uint16_t id = PTR_TO_UINT(user_data);
>>>> +
>>>> + return m->manufacturer_id == id;
>>>> +}
>>>> +
>>>> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>> void *data, size_t len)
>>>> {
>>>> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
>>>> return false;
>>>>
>>>> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
>>>> + UINT_TO_PTR(manufacturer_id));
>>>> + if (new_data) {
>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>> + return false;
>>>
>>> I don't think we want this check. If the data is different but length
>>> the same, we still want to update the data.
>>
>> Sorry I read this wrong. We would want to return true here because
>> the data in the ad is as expected.
>
> This is to prevent emitting duplicated signals since in fact it did
> not change anything.
>
Previously the false return value meant that some error had occurred,
such as memory allocation failure or the data being too long for the
ad. Having nothing change and the ad be basically intact seems like
not a problem. I guess this is a API semantics question, whether a
true return value means "the data is set correctly" vs "the data is
updated".
>>>> + new_data->data = realloc(new_data->data, len);
>>>> + memcpy(new_data->data, data, len);
>>>> + new_data->len = len;
>>>> + return true;
>>>> + }
>>>> +
>>>> new_data = new0(struct bt_ad_manufacturer_data, 1);
>>>> if (!new_data)
>>>> return false;
>>>> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>>
>>>> new_data->len = len;
>>>>
>>>> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
>>>> - manuf_destroy(new_data);
>>>> - return false;
>>>> - }
>>>> -
>>>> if (queue_push_tail(ad->manufacturer_data, new_data))
>>>> return true;
>>>>
>>>> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
>>>> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
>>>> }
>>>>
>>>> +
>>>> +static bool service_uuid_match(const void *data, const void *user_data)
>>>> +{
>>>> + const struct bt_ad_service_data *s = data;
>>>> + const bt_uuid_t *uuid = user_data;
>>>> +
>>>> + return bt_uuid_cmp(&s->uuid, uuid);
>>>> +}
>>>> +
>>>> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>> size_t len)
>>>> {
>>>> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>>> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
>>>> return false;
>>>>
>>>> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
>>>> + if (new_data) {
>>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>>> + return false;
>>>
>>> Similarly here as from above, it would be better to replace the data.
>>>
>>>> + new_data->data = realloc(new_data->data, len);
>>>> + memcpy(new_data->data, data, len);
>>>> + new_data->len = len;
>>>> + return true;
>>>> + }
>>>> +
>>>> new_data = new0(struct bt_ad_service_data, 1);
>>>> if (!new_data)
>>>> return false;
>>>> --
>>>> 2.1.0
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Marie Janssen
>
>
>
> --
> Luiz Augusto von Dentz
--
Marie Janssen
Hi Marie,
On Fri, May 15, 2015 at 7:04 PM, Marie Janssen <[email protected]> wrote:
> On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> If either manufacturer or service data already exist but the content
>>> is different replace with the new data.
>>> ---
>>> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/shared/ad.c b/src/shared/ad.c
>>> index 00d138d..dfc6b9a 100644
>>> --- a/src/shared/ad.c
>>> +++ b/src/shared/ad.c
>>> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
>>> queue_remove_all(ad->service_uuids, NULL, NULL, free);
>>> }
>>>
>>> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
>>> +{
>>> + const struct bt_ad_manufacturer_data *m = data;
>>> + uint16_t id = PTR_TO_UINT(user_data);
>>> +
>>> + return m->manufacturer_id == id;
>>> +}
>>> +
>>> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>> void *data, size_t len)
>>> {
>>> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
>>> return false;
>>>
>>> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
>>> + UINT_TO_PTR(manufacturer_id));
>>> + if (new_data) {
>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>> + return false;
>>
>> I don't think we want this check. If the data is different but length
>> the same, we still want to update the data.
>
> Sorry I read this wrong. We would want to return true here because
> the data in the ad is as expected.
This is to prevent emitting duplicated signals since in fact it did
not change anything.
>>> + new_data->data = realloc(new_data->data, len);
>>> + memcpy(new_data->data, data, len);
>>> + new_data->len = len;
>>> + return true;
>>> + }
>>> +
>>> new_data = new0(struct bt_ad_manufacturer_data, 1);
>>> if (!new_data)
>>> return false;
>>> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>>
>>> new_data->len = len;
>>>
>>> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
>>> - manuf_destroy(new_data);
>>> - return false;
>>> - }
>>> -
>>> if (queue_push_tail(ad->manufacturer_data, new_data))
>>> return true;
>>>
>>> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
>>> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
>>> }
>>>
>>> +
>>> +static bool service_uuid_match(const void *data, const void *user_data)
>>> +{
>>> + const struct bt_ad_service_data *s = data;
>>> + const bt_uuid_t *uuid = user_data;
>>> +
>>> + return bt_uuid_cmp(&s->uuid, uuid);
>>> +}
>>> +
>>> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>> size_t len)
>>> {
>>> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>>> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
>>> return false;
>>>
>>> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
>>> + if (new_data) {
>>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>>> + return false;
>>
>> Similarly here as from above, it would be better to replace the data.
>>
>>> + new_data->data = realloc(new_data->data, len);
>>> + memcpy(new_data->data, data, len);
>>> + new_data->len = len;
>>> + return true;
>>> + }
>>> +
>>> new_data = new0(struct bt_ad_service_data, 1);
>>> if (!new_data)
>>> return false;
>>> --
>>> 2.1.0
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Marie Janssen
--
Luiz Augusto von Dentz
On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <[email protected]> wrote:
> Hi Luiz,
>
> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> If either manufacturer or service data already exist but the content
>> is different replace with the new data.
>> ---
>> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/shared/ad.c b/src/shared/ad.c
>> index 00d138d..dfc6b9a 100644
>> --- a/src/shared/ad.c
>> +++ b/src/shared/ad.c
>> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
>> queue_remove_all(ad->service_uuids, NULL, NULL, free);
>> }
>>
>> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
>> +{
>> + const struct bt_ad_manufacturer_data *m = data;
>> + uint16_t id = PTR_TO_UINT(user_data);
>> +
>> + return m->manufacturer_id == id;
>> +}
>> +
>> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>> void *data, size_t len)
>> {
>> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
>> return false;
>>
>> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
>> + UINT_TO_PTR(manufacturer_id));
>> + if (new_data) {
>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>> + return false;
>
> I don't think we want this check. If the data is different but length
> the same, we still want to update the data.
Sorry I read this wrong. We would want to return true here because
the data in the ad is as expected.
>> + new_data->data = realloc(new_data->data, len);
>> + memcpy(new_data->data, data, len);
>> + new_data->len = len;
>> + return true;
>> + }
>> +
>> new_data = new0(struct bt_ad_manufacturer_data, 1);
>> if (!new_data)
>> return false;
>> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>>
>> new_data->len = len;
>>
>> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
>> - manuf_destroy(new_data);
>> - return false;
>> - }
>> -
>> if (queue_push_tail(ad->manufacturer_data, new_data))
>> return true;
>>
>> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
>> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
>> }
>>
>> +
>> +static bool service_uuid_match(const void *data, const void *user_data)
>> +{
>> + const struct bt_ad_service_data *s = data;
>> + const bt_uuid_t *uuid = user_data;
>> +
>> + return bt_uuid_cmp(&s->uuid, uuid);
>> +}
>> +
>> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>> size_t len)
>> {
>> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
>> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
>> return false;
>>
>> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
>> + if (new_data) {
>> + if (new_data->len == len && !memcmp(new_data->data, data, len))
>> + return false;
>
> Similarly here as from above, it would be better to replace the data.
>
>> + new_data->data = realloc(new_data->data, len);
>> + memcpy(new_data->data, data, len);
>> + new_data->len = len;
>> + return true;
>> + }
>> +
>> new_data = new0(struct bt_ad_service_data, 1);
>> if (!new_data)
>> return false;
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Marie Janssen
Hi Luiz,
On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> If either manufacturer or service data already exist but the content
> is different replace with the new data.
> ---
> src/shared/ad.c | 43 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/src/shared/ad.c b/src/shared/ad.c
> index 00d138d..dfc6b9a 100644
> --- a/src/shared/ad.c
> +++ b/src/shared/ad.c
> @@ -431,6 +431,14 @@ void bt_ad_clear_service_uuid(struct bt_ad *ad)
> queue_remove_all(ad->service_uuids, NULL, NULL, free);
> }
>
> +static bool manufacturer_id_data_match(const void *data, const void *user_data)
> +{
> + const struct bt_ad_manufacturer_data *m = data;
> + uint16_t id = PTR_TO_UINT(user_data);
> +
> + return m->manufacturer_id == id;
> +}
> +
> bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
> void *data, size_t len)
> {
> @@ -442,6 +450,17 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
> if (len > (MAX_ADV_DATA_LEN - 2 - sizeof(uint16_t)))
> return false;
>
> + new_data = queue_find(ad->manufacturer_data, manufacturer_id_data_match,
> + UINT_TO_PTR(manufacturer_id));
> + if (new_data) {
> + if (new_data->len == len && !memcmp(new_data->data, data, len))
> + return false;
I don't think we want this check. If the data is different but length
the same, we still want to update the data.
> + new_data->data = realloc(new_data->data, len);
> + memcpy(new_data->data, data, len);
> + new_data->len = len;
> + return true;
> + }
> +
> new_data = new0(struct bt_ad_manufacturer_data, 1);
> if (!new_data)
> return false;
> @@ -458,11 +477,6 @@ bool bt_ad_add_manufacturer_data(struct bt_ad *ad, uint16_t manufacturer_id,
>
> new_data->len = len;
>
> - if (bt_ad_has_manufacturer_data(ad, new_data)) {
> - manuf_destroy(new_data);
> - return false;
> - }
> -
> if (queue_push_tail(ad->manufacturer_data, new_data))
> return true;
>
> @@ -556,6 +570,15 @@ void bt_ad_clear_solicit_uuid(struct bt_ad *ad)
> queue_remove_all(ad->solicit_uuids, NULL, NULL, free);
> }
>
> +
> +static bool service_uuid_match(const void *data, const void *user_data)
> +{
> + const struct bt_ad_service_data *s = data;
> + const bt_uuid_t *uuid = user_data;
> +
> + return bt_uuid_cmp(&s->uuid, uuid);
> +}
> +
> bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
> size_t len)
> {
> @@ -567,6 +590,16 @@ bool bt_ad_add_service_data(struct bt_ad *ad, const bt_uuid_t *uuid, void *data,
> if (len > (MAX_ADV_DATA_LEN - 2 - (size_t)bt_uuid_len(uuid)))
> return false;
>
> + new_data = queue_find(ad->service_uuids, service_uuid_match, uuid);
> + if (new_data) {
> + if (new_data->len == len && !memcmp(new_data->data, data, len))
> + return false;
Similarly here as from above, it would be better to replace the data.
> + new_data->data = realloc(new_data->data, len);
> + memcpy(new_data->data, data, len);
> + new_data->len = len;
> + return true;
> + }
> +
> new_data = new0(struct bt_ad_service_data, 1);
> if (!new_data)
> return false;
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Marie Janssen