[PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[Luiz Augusto von Dentz]

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

This ensures that the service is disconnected before setting the state
to unavailable.
---
 src/service.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/service.c b/src/service.c
index 83e1c1a..dce5c05 100644
--- a/src/service.c
+++ b/src/service.c
@@ -170,6 +170,7 @@ int service_probe(struct btd_service *service)
 
 void service_shutdown(struct btd_service *service)
 {
+	btd_service_disconnect(service);
 	change_state(service, BTD_SERVICE_STATE_UNAVAILABLE, 0);
 	service->profile->device_remove(service);
 	service->device = NULL;
-- 
1.8.1.4

[PATCH BlueZ 2/2] core/device: Fix crash while freeing services list

[Luiz Augusto von Dentz]

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

btd_service do alter its state on service_shutdown which can cause
plugins to attempt to access services list which may have freed some
services already.

To fix this the code now updates the list  in place so any service that
is gonna be shutdown is removed from the list so it no longer accessible
after freed.
---
 src/device.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/device.c b/src/device.c
index edd377c..300c358 100644
--- a/src/device.c
+++ b/src/device.c
@@ -994,8 +994,13 @@ int device_block(struct btd_device *device, gboolean update_only)
 	if (device->connected)
 		do_disconnect(device);
 
-	g_slist_free_full(device->services, remove_service);
-	device->services = NULL;
+	while (device->services != NULL) {
+		struct btd_service *service = device->services->data;
+
+		device->services = g_slist_remove(device->services, service);
+		service_shutdown(service);
+		btd_service_unref(service);
+	}
 
 	if (!update_only)
 		err = btd_adapter_block_address(device->adapter,
@@ -2362,7 +2367,6 @@ static void device_remove_stored(struct btd_device *device)
 
 void device_remove(struct btd_device *device, gboolean remove_stored)
 {
-
 	DBG("Removing device %s", device->path);
 
 	if (device->bonding) {
@@ -2379,7 +2383,13 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
 	if (device->browse)
 		browse_request_cancel(device->browse);
 
-	g_slist_foreach(device->services, dev_disconn_service, NULL);
+	while (device->services != NULL) {
+		struct btd_service *service = device->services->data;
+
+		device->services = g_slist_remove(device->services, service);
+		service_shutdown(service);
+		btd_service_unref(service);
+	}
 
 	g_slist_free(device->pending);
 	device->pending = NULL;
@@ -2398,9 +2408,6 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
 	if (remove_stored)
 		device_remove_stored(device);
 
-	g_slist_free_full(device->services, remove_service);
-	device->services = NULL;
-
 	btd_device_unref(device);
 }
 
-- 
1.8.1.4

Re: [PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[Mikel Astiz]

Hi Luiz,

On Fri, Jul 5, 2013 at 4:03 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This ensures that the service is disconnected before setting the state
> to unavailable.
> ---
>  src/service.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/service.c b/src/service.c
> index 83e1c1a..dce5c05 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -170,6 +170,7 @@ int service_probe(struct btd_service *service)
>
>  void service_shutdown(struct btd_service *service)
>  {
> +       btd_service_disconnect(service);
>         change_state(service, BTD_SERVICE_STATE_UNAVAILABLE, 0);
>         service->profile->device_remove(service);
>         service->device = NULL;
> --
> 1.8.1.4
>
> --
> 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

I'm not a big fan of this approach. The service should already be
disconnected by the time it's shut down, so this additional transition
should not be needed.

Having a look at the current code paths leading to service_shutdown(),
one tricky case I can think of is the call to
device_remove_profiles(), specially from search_cb(). I have my doubts
whether a service shutdown makes sense if it's connected, but in case
yes, I'd add the disconnection code to device_remove_profiles().

Or do you have some other examples where the disconnection is not triggered?

Adding one more side effect to service_shutdown() is IMO undesirable,
where the transitional DISCONNECTED state would be artificially
introduced. Think about an external profile being unregistered while
connected devices exist: not only calling
Profile.RequestDisconnection() doesn't make any sense, but a
transition such as STATE_CONNECTED->STATE_UNAVAILABLE is probably what
you want to observe. This can be different from a graceful
disconnection, and a policy module could use this distinction to
reconnect the service once the external profile gets registered.

Cheers,
Mikel

Re: [PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[Luiz Augusto von Dentz]

Hi Mikel,

On Mon, Jul 8, 2013 at 2:46 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> Hi Luiz,
>
> On Fri, Jul 5, 2013 at 4:03 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This ensures that the service is disconnected before setting the state
>> to unavailable.
>> ---
>>  src/service.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/service.c b/src/service.c
>> index 83e1c1a..dce5c05 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -170,6 +170,7 @@ int service_probe(struct btd_service *service)
>>
>>  void service_shutdown(struct btd_service *service)
>>  {
>> +       btd_service_disconnect(service);
>>         change_state(service, BTD_SERVICE_STATE_UNAVAILABLE, 0);
>>         service->profile->device_remove(service);
>>         service->device = NULL;
>> --
>> 1.8.1.4
>>
>> --
>> 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
>
> I'm not a big fan of this approach. The service should already be
> disconnected by the time it's shut down, so this additional transition
> should not be needed.

Im not following then, why do you call it shutdown then? If just to
free data this should have been service_remove or something like that.

> Having a look at the current code paths leading to service_shutdown(),
> one tricky case I can think of is the call to
> device_remove_profiles(), specially from search_cb(). I have my doubts
> whether a service shutdown makes sense if it's connected, but in case
> yes, I'd add the disconnection code to device_remove_profiles().

It depends on whether we want a clean disconnect or a link loss, IMO
if the driver gets a .disconnect callback it should disconnect
properly while .device_remove it basically frees private data but if
we are connected this most likely will cause an abrupt disconnect in
most drivers.

> Or do you have some other examples where the disconnection is not triggered?

All our shutdown related API, including g_io_channel_shutdown,
normally do disconnect as well.

> Adding one more side effect to service_shutdown() is IMO undesirable,
> where the transitional DISCONNECTED state would be artificially
> introduced. Think about an external profile being unregistered while
> connected devices exist: not only calling
> Profile.RequestDisconnection() doesn't make any sense, but a
> transition such as STATE_CONNECTED->STATE_UNAVAILABLE is probably what
> you want to observe. This can be different from a graceful
> disconnection, and a policy module could use this distinction to
> reconnect the service once the external profile gets registered.

While I agree that could be useful for tracking thinks like link loss
this would be initiated by the profile/service themselves somehow not
by device_remove code path that should never trigger any link loss
policy, it is a cleanup path btw so nothing should be pending after
that so your argument actually works against having this type of
transition from connected directly to unavailable.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[Mikel Astiz]

Hi Luiz,

On Mon, Jul 8, 2013 at 2:50 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Mon, Jul 8, 2013 at 2:46 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>> Hi Luiz,
>>
>> On Fri, Jul 5, 2013 at 4:03 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This ensures that the service is disconnected before setting the state
>>> to unavailable.
>>> ---
>>>  src/service.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/service.c b/src/service.c
>>> index 83e1c1a..dce5c05 100644
>>> --- a/src/service.c
>>> +++ b/src/service.c
>>> @@ -170,6 +170,7 @@ int service_probe(struct btd_service *service)
>>>
>>>  void service_shutdown(struct btd_service *service)
>>>  {
>>> +       btd_service_disconnect(service);
>>>         change_state(service, BTD_SERVICE_STATE_UNAVAILABLE, 0);
>>>         service->profile->device_remove(service);
>>>         service->device = NULL;
>>> --
>>> 1.8.1.4
>>>
>>> --
>>> 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
>>
>> I'm not a big fan of this approach. The service should already be
>> disconnected by the time it's shut down, so this additional transition
>> should not be needed.
>
> Im not following then, why do you call it shutdown then? If just to
> free data this should have been service_remove or something like that.

Perhaps the naming needs to be improved but it's not a simple free
either. It just makes the object unusable since it's reference-counted
and thus cannot be freed immediately.

>
>> Having a look at the current code paths leading to service_shutdown(),
>> one tricky case I can think of is the call to
>> device_remove_profiles(), specially from search_cb(). I have my doubts
>> whether a service shutdown makes sense if it's connected, but in case
>> yes, I'd add the disconnection code to device_remove_profiles().
>
> It depends on whether we want a clean disconnect or a link loss, IMO
> if the driver gets a .disconnect callback it should disconnect

That's clear.

> properly while .device_remove it basically frees private data but if
> we are connected this most likely will cause an abrupt disconnect in
> most drivers.

I agree that the .disconnect callback should be called before
.device_remove in most cases. I'm just questioning if this is the best
way to do it, since most of the code paths already handle the
disconnection explicitly in device.c. What's the exact case you're
fixing here?

Regarding external profiles, in particular, the .disconnect callback
would be useless during removal because Profile.RequestDisconnection()
shouldn't be called anyway, right? Unless the D-Bus API is designed in
a way that this method is expected to be called as a side effect of
ProfileManager1.UnregisterProfile(), which I'd personally find rather
strange.

>
>> Or do you have some other examples where the disconnection is not triggered?
>
> All our shutdown related API, including g_io_channel_shutdown,
> normally do disconnect as well.
>
>> Adding one more side effect to service_shutdown() is IMO undesirable,
>> where the transitional DISCONNECTED state would be artificially
>> introduced. Think about an external profile being unregistered while
>> connected devices exist: not only calling
>> Profile.RequestDisconnection() doesn't make any sense, but a
>> transition such as STATE_CONNECTED->STATE_UNAVAILABLE is probably what
>> you want to observe. This can be different from a graceful
>> disconnection, and a policy module could use this distinction to
>> reconnect the service once the external profile gets registered.
>
> While I agree that could be useful for tracking thinks like link loss
> this would be initiated by the profile/service themselves somehow not
> by device_remove code path that should never trigger any link loss
> policy, it is a cleanup path btw so nothing should be pending after
> that so your argument actually works against having this type of
> transition from connected directly to unavailable.

I was not referring to link-loss cases here but for example a crash in
oFono, which would presumably restart immediately afterwards. A policy
module could remember that the profile was disconnected due to a crash
and reconnect automatically.

Cheers,
Mikel

Re: [PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[Luiz Augusto von Dentz]

Hi Mikel,

On Mon, Jul 8, 2013 at 5:13 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>> Adding one more side effect to service_shutdown() is IMO undesirable,
>>> where the transitional DISCONNECTED state would be artificially
>>> introduced. Think about an external profile being unregistered while
>>> connected devices exist: not only calling
>>> Profile.RequestDisconnection() doesn't make any sense, but a
>>> transition such as STATE_CONNECTED->STATE_UNAVAILABLE is probably what
>>> you want to observe. This can be different from a graceful
>>> disconnection, and a policy module could use this distinction to
>>> reconnect the service once the external profile gets registered.
>>
>> While I agree that could be useful for tracking thinks like link loss
>> this would be initiated by the profile/service themselves somehow not
>> by device_remove code path that should never trigger any link loss
>> policy, it is a cleanup path btw so nothing should be pending after
>> that so your argument actually works against having this type of
>> transition from connected directly to unavailable.
>
> I was not referring to link-loss cases here but for example a crash in
> oFono, which would presumably restart immediately afterwards. A policy
> module could remember that the profile was disconnected due to a crash
> and reconnect automatically.

Well that can be considered a profile link loss, anyway what we are
discussing here is related to device_remove path and the effects of
having service_shutdown to do btd_service_disconnect, it seems that
when we do DeviceRemove we actually do disconnect before so that is
okay to no do it again on device_remove in the other hand if we are
quitting we perhaps don't really need to disconnect after all it could
be restarting for some reason like the device would reboot so it
doesn't necessarily need to disconnect to avoid reconnection strategy
to take place, so perhaps there is no real use of disconnecting after
all.

So I will do the following:

rename service_shutdown to service_remove which does btd_service_unref
internally similarly to what device_remove does so we keep similar
APIs for this matter, service_remove will not disconnect.

--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] core/service: Make sure service is disconnected before shutdown

[Luiz Augusto von Dentz]

Hi Mikel,

On Mon, Jul 8, 2013 at 5:52 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Mon, Jul 8, 2013 at 5:13 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>>> Adding one more side effect to service_shutdown() is IMO undesirable,
>>>> where the transitional DISCONNECTED state would be artificially
>>>> introduced. Think about an external profile being unregistered while
>>>> connected devices exist: not only calling
>>>> Profile.RequestDisconnection() doesn't make any sense, but a
>>>> transition such as STATE_CONNECTED->STATE_UNAVAILABLE is probably what
>>>> you want to observe. This can be different from a graceful
>>>> disconnection, and a policy module could use this distinction to
>>>> reconnect the service once the external profile gets registered.
>>>
>>> While I agree that could be useful for tracking thinks like link loss
>>> this would be initiated by the profile/service themselves somehow not
>>> by device_remove code path that should never trigger any link loss
>>> policy, it is a cleanup path btw so nothing should be pending after
>>> that so your argument actually works against having this type of
>>> transition from connected directly to unavailable.
>>
>> I was not referring to link-loss cases here but for example a crash in
>> oFono, which would presumably restart immediately afterwards. A policy
>> module could remember that the profile was disconnected due to a crash
>> and reconnect automatically.
>
> Well that can be considered a profile link loss, anyway what we are
> discussing here is related to device_remove path and the effects of
> having service_shutdown to do btd_service_disconnect, it seems that
> when we do DeviceRemove we actually do disconnect before so that is
> okay to no do it again on device_remove in the other hand if we are
> quitting we perhaps don't really need to disconnect after all it could
> be restarting for some reason like the device would reboot so it
> doesn't necessarily need to disconnect to avoid reconnection strategy
> to take place, so perhaps there is no real use of disconnecting after
> all.
>
> So I will do the following:
>
> rename service_shutdown to service_remove which does btd_service_unref
> internally similarly to what device_remove does so we keep similar
> APIs for this matter, service_remove will not disconnect.

One more detail, following this discussing Im removing
dev_disconnect_service from device_remove as we don't really want this
to happen on the cleanup case.


--
Luiz Augusto von Dentz