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
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
Hi Arik,
On Wed, Mar 14, 2012 at 8:09 AM, Arik Nemtsov <[email protected]> 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
Hi Arik,
On Wed, Mar 14, 2012 at 8:11 AM, Arik Nemtsov <[email protected]> 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
On Wed, Mar 14, 2012 at 14:08, Anderson Lizardo
<[email protected]> wrote:
> Hi Arik,
>
> On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <[email protected]> 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
On Wed, Mar 14, 2012 at 14:03, Anderson Lizardo
<[email protected]> wrote:
> Hi Arik,
>
> On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <[email protected]> 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
Hi Arik,
On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <[email protected]> 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
Hi Arik,
On Thu, Mar 8, 2012 at 9:44 AM, Arik Nemtsov <[email protected]> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
On Thu, Mar 8, 2012 at 15:44, Arik Nemtsov <[email protected]> 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
>
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