Hi,
On Tue, May 19, 2015 at 8:41 PM, Marie Janssen <jamuraa@xxxxxxxxxxxx> wrote:
> Hi Luiz,
>
> On Tue, May 19, 2015 at 10:35 AM, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
>> Hi Marie,
>>
>> On Tue, May 19, 2015 at 8:16 PM, Marie Janssen <jamuraa@xxxxxxxxxxxx> wrote:
>>> Hi Luiz,
>>>
>>> On Mon, May 18, 2015 at 4:14 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@xxxxxxxxx> wrote:
>>>> Hi Marie,
>>>>
>>>> On Fri, May 15, 2015 at 7:04 PM, Marie Janssen <jamuraa@xxxxxxxxxxxx> wrote:
>>>>> On Fri, May 15, 2015 at 9:02 AM, Marie Janssen <jamuraa@xxxxxxxxxxxx> wrote:
>>>>>> Hi Luiz,
>>>>>>
>>>>>> On Fri, May 15, 2015 at 6:30 AM, Luiz Augusto von Dentz
>>>>>> <luiz.dentz@xxxxxxxxx> wrote:
>>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>>>>>>>
>>>>>>> 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 majordomo@xxxxxxxxxxxxxxx
>>>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
