Hi Felipe,
On Fri, Dec 11, 2015 at 7:52 PM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> 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
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
--
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
