[PATCH BlueZ] attrib: Fix memory leak on watcher exit

[PATCH BlueZ] attrib: Fix memory leak on watcher exit

[Anderson Lizardo]

The "destroy" callback of g_dbus_add_disconnect_watch() is actually
never called, therefore the watcher data should be freed on
watcher_exit().
---
 attrib/client.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/attrib/client.c b/attrib/client.c
index 60cff01..e6cd3dc 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -212,6 +212,7 @@ static void watcher_exit(DBusConnection *conn, void *user_data)
 	DBG("%s watcher %s exited", gatt->path, watcher->name);
 
 	gatt->watchers = g_slist_remove(gatt->watchers, watcher);
+	watcher_free(watcher);
 }
 
 static int characteristic_set_value(struct characteristic *chr,
@@ -355,7 +356,7 @@ static DBusMessage *register_watcher(DBusConnection *conn,
 	watcher->gatt = gatt;
 	watcher->path = g_strdup(path);
 	watcher->id = g_dbus_add_disconnect_watch(conn, sender, watcher_exit,
-							watcher, watcher_free);
+								watcher, NULL);
 
 	if (gatt->attioid == 0)
 		gatt->attioid = btd_device_add_attio_callback(gatt->dev,
-- 
1.7.5.4

Re: [PATCH BlueZ] attrib: Fix memory leak on watcher exit

[Johan Hedberg]

Hi Lizardo,

On Thu, Mar 08, 2012, Anderson Lizardo wrote:
> The "destroy" callback of g_dbus_add_disconnect_watch() is actually
> never called, therefore the watcher data should be freed on
> watcher_exit().

Wouldn't it make more sense to fix GDBus so that the destroy callback
*is* called than to work around it like this?

Johan

Re: [PATCH BlueZ] attrib: Fix memory leak on watcher exit

[Anderson Lizardo]

Hi Johan,

On Thu, Mar 8, 2012 at 7:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Lizardo,
>
> On Thu, Mar 08, 2012, Anderson Lizardo wrote:
>> The "destroy" callback of g_dbus_add_disconnect_watch() is actually
>> never called, therefore the watcher data should be freed on
>> watcher_exit().
>
> Wouldn't it make more sense to fix GDBus so that the destroy callback
> *is* called than to work around it like this?

No other g_dbus_add_disconnect_watch() callers (besides this and LE
thermometer code which is also leaking and for which I have a patch as
well) use destroy callback. To me it seemed it was a known fact that
this parameter is not implemented in gdbus code, so that's why I went
this route.

If you prefer that I implement this destroy functionality, when do you
think it should be called? Only when the watch is removed?

Personally, I think it is better to simply remove this argument from
g_dbus_add_disconnect_watch(), if that is allowed. The caller usually
knows better when to "destroy" its own allocated resources (which may
explain why most users did not try to use this parameter?). What do
you think?

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

Re: [PATCH BlueZ] attrib: Fix memory leak on watcher exit

[Anderson Lizardo]

Hi Johan,

On Thu, Mar 8, 2012 at 10:32 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Johan,
>
> On Thu, Mar 8, 2012 at 7:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Hi Lizardo,
>>
>> On Thu, Mar 08, 2012, Anderson Lizardo wrote:
>>> The "destroy" callback of g_dbus_add_disconnect_watch() is actually
>>> never called, therefore the watcher data should be freed on
>>> watcher_exit().
>>
>> Wouldn't it make more sense to fix GDBus so that the destroy callback
>> *is* called than to work around it like this?
>
> No other g_dbus_add_disconnect_watch() callers (besides this and LE
> thermometer code which is also leaking and for which I have a patch as
> well) use destroy callback. To me it seemed it was a known fact that
> this parameter is not implemented in gdbus code, so that's why I went
> this route.
>
> If you prefer that I implement this destroy functionality, when do you
> think it should be called? Only when the watch is removed?
>
> Personally, I think it is better to simply remove this argument from
> g_dbus_add_disconnect_watch(), if that is allowed. The caller usually
> knows better when to "destroy" its own allocated resources (which may
> explain why most users did not try to use this parameter?). What do
> you think?

Any comments?

Maybe we can fix the memory leaks now and solve the gdbus API issue later?

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