[PATCH 1/2] device: don't auto-connect on disc-cb attio callback registration

[PATCH 1/2] device: don't auto-connect on disc-cb attio callback registration

[Arik Nemtsov]

If a device is already connected, don't auto-connect if we register
a disconnect-only attio callback. This will obviously fail.
---
 src/device.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/device.c b/src/device.c
index dfc8e59..b339ac1 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2942,10 +2942,15 @@ guint btd_device_add_attio_callback(struct btd_device *device,
 	attio->dcfunc = dcfunc;
 	attio->user_data = user_data;
 
-	if (device->attrib && cfunc) {
-		device->attios_offline = g_slist_append(device->attios_offline,
-									attio);
-		g_idle_add(notify_attios, device);
+	if (device->attrib) {
+		if (cfunc) {
+			device->attios_offline =
+				g_slist_append(device->attios_offline, attio);
+
+			g_idle_add(notify_attios, device);
+		} else {
+			device->attios = g_slist_append(device->attios, attio);
+		}
 	} else {
 		device->auto_id = g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
 						att_connect, device,
-- 
1.7.5.4

[PATCH 2/2] Revert "core: Add provision for terminating an ATT connection"

[Arik Nemtsov]

This reverts commit f89a77478af78d41c80ab7605662382b9e4e1c36.

This is not needed and actually introduces a bug. When the "Disconnect"
API of device is called device->attrib is unref-ed via a watch set on
G_IO_HUP. The channel is shutdown when the last reference is removed.

The code introduced here shuts down the channel and prevents the watch
from getting called. This means we leak a reference to device->attrib.
This can cause a number of bad things. For example, if the device is
temporary, it will never be freed, and we won't be able to pair to it
again.
---
 src/device.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/src/device.c b/src/device.c
index b339ac1..0a1de7c 100644
--- a/src/device.c
+++ b/src/device.c
@@ -821,14 +821,6 @@ void device_request_disconnect(struct btd_device *device, DBusMessage *msg)
 		browse_request_cancel(device->browse);
 	}
 
-	if (device->attrib) {
-		GIOChannel *io = g_attrib_get_channel(device->attrib);
-		if (io) {
-			g_io_channel_shutdown(io, FALSE, NULL);
-			g_io_channel_unref(io);
-		}
-	}
-
 	if (msg)
 		device->disconnects = g_slist_append(device->disconnects,
 						dbus_message_ref(msg));
-- 
1.7.5.4

Re: [PATCH 2/2] Revert "core: Add provision for terminating an ATT connection"

[Arik Nemtsov]

On Thu, Mar 8, 2012 at 15:44, Arik Nemtsov <arik@wizery.com> wrote:
> This reverts commit f89a77478af78d41c80ab7605662382b9e4e1c36.

The actual commit Id was c4a8ba6d82f1b5c7f14c2abc01242925c60b468d.
It's wrong in the original patch since I reverted it in a private
tree.

>
> This is not needed and actually introduces a bug. When the "Disconnect"
> API of device is called device->attrib is unref-ed via a watch set on
> G_IO_HUP. The channel is shutdown when the last reference is removed.
>
> The code introduced here shuts down the channel and prevents the watch
> from getting called. This means we leak a reference to device->attrib.
> This can cause a number of bad things. For example, if the device is
> temporary, it will never be freed, and we won't be able to pair to it
> again.
> ---
>  src/device.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index b339ac1..0a1de7c 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -821,14 +821,6 @@ void device_request_disconnect(struct btd_device *device, DBusMessage *msg)
>                browse_request_cancel(device->browse);
>        }
>
> -       if (device->attrib) {
> -               GIOChannel *io = g_attrib_get_channel(device->attrib);
> -               if (io) {
> -                       g_io_channel_shutdown(io, FALSE, NULL);
> -                       g_io_channel_unref(io);
> -               }
> -       }
> -
>        if (msg)
>                device->disconnects = g_slist_append(device->disconnects,
>                                                dbus_message_ref(msg));
> --
> 1.7.5.4
>

Re: [PATCH 2/2] Revert "core: Add provision for terminating an ATT connection"

[Anderson Lizardo]

Hi Arik,

On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <arik@wizery.com> wrote:
> This reverts commit f89a77478af78d41c80ab7605662382b9e4e1c36.
>
> This is not needed and actually introduces a bug. When the "Disconnect"
> API of device is called device->attrib is unref-ed via a watch set on
> G_IO_HUP. The channel is shutdown when the last reference is removed.
>
> The code introduced here shuts down the channel and prevents the watch
> from getting called. This means we leak a reference to device->attrib.
> This can cause a number of bad things. For example, if the device is
> temporary, it will never be freed, and we won't be able to pair to it
> again.
> ---
>  src/device.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)

While I agree with your explanation, we still need a way to for link
disconnection. There is a Disconnect command on mgmt API that may be
used for this (I suppose there is a similar one for hciops). This way
the HUP watches will be called because disconnection will be initiated
by the kernel.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 2/2] Revert "core: Add provision for terminating an ATT connection"

[Arik Nemtsov]

On Wed, Mar 14, 2012 at 14:08, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Arik,
>
> On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <arik@wizery.com> wrote:
>> This reverts commit f89a77478af78d41c80ab7605662382b9e4e1c36.
>>
>> This is not needed and actually introduces a bug. When the "Disconnect"
>> API of device is called device->attrib is unref-ed via a watch set on
>> G_IO_HUP. The channel is shutdown when the last reference is removed.
>>
>> The code introduced here shuts down the channel and prevents the watch
>> from getting called. This means we leak a reference to device->attrib.
>> This can cause a number of bad things. For example, if the device is
>> temporary, it will never be freed, and we won't be able to pair to it
>> again.
>> ---
>>  src/device.c |    8 --------
>>  1 files changed, 0 insertions(+), 8 deletions(-)
>
> While I agree with your explanation, we still need a way to for link
> disconnection. There is a Disconnect command on mgmt API that may be
> used for this (I suppose there is a similar one for hciops). This way
> the HUP watches will be called because disconnection will be initiated
> by the kernel.
>

The current disconnect command (exposed via D-Bus as
Device.Disconnect()) works just fine for me.

Arik

Re: [PATCH 2/2] Revert "core: Add provision for terminating an ATT connection"

[Anderson Lizardo]

Hi Arik,

On Wed, Mar 14, 2012 at 8:11 AM, Arik Nemtsov <arik@wizery.com> wrote:
> The current disconnect command (exposed via D-Bus as
> Device.Disconnect()) works just fine for me.

This might have been fixed by other means in this case then.

And I see there is a btd_adapter_disconnect_device() (from
do_disconnect()) being called there, so what I proposed is what
actually happens :)

In this case, I think the patch is okay, we can revisit this if the
revert causes problems.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/2] device: don't auto-connect on disc-cb attio callback registration

[Anderson Lizardo]

Hi Arik,

On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <arik@wizery.com> wrote:
> If a device is already connected, don't auto-connect if we register
> a disconnect-only attio callback. This will obviously fail.
> ---
>  src/device.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)

I can't see why this patch is necessary. attio_connected() seems to
properly check that device->cfunc is set before calling it.

Can you explain why you need it?

>
> diff --git a/src/device.c b/src/device.c
> index dfc8e59..b339ac1 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2942,10 +2942,15 @@ guint btd_device_add_attio_callback(struct btd_device *device,
>        attio->dcfunc = dcfunc;
>        attio->user_data = user_data;
>
> -       if (device->attrib && cfunc) {
> -               device->attios_offline = g_slist_append(device->attios_offline,
> -                                                                       attio);
> -               g_idle_add(notify_attios, device);
> +       if (device->attrib) {
> +               if (cfunc) {
> +                       device->attios_offline =
> +                               g_slist_append(device->attios_offline, attio);
> +
> +                       g_idle_add(notify_attios, device);
> +               } else {
> +                       device->attios = g_slist_append(device->attios, attio);
> +               }
>        } else {
>                device->auto_id = g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
>                                                att_connect, device,
> --
> 1.7.5.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


Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/2] device: don't auto-connect on disc-cb attio callback registration

[Arik Nemtsov]

On Wed, Mar 14, 2012 at 14:03, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Arik,
>
> On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <arik@wizery.com> wrote:
>> If a device is already connected, don't auto-connect if we register
>> a disconnect-only attio callback. This will obviously fail.
>> ---
>>  src/device.c |   13 +++++++++----
>>  1 files changed, 9 insertions(+), 4 deletions(-)
>
> I can't see why this patch is necessary. attio_connected() seems to
> properly check that device->cfunc is set before calling it.
>
> Can you explain why you need it?

Before this patch, if we register a disconnect-only attio callback
(cfunc is NULL) when the device is connected, it will cause this line
to be called:

		device->auto_id = g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
						att_connect, device,
						att_connect_dispatched);

I guess the code wasn't really tested with a disconnect-only attio
callback. I think I added the first such callback in the proximity
reporter profiles.

Arik

Re: [PATCH 1/2] device: don't auto-connect on disc-cb attio callback registration

[Anderson Lizardo]

Hi Arik,

On Wed, Mar 14, 2012 at 8:09 AM, Arik Nemtsov <arik@wizery.com> wrote:
>> Can you explain why you need it?
>
> Before this patch, if we register a disconnect-only attio callback
> (cfunc is NULL) when the device is connected, it will cause this line
> to be called:
>
>                device->auto_id = g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
>                                                att_connect, device,
>                                                att_connect_dispatched);
>
> I guess the code wasn't really tested with a disconnect-only attio
> callback. I think I added the first such callback in the proximity
> reporter profiles.

Ok, so the problem is actually the " && cfunc" condition which makes
the wrong branch to be followed if only a disconnect callback is
registered.

So the patch makes sense and looks good.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/2] device: don't auto-connect on disc-cb attio callback registration

[Johan Hedberg]

Hi Arik,

On Thu, Mar 08, 2012, Arik Nemtsov wrote:
> If a device is already connected, don't auto-connect if we register
> a disconnect-only attio callback. This will obviously fail.
> ---
>  src/device.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)

Both patches have been applied. Thanks.

Johan