2010-02-16 14:59:42

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH] Fix signal watch when a service name is given

Hi,

This should fix g_dbus_add_signal_watch when a service like org.bluez is given.

--
Luiz Augusto von Dentz
Computer Engineer


Attachments:
0002-Fix-signal-watch-when-a-service-name-is-given.patch (9.12 kB)

2010-02-21 20:41:59

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi,

On Wed, Feb 17, 2010 at 6:08 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marcel,
>
> On Wed, Feb 17, 2010 at 12:00 PM, Marcel Holtmann <[email protected]> wrote:
>> Hi Luiz,
>>
>>> @Marcel: So before you apply this you should really apply Vinicius
>>> patch to the rest of projects: bluez, ofono, connman...
>>
>> has been applied to all projects now. Please resend your other patch
>> based on the latest tree.
>
> Resending the patch and also add another one to make sure we cancel
> any pending operation during watch removal.

Resending with the latests fixes to make hfp ofono plugin to work properly.


--
Luiz Augusto von Dentz
Computer Engineer


Attachments:
0001-Do-not-automatically-remove-watches-for-service-name.patch (1.18 kB)
0002-Fix-signal-watch-when-a-service-name-is-given.patch (9.17 kB)
0003-Fix-calling-watch-callbacks-after-it-has-been-remove.patch (4.90 kB)
Download all attachments

2010-02-17 16:08:57

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi Marcel,

On Wed, Feb 17, 2010 at 12:00 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Luiz,
>
>> @Marcel: So before you apply this you should really apply Vinicius
>> patch to the rest of projects: bluez, ofono, connman...
>
> has been applied to all projects now. Please resend your other patch
> based on the latest tree.

Resending the patch and also add another one to make sure we cancel
any pending operation during watch removal.

--
Luiz Augusto von Dentz
Computer Engineer


Attachments:
0001-Fix-signal-watch-when-a-service-name-is-given.patch (9.17 kB)
0002-Fix-calling-watch-callbacks-after-it-has-been-remove.patch (4.90 kB)
Download all attachments

2010-02-17 10:00:25

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi Luiz,

> @Marcel: So before you apply this you should really apply Vinicius
> patch to the rest of projects: bluez, ofono, connman...

has been applied to all projects now. Please resend your other patch
based on the latest tree.

Regards

Marcel



2010-02-17 09:06:43

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi Vinicius,

On Wed, Feb 17, 2010 at 2:34 AM, Vinicius Gomes
<[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Feb 16, 2010 at 12:58 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Marcel,
>>
>> On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <[email protected]> w=
rote:
>>> Hi Luiz,
>>>
>>>> > This should fix g_dbus_add_signal_watch when a service like org.blue=
z is given.
>>>>
>>>>
>>>> Updating since the last one was broken.
>>>
>>> I also think that in check_service() we actually have a missing call to
>>> dbus_pending_unref(). That would cause a memory leak.
>>
>> There is a call to =A0dbus_pending_call_unref on check_service after
>> dbus_pending_call_set_notify, which I took a look and seems correct
>> but we have another problem there since we never cancel the pending
>> call if the filter is unregister. Anyway there is a leak on
>> service_reply where I don't call dbus_message_unref, so I will come up
>> with another update soon.
>>
>
> This is fixed on obexd, but as far as I could see it is not (yet?)
> applied on the other users
> of gdbus.

Hmm, that why I see it, I used obexd to test this not bluetoothd as
(probably) Marcel did, anyway we still need to store and cancel the
pending call if we really want the watches to be truly cancelable.

> And, from what I tested the greater problem is that the timeout of the
> pending call would still
> trigger. This would cause libdbus to terminate the application, if
> libdbus was built with debug enabled,
> because one assert would fail (the message is "trying to remove an
> nonexistent timeout" or
> =A0something like it). The timeout still triggering is the reason we
> don't see the pending call leaking.

@Marcel: So before you apply this you should really apply Vinicius
patch to the rest of projects: bluez, ofono, connman...

--=20
Luiz Augusto von Dentz
Computer Engineer

2010-02-17 00:34:47

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi Luiz,

On Tue, Feb 16, 2010 at 12:58 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marcel,
>
> On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <[email protected]> wr=
ote:
>> Hi Luiz,
>>
>>> > This should fix g_dbus_add_signal_watch when a service like org.bluez=
is given.
>>>
>>>
>>> Updating since the last one was broken.
>>
>> I also think that in check_service() we actually have a missing call to
>> dbus_pending_unref(). That would cause a memory leak.
>
> There is a call to =C2=A0dbus_pending_call_unref on check_service after
> dbus_pending_call_set_notify, which I took a look and seems correct
> but we have another problem there since we never cancel the pending
> call if the filter is unregister. Anyway there is a leak on
> service_reply where I don't call dbus_message_unref, so I will come up
> with another update soon.
>

This is fixed on obexd, but as far as I could see it is not (yet?)
applied on the other users
of gdbus.

And, from what I tested the greater problem is that the timeout of the
pending call would still
trigger. This would cause libdbus to terminate the application, if
libdbus was built with debug enabled,
because one assert would fail (the message is "trying to remove an
nonexistent timeout" or
something like it). The timeout still triggering is the reason we
don't see the pending call leaking.

> --
> Luiz Augusto von Dentz
> Computer Engineer
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>


Cheers,
--=20
Vinicius Gomes
INdT - Instituto Nokia de Tecnologia

2010-02-16 16:25:36

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi,

On Tue, Feb 16, 2010 at 5:58 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Marcel,
>
> On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <[email protected]> wrote:
>> Hi Luiz,
>>
>>> > This should fix g_dbus_add_signal_watch when a service like org.bluez is given.
>>>
>>>
>>> Updating since the last one was broken.
>>
>> I also think that in check_service() we actually have a missing call to
>> dbus_pending_unref(). That would cause a memory leak.
>
> There is a call to ?dbus_pending_call_unref on check_service after
> dbus_pending_call_set_notify, which I took a look and seems correct
> but we have another problem there since we never cancel the pending
> call if the filter is unregister. Anyway there is a leak on
> service_reply where I don't call dbus_message_unref, so I will come up
> with another update soon.

Now with the memory leaks fixed.

--
Luiz Augusto von Dentz
Computer Engineer


Attachments:
0001-Fix-signal-watch-when-a-service-name-is-given.patch (9.03 kB)

2010-02-16 15:58:04

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi Marcel,

On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Luiz,
>
>> > This should fix g_dbus_add_signal_watch when a service like org.bluez is given.
>>
>>
>> Updating since the last one was broken.
>
> I also think that in check_service() we actually have a missing call to
> dbus_pending_unref(). That would cause a memory leak.

There is a call to dbus_pending_call_unref on check_service after
dbus_pending_call_set_notify, which I took a look and seems correct
but we have another problem there since we never cancel the pending
call if the filter is unregister. Anyway there is a leak on
service_reply where I don't call dbus_message_unref, so I will come up
with another update soon.

--
Luiz Augusto von Dentz
Computer Engineer

2010-02-16 15:37:08

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi Luiz,

> > This should fix g_dbus_add_signal_watch when a service like org.bluez is given.
>
>
> Updating since the last one was broken.

I also think that in check_service() we actually have a missing call to
dbus_pending_unref(). That would cause a memory leak.

Regards

Marcel



2010-02-16 15:15:36

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Fix signal watch when a service name is given

Hi,

On Tue, Feb 16, 2010 at 4:59 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi,
>
> This should fix g_dbus_add_signal_watch when a service like org.bluez is given.


Updating since the last one was broken.


--
Luiz Augusto von Dentz
Computer Engineer


Attachments:
0002-Fix-signal-watch-when-a-service-name-is-given.patch (9.12 kB)