Hi Luiz,

On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
> Hi Felipe,
> 
> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
> <eu@felipetonello.com> wrote:
>> Hi Luiz,
>>
>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> The accept calback may transit the state to connected on the call itself
>>> since most of the time it is just a matter of selecting the attributes
>>> in case of GATT profiles.
>>> ---
>>>  src/service.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/service.c b/src/service.c
>>> index f387fc4..20a41d0 100644
>>> --- a/src/service.c
>>> +++ b/src/service.c
>>> @@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
>>>       return err;
>>>
>>>  done:
>>> -     change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>> +     if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
>>> +             change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>
>> This looks really hacky - I know it was like this before already.
> 
> This is to avoid resetting the connection if the accept callback has
> set it already.
> 
>> Why can you change the state in two different places? What if the
>> profile->accept doesn't change the state to connected? Will this change
>> to connecting and then the connected state will never be set?
> 
> Going from connected to connecting is not valid, thus if the driver
> had changed the states there is nothing else to be done.

Why do you have this done label if the driver is supposed to call
btd_service_connecting_complete() on success?
> 
>> I think btd_service_connecting_complete() should always be called with
>> the same err as returned by profile->accept()
> 
> Not sure what difference does this make?

My suggestion is to handle the state based on the return value of the
driver's callback and not let the driver to change the state itself.
It's boilerplate code anyway.

> 
>>
>>>       return 0;
>>>  }
>>>
>>>
>>

-- 
Felipe