All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix bug that caused a profile's accept() not to be called
@ 2015-12-04 11:36 Felipe F. Tonello
  2015-12-04 11:36 ` [PATCH 1/2] core/device: call btd_service_disconnect when discconected from peripheral Felipe F. Tonello
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Felipe F. Tonello @ 2015-12-04 11:36 UTC (permalink / raw)
  To: linux-bluetooth

A profile's accept function callback was only been called once. If a device
was disconnected and reconnected, the accept would not been called because the
service's states were not been update accordingly.

These patches fixes this problem.

Felipe F. Tonello (2):
  core/device: call btd_service_disconnect when discconected from
    peripheral
  core/service: fix connection and disconnection state handling

 src/device.c  |  1 +
 src/service.c | 43 ++++++++++++++++++++++---------------------
 2 files changed, 23 insertions(+), 21 deletions(-)

-- 
2.5.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] core/device: call btd_service_disconnect when discconected from peripheral
  2015-12-04 11:36 [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe F. Tonello
@ 2015-12-04 11:36 ` Felipe F. Tonello
  2015-12-04 11:36 ` [PATCH 2/2] core/service: fix connection and disconnection state handling Felipe F. Tonello
  2015-12-11 17:05 ` [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe Ferreri Tonello
  2 siblings, 0 replies; 8+ messages in thread
From: Felipe F. Tonello @ 2015-12-04 11:36 UTC (permalink / raw)
  To: linux-bluetooth

This call was missing, so services were never changing its state properly and
never called profiles disconnect callback function as well.
---
 src/device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/device.c b/src/device.c
index 9021914..d4fe861 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4479,6 +4479,7 @@ static void att_disconnected_cb(int err, void *user_data)
 	DBG("%s (%d)", strerror(err), err);
 
 	g_slist_foreach(device->attios, attio_disconnected, NULL);
+	g_slist_foreach(device->services, dev_disconn_service, NULL);
 
 	btd_gatt_client_disconnected(device->client_dbus);
 
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] core/service: fix connection and disconnection state handling
  2015-12-04 11:36 [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe F. Tonello
  2015-12-04 11:36 ` [PATCH 1/2] core/device: call btd_service_disconnect when discconected from peripheral Felipe F. Tonello
@ 2015-12-04 11:36 ` Felipe F. Tonello
  2015-12-04 13:02   ` Luiz Augusto von Dentz
  2015-12-11 17:05 ` [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe Ferreri Tonello
  2 siblings, 1 reply; 8+ messages in thread
From: Felipe F. Tonello @ 2015-12-04 11:36 UTC (permalink / raw)
  To: linux-bluetooth

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);
+		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;
+		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);
 
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] core/service: fix connection and disconnection state handling
  2015-12-04 11:36 ` [PATCH 2/2] core/service: fix connection and disconnection state handling Felipe F. Tonello
@ 2015-12-04 13:02   ` Luiz Augusto von Dentz
  2015-12-04 14:44     ` Felipe Ferreri Tonello
  0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2015-12-04 13:02 UTC (permalink / raw)
  To: Felipe F. Tonello; +Cc: linux-bluetooth

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.

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

> +               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);
>
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] core/service: fix connection and disconnection state handling
  2015-12-04 13:02   ` Luiz Augusto von Dentz
@ 2015-12-04 14:44     ` Felipe Ferreri Tonello
  2015-12-11 22:52       ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 8+ messages in thread
From: Felipe Ferreri Tonello @ 2015-12-04 14:44 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] Fix bug that caused a profile's accept() not to be called
  2015-12-04 11:36 [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe F. Tonello
  2015-12-04 11:36 ` [PATCH 1/2] core/device: call btd_service_disconnect when discconected from peripheral Felipe F. Tonello
  2015-12-04 11:36 ` [PATCH 2/2] core/service: fix connection and disconnection state handling Felipe F. Tonello
@ 2015-12-11 17:05 ` Felipe Ferreri Tonello
  2 siblings, 0 replies; 8+ messages in thread
From: Felipe Ferreri Tonello @ 2015-12-11 17:05 UTC (permalink / raw)
  To: linux-bluetooth

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

Hi all,

On 04/12/15 11:36, Felipe F. Tonello wrote:
> A profile's accept function callback was only been called once. If a device
> was disconnected and reconnected, the accept would not been called because the
> service's states were not been update accordingly.
> 
> These patches fixes this problem.
> 
> Felipe F. Tonello (2):
>   core/device: call btd_service_disconnect when discconected from
>     peripheral
>   core/service: fix connection and disconnection state handling
> 
>  src/device.c  |  1 +
>  src/service.c | 43 ++++++++++++++++++++++---------------------
>  2 files changed, 23 insertions(+), 21 deletions(-)
> 

Ping?

-- 
Felipe

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] core/service: fix connection and disconnection state handling
  2015-12-04 14:44     ` Felipe Ferreri Tonello
@ 2015-12-11 22:52       ` Luiz Augusto von Dentz
  2016-01-11 15:01         ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2015-12-11 22:52 UTC (permalink / raw)
  To: Felipe Ferreri Tonello; +Cc: linux-bluetooth

Hi Felipe,

On Fri, Dec 4, 2015 at 12:44 PM, Felipe Ferreri Tonello
<eu@felipetonello.com> wrote:
> 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.

Well perhaps we should set connected if accept succeed, or the plugin
has to call btd_service_connecting_complete like when
btd_service_connect is called I guess this makes more sense since the
profile may have to do a few things before it is considered connected
so it would work similarly for both incoming or outgoing connections.

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



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] core/service: fix connection and disconnection state handling
  2015-12-11 22:52       ` Luiz Augusto von Dentz
@ 2016-01-11 15:01         ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2016-01-11 15:01 UTC (permalink / raw)
  To: Felipe Ferreri Tonello; +Cc: linux-bluetooth

Hi Felipe,

On Fri, Dec 11, 2015 at 7:52 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Felipe,
>
> On Fri, Dec 4, 2015 at 12:44 PM, Felipe Ferreri Tonello
> <eu@felipetonello.com> wrote:
>> 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.
>
> Well perhaps we should set connected if accept succeed, or the plugin
> has to call btd_service_connecting_complete like when
> btd_service_connect is called I guess this makes more sense since the
> profile may have to do a few things before it is considered connected
> so it would work similarly for both incoming or outgoing connections.
>
>>>
>>>> +               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

Are you still working in this, I was planning to convert HoG to
actually use accept callback and remove attio to make sure this works
properly.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-01-11 15:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-04 11:36 [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe F. Tonello
2015-12-04 11:36 ` [PATCH 1/2] core/device: call btd_service_disconnect when discconected from peripheral Felipe F. Tonello
2015-12-04 11:36 ` [PATCH 2/2] core/service: fix connection and disconnection state handling Felipe F. Tonello
2015-12-04 13:02   ` Luiz Augusto von Dentz
2015-12-04 14:44     ` Felipe Ferreri Tonello
2015-12-11 22:52       ` Luiz Augusto von Dentz
2016-01-11 15:01         ` Luiz Augusto von Dentz
2015-12-11 17:05 ` [PATCH 0/2] Fix bug that caused a profile's accept() not to be called Felipe Ferreri Tonello

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.