2014-10-27 14:01:25

by Andrei Emeltchenko

Subject: [RFC] gatt: Fix wrong function exit

From: Andrei Emeltchenko <[email protected]>

---
src/shared/gatt-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index d751d5a..4f53de3 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1029,7 +1029,7 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
data = new_read_included(cur_result);
if (!data) {
success = false;
- goto done;
+ goto failed;
}

read_included(data);
--
1.9.1

2014-10-27 14:44:08

by Marcin Kraglak

Subject: Re: [RFC] gatt: Fix wrong function exit

Hi Arman,

On 27 October 2014 15:41, Arman Uguray <[email protected]> wrote:
> Hi Marcin,
>
> On Mon, Oct 27, 2014 at 7:14 AM, Marcin Kraglak
> <[email protected]> wrote:
>> Hi Andrei,
>>
>> On 27 October 2014 15:01, Andrei Emeltchenko
>> <[email protected]> wrote:
>>> From: Andrei Emeltchenko <[email protected]>
>>>
>>> ---
>>> src/shared/gatt-helpers.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
>>> index d751d5a..4f53de3 100644
>>> --- a/src/shared/gatt-helpers.c
>>> +++ b/src/shared/gatt-helpers.c
>>> @@ -1029,7 +1029,7 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
>>> data = new_read_included(cur_result);
>>> if (!data) {
>>> success = false;
>>> - goto done;
>>> + goto failed;
>>> }
>>>
>>> read_included(data);
>>> --
>>> 1.9.1
>> In fact exit here is wrong and should be goto failed.
>> Anyway, I found that we call goto failed more times in this function
>> than goto done, so maybe it is better to change labels like this:
>> failed:
>> success = false;
>>
>> done:
>> if (op->callback)
>> op->callback(success, att_ecode, final_result, op->user_data);
>>
>> and set success = true before goto done;
>>
>
> Might actually be better leave the current structure as it is, except
> initialize success = false at declaration. Then code would simply call
> goto fail and not set success to false every time.

Sounds good
>
> -Arman

BR
Marcin

2014-10-27 14:41:07

by Arman Uguray

Subject: Re: [RFC] gatt: Fix wrong function exit

Hi Marcin,

On Mon, Oct 27, 2014 at 7:14 AM, Marcin Kraglak
<[email protected]> wrote:
> Hi Andrei,
>
> On 27 October 2014 15:01, Andrei Emeltchenko
> <[email protected]> wrote:
>> From: Andrei Emeltchenko <[email protected]>
>>
>> ---
>> src/shared/gatt-helpers.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
>> index d751d5a..4f53de3 100644
>> --- a/src/shared/gatt-helpers.c
>> +++ b/src/shared/gatt-helpers.c
>> @@ -1029,7 +1029,7 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
>> data = new_read_included(cur_result);
>> if (!data) {
>> success = false;
>> - goto done;
>> + goto failed;
>> }
>>
>> read_included(data);
>> --
>> 1.9.1
> In fact exit here is wrong and should be goto failed.
> Anyway, I found that we call goto failed more times in this function
> than goto done, so maybe it is better to change labels like this:
> failed:
> success = false;
>
> done:
> if (op->callback)
> op->callback(success, att_ecode, final_result, op->user_data);
>
> and set success = true before goto done;
>

Might actually be better leave the current structure as it is, except
initialize success = false at declaration. Then code would simply call
goto fail and not set success to false every time.

-Arman

2014-10-27 14:14:10

by Marcin Kraglak

Subject: Re: [RFC] gatt: Fix wrong function exit

Hi Andrei,

On 27 October 2014 15:01, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> ---
> src/shared/gatt-helpers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index d751d5a..4f53de3 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -1029,7 +1029,7 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
> data = new_read_included(cur_result);
> if (!data) {
> success = false;
> - goto done;
> + goto failed;
> }
>
> read_included(data);
> --
> 1.9.1
In fact exit here is wrong and should be goto failed.
Anyway, I found that we call goto failed more times in this function
than goto done, so maybe it is better to change labels like this:
failed:
success = false;

done:
if (op->callback)
op->callback(success, att_ecode, final_result, op->user_data);

and set success = true before goto done;

>
> --
> 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

BR
Marcin