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