Re: [PATCH BlueZ 1/5] core/service: Fix setting wrong state after calling accept

Hi Felipe,

On Thu, Sep 8, 2016 at 6:37 PM, Felipe Ferreri Tonello
[off-list ref] wrote:

Hi Luiz,

On 08/09/16 16:26, Luiz Augusto von Dentz wrote:

quoted

Hi Felipe,

On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
[off-list ref] wrote:

quoted

Hi Luiz,

On 08/09/16 13:38, Luiz Augusto von Dentz wrote:

quoted

From: Luiz Augusto von Dentz <redacted>

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.

quoted

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?

quoted

quoted

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.

The driver may have to respond this asynchronously so we can't really
depend on the return alone.

-- 
Luiz Augusto von Dentz