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

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

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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);
 	return 0;
 }
 
-- 
2.7.4

[PATCH BlueZ 2/5] core/device: Call profile disconnect if ATT disconnects

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If the profile accepts connections it should also be notified when ATT
disconnects so it can cleanup properly.
---
 src/device.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/device.c b/src/device.c
index 9586022..eda873f 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4563,6 +4563,18 @@ static void attio_disconnected(gpointer data, gpointer user_data)
 		attio->dcfunc(attio->user_data);
 }
 
+static void disconnect_gatt_service(gpointer data, gpointer user_data)
+{
+	struct btd_service *service = data;
+	struct btd_profile *profile = btd_service_get_profile(service);
+
+	/* Ignore if profile cannot accept connections */
+	if (!profile->accept)
+		return;
+
+	btd_service_disconnect(service);
+}
+
 static void att_disconnected_cb(int err, void *user_data)
 {
 	struct btd_device *device = user_data;
@@ -4575,6 +4587,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, disconnect_gatt_service, NULL);
 
 	btd_gatt_client_disconnected(device->client_dbus);
 
-- 
2.7.4

[PATCH BlueZ 3/5] scanparam: Fix not handling accept properly

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

On accept the profile shall check about existing attribute, etc and once
done call btd_service_connecting_complete updating the service state
properly.
---
 profiles/scanparam/scan.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index d3ca762..d12e09e 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -201,7 +201,6 @@ static int scan_param_accept(struct btd_service *service)
 	gatt_db_unref(scan->db);
 	bt_gatt_client_unref(scan->client);
 
-
 	scan->db = gatt_db_ref(db);
 	scan->client = bt_gatt_client_ref(client);
 
@@ -209,6 +208,17 @@ static int scan_param_accept(struct btd_service *service)
 	gatt_db_foreach_service(db, &scan_parameters_uuid,
 					foreach_scan_param_service, scan);
 
+	if (!scan->attr) {
+		error("Scan Parameters attribute not found");
+		gatt_db_unref(scan->db);
+		scan->db = NULL;
+		bt_gatt_client_unref(scan->client);
+		scan->client = NULL;
+		return -1;
+	}
+
+	btd_service_connecting_complete(service, 0);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH BlueZ 4/5] scanparam: Make use of service user_data to store service context

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Instead of storing service context data in a list this make use of
btd_service_set_user_data to store the context data which later can be
retrieved with btd_service_get_user_data.
---
 profiles/scanparam/scan.c | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index d12e09e..f8ad810 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -62,8 +62,6 @@ struct scan {
 	guint refresh_cb_id;
 };
 
-static GSList *devices;
-
 static void scan_free(struct scan *scan)
 {
 	bt_gatt_client_unregister_notify(scan->client, scan->refresh_cb_id);
@@ -73,14 +71,6 @@ static void scan_free(struct scan *scan)
 	g_free(scan);
 }
 
-static int cmp_device(gconstpointer a, gconstpointer b)
-{
-	const struct scan *scan = a;
-	const struct btd_device *device = b;
-
-	return scan->device == device ? 0 : -1;
-}
-
 static void write_scan_params(struct scan *scan)
 {
 	uint8_t value[4];
@@ -181,21 +171,17 @@ static int scan_param_accept(struct btd_service *service)
 	struct gatt_db *db = btd_device_get_gatt_db(device);
 	struct bt_gatt_client *client = btd_device_get_gatt_client(device);
 	bt_uuid_t scan_parameters_uuid;
-	struct scan *scan;
-	GSList *l;
+	struct scan *scan = btd_service_get_user_data(service);
 	char addr[18];
 
 	ba2str(device_get_address(device), addr);
 	DBG("Scan Parameters Client Driver profile accept (%s)", addr);
 
-	l = g_slist_find_custom(devices, device, cmp_device);
-	if (!l) {
+	if (!scan) {
 		error("Scan Parameters service not handled by profile");
 		return -1;
 	}
 
-	scan = l->data;
-
 	/* Clean-up any old client/db and acquire the new ones */
 	scan->attr = NULL;
 	gatt_db_unref(scan->db);
@@ -226,21 +212,17 @@ static void scan_param_remove(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
 	struct scan *scan;
-	GSList *l;
 	char addr[18];
 
 	ba2str(device_get_address(device), addr);
 	DBG("GAP profile remove (%s)", addr);
 
-	l = g_slist_find_custom(devices, device, cmp_device);
-	if (!l) {
+	scan = btd_service_get_user_data(service);
+	if (!scan) {
 		error("GAP service not handled by profile");
 		return;
 	}
 
-	scan = l->data;
-
-	devices = g_slist_remove(devices, scan);
 	scan_free(scan);
 }
 
@@ -248,16 +230,15 @@ static int scan_param_probe(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
 	struct scan *scan;
-	GSList *l;
 	char addr[18];
 
 	ba2str(device_get_address(device), addr);
 	DBG("Scan Parameters Client Driver profile probe (%s)", addr);
 
 	/* Ignore, if we were probed for this device already */
-	l = g_slist_find_custom(devices, device, cmp_device);
-	if (l) {
-		error("Profile probed twice for the same device!");
+	scan = btd_service_get_user_data(service);
+	if (scan) {
+		error("Profile probed twice for the same service!");
 		return -1;
 	}
 
@@ -266,7 +247,7 @@ static int scan_param_probe(struct btd_service *service)
 		return -1;
 
 	scan->device = btd_device_ref(device);
-	devices = g_slist_append(devices, scan);
+	btd_service_set_user_data(service, scan);
 	return 0;
 }
 
-- 
2.7.4

[PATCH BlueZ 5/5] scanparam: Implement disconnect callback

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This implements the profile disconnect callback using it to cleanup
existing references of bt_gatt_client and gatt_db.
---
 profiles/scanparam/scan.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index f8ad810..0ff4a43 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -165,6 +165,15 @@ static void foreach_scan_param_service(struct gatt_db_attribute *attr,
 	gatt_db_service_foreach_char(scan->attr, handle_characteristic, scan);
 }
 
+static void scan_reset(struct scan *scan)
+{
+	scan->attr = NULL;
+	gatt_db_unref(scan->db);
+	scan->db = NULL;
+	bt_gatt_client_unref(scan->client);
+	scan->client = NULL;
+}
+
 static int scan_param_accept(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
@@ -182,11 +191,6 @@ static int scan_param_accept(struct btd_service *service)
 		return -1;
 	}
 
-	/* Clean-up any old client/db and acquire the new ones */
-	scan->attr = NULL;
-	gatt_db_unref(scan->db);
-	bt_gatt_client_unref(scan->client);
-
 	scan->db = gatt_db_ref(db);
 	scan->client = bt_gatt_client_ref(client);
 
@@ -196,10 +200,7 @@ static int scan_param_accept(struct btd_service *service)
 
 	if (!scan->attr) {
 		error("Scan Parameters attribute not found");
-		gatt_db_unref(scan->db);
-		scan->db = NULL;
-		bt_gatt_client_unref(scan->client);
-		scan->client = NULL;
+		scan_reset(scan);
 		return -1;
 	}
 
@@ -208,6 +209,17 @@ static int scan_param_accept(struct btd_service *service)
 	return 0;
 }
 
+static int scan_param_disconnect(struct btd_service *service)
+{
+	struct scan *scan = btd_service_get_user_data(service);
+
+	scan_reset(scan);
+
+	btd_service_disconnecting_complete(service, 0);
+
+	return 0;
+}
+
 static void scan_param_remove(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
@@ -257,6 +269,7 @@ static struct btd_profile scan_profile = {
 	.device_probe = scan_param_probe,
 	.device_remove = scan_param_remove,
 	.accept = scan_param_accept,
+	.disconnect = scan_param_disconnect,
 };
 
 static int scan_param_init(void)
-- 
2.7.4

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

[Felipe Ferreri Tonello]

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

Hi Luiz,

On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> 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.

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?

I think btd_service_connecting_complete() should always be called with
the same err as returned by profile->accept()

>  	return 0;
>  }
>  
> 

-- 
Felipe

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

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

[Luiz Augusto von Dentz]

Hi Felipe,

On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
<eu@felipetonello.com> wrote:
> Hi Luiz,
>
> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> 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.

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

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

>
>>       return 0;
>>  }
>>
>>
>
> --
> Felipe



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/5] core/device: Call profile disconnect if ATT disconnects

[Felipe Ferreri Tonello]

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

Hi Luiz,

On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> If the profile accepts connections it should also be notified when ATT
> disconnects so it can cleanup properly.
> ---
>  src/device.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/device.c b/src/device.c
> index 9586022..eda873f 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4563,6 +4563,18 @@ static void attio_disconnected(gpointer data, gpointer user_data)
>  		attio->dcfunc(attio->user_data);
>  }
>  
> +static void disconnect_gatt_service(gpointer data, gpointer user_data)
> +{
> +	struct btd_service *service = data;
> +	struct btd_profile *profile = btd_service_get_profile(service);
> +
> +	/* Ignore if profile cannot accept connections */
> +	if (!profile->accept)
> +		return;

What if the profile has connect and not accept?

> +
> +	btd_service_disconnect(service);
> +}
> +
>  static void att_disconnected_cb(int err, void *user_data)
>  {
>  	struct btd_device *device = user_data;
> @@ -4575,6 +4587,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, disconnect_gatt_service, NULL);
>  
>  	btd_gatt_client_disconnected(device->client_dbus);
>  
> 

-- 
Felipe

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

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

[Felipe Ferreri Tonello]

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

Hi Luiz,

On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
> Hi Felipe,
> 
> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
> <eu@felipetonello.com> wrote:
>> Hi Luiz,
>>
>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> 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.
> 
>> 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?
> 
>> 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.

> 
>>
>>>       return 0;
>>>  }
>>>
>>>
>>

-- 
Felipe

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

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

[Luiz Augusto von Dentz]

Hi Felipe,

On Thu, Sep 8, 2016 at 6:37 PM, Felipe Ferreri Tonello
<eu@felipetonello.com> wrote:
> Hi Luiz,
>
> On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
>> Hi Felipe,
>>
>> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
>> <eu@felipetonello.com> wrote:
>>> Hi Luiz,
>>>
>>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>
>>>> 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.
>>
>>> 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?
>>
>>> 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

Re: [PATCH BlueZ 2/5] core/device: Call profile disconnect if ATT disconnects

[Luiz Augusto von Dentz]

Hi Felipe,

On Thu, Sep 8, 2016 at 6:33 PM, Felipe Ferreri Tonello
<eu@felipetonello.com> wrote:
> Hi Luiz,
>
> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> If the profile accepts connections it should also be notified when ATT
>> disconnects so it can cleanup properly.
>> ---
>>  src/device.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/src/device.c b/src/device.c
>> index 9586022..eda873f 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -4563,6 +4563,18 @@ static void attio_disconnected(gpointer data, gpointer user_data)
>>               attio->dcfunc(attio->user_data);
>>  }
>>
>> +static void disconnect_gatt_service(gpointer data, gpointer user_data)
>> +{
>> +     struct btd_service *service = data;
>> +     struct btd_profile *profile = btd_service_get_profile(service);
>> +
>> +     /* Ignore if profile cannot accept connections */
>> +     if (!profile->accept)
>> +             return;
>
> What if the profile has connect and not accept?

GATT drivers can only accept not connect so this specific to avoid
disconnecting BR/EDR profiles if both bearers are connected, we might
as well introduce a flag to indicate the driver is to be using with
GATT only if we want to make it more clear.

>> +
>> +     btd_service_disconnect(service);
>> +}
>> +
>>  static void att_disconnected_cb(int err, void *user_data)
>>  {
>>       struct btd_device *device = user_data;
>> @@ -4575,6 +4587,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, disconnect_gatt_service, NULL);
>>
>>       btd_gatt_client_disconnected(device->client_dbus);
>>
>>
>
> --
> Felipe



-- 
Luiz Augusto von Dentz

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

[Luiz Augusto von Dentz]

Hi,

On Fri, Sep 9, 2016 at 3:35 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Felipe,
>
> On Thu, Sep 8, 2016 at 6:37 PM, Felipe Ferreri Tonello
> <eu@felipetonello.com> wrote:
>> Hi Luiz,
>>
>> On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
>>> Hi Felipe,
>>>
>>> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
>>> <eu@felipetonello.com> wrote:
>>>> Hi Luiz,
>>>>
>>>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>
>>>>> 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.
>>>
>>>> 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?
>>>
>>>> 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.

Applied.

-- 
Luiz Augusto von Dentz