Re: [PATCH 2/2] core/service: fix connection and disconnection state handling

[Felipe Ferreri Tonello <eu@felipetonello.com>]

[-- Attachment #1: Type: text/plain, Size: 4728 bytes --]

Hi Luiz,

On 12/04/2015 01:02 PM, Luiz Augusto von Dentz wrote:
> Hi Felipe,
> 
> On Fri, Dec 4, 2015 at 1:36 PM, Felipe F. Tonello <eu@felipetonello.com> wrote:
>> Service state was not been update correctly if a profile didn't implement
>> state changes itself. This patch makes sure that states will be always
>> properly update, even if a profile doesn't implement itself.
>>
>> As a side effect, this fix a bug causing a profile's accept function not to be
>> called on a connection after a disconnection.
>> ---
>>  src/service.c | 43 ++++++++++++++++++++++---------------------
>>  1 file changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/service.c b/src/service.c
>> index f7912f5..85694a5 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -219,9 +219,6 @@ int btd_service_connect(struct btd_service *service)
>>         char addr[18];
>>         int err;
>>
>> -       if (!profile->connect)
>> -               return -ENOTSUP;
>> -
>>         switch (service->state) {
>>         case BTD_SERVICE_STATE_UNAVAILABLE:
>>                 return -EINVAL;
>> @@ -235,15 +232,21 @@ int btd_service_connect(struct btd_service *service)
>>                 return -EBUSY;
>>         }
>>
>> +       change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>> +
>> +       if (!profile->connect) {
>> +               btd_service_connecting_complete(service, 0);
> 
> Hmm, I wonder if the will actually work properly since
> btd_service_connecting_complete will set the state to connected if
> error is 0 but it is not actually connected. Actually if accept is to
> be called connect should not be called.

Well, at least on my tests it works fine. How come it is not actually
connected? This function is called when a connection happens.

Why can't we call both? A profile will not install both callbacks
anyway. But I think the states should be updated anyway, because that is
the root cause of all problems here.

The main objective of this patch is to update the states.

> 
>> +               return -ENOTSUP;
>> +       }
>> +
>>         err = profile->connect(service);
>> -       if (err == 0) {
>> -               change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>> -               return 0;
>> +       if (err < 0) {
>> +               ba2str(device_get_address(service->device), addr);
>> +               error("%s profile connect failed for %s: %s", profile->name, addr,
>> +                     strerror(-err));
>>         }
>>
>> -       ba2str(device_get_address(service->device), addr);
>> -       error("%s profile connect failed for %s: %s", profile->name, addr,
>> -                                                               strerror(-err));
>> +       btd_service_connecting_complete(service, err);
>>
>>         return err;
>>  }
>> @@ -254,9 +257,6 @@ int btd_service_disconnect(struct btd_service *service)
>>         char addr[18];
>>         int err;
>>
>> -       if (!profile->disconnect)
>> -               return -ENOTSUP;
>> -
>>         switch (service->state) {
>>         case BTD_SERVICE_STATE_UNAVAILABLE:
>>                 return -EINVAL;
>> @@ -270,18 +270,19 @@ int btd_service_disconnect(struct btd_service *service)
>>
>>         change_state(service, BTD_SERVICE_STATE_DISCONNECTING, 0);
>>
>> -       err = profile->disconnect(service);
>> -       if (err == 0)
>> -               return 0;
>> -
>> -       if (err == -ENOTCONN) {
>> +       if (!profile->disconnect) {
>>                 btd_service_disconnecting_complete(service, 0);
>> -               return 0;
> 
> This I think was actually correct, because if the profile don't
> implement .disconnect we still can disconnect when ACL is dropped but
> contrary to connect I think we can return 0 since the state will
> actually be correct.

Like I mentioned before, the problem was that the state was not been
updated. It shouldn't be the job for the profile to do it so.

This makes sure that we update the service->state correctly regardless
of profile's disconnect existence.

> 
>> +               return -ENOTSUP;
>>         }
>>
>> -       ba2str(device_get_address(service->device), addr);
>> -       error("%s profile disconnect failed for %s: %s", profile->name, addr,
>> -                                                               strerror(-err));
>> +       err = profile->disconnect(service);
>> +       if (err == -ENOTCONN) {
>> +               err = 0;
>> +       } else if (err < 0) {
>> +               ba2str(device_get_address(service->device), addr);
>> +               error("%s profile disconnect failed for %s: %s", profile->name, addr,
>> +                     strerror(-err));
>> +       }
>>
>>         btd_service_disconnecting_complete(service, err);
>>

Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]