The discovery_list list has the list of current discovery clients and is
removed on adapter_stop (for example due a "power off" command). The
g_slist_free_full will call discovery_free on every element of the list
and remove the nodes of the list, but discovery_destroy (called by
discovery_free) will not only free the element, but also remove it from
the list. This causes the list node to be freed twice, once by
g_slist_free_full and once by g_slist_remove.
This fix calls successively discovery_free and lets it remove the list one
by one.
---
src/adapter.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index e553626..ac322de 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4272,8 +4272,11 @@ static void adapter_stop(struct btd_adapter *adapter)
cancel_passive_scanning(adapter);
if (adapter->discovery_list) {
- g_slist_free_full(adapter->discovery_list, discovery_free);
- adapter->discovery_list = NULL;
+ while (adapter->discovery_list) {
+ struct discovery_client *client =
+ adapter->discovery_list->data;
+ discovery_free(client);
+ }
adapter->discovering = false;
}
--
1.8.1.3
Hi Alex,
On Fri, Mar 29, 2013, Alex Deymo wrote:
> The discovery_list list has the list of current discovery clients and is
> removed on adapter_stop (for example due a "power off" command). The
> g_slist_free_full will call discovery_free on every element of the list
> and remove the nodes of the list, but discovery_destroy (called by
> discovery_free) will not only free the element, but also remove it from
> the list. This causes the list node to be freed twice, once by
> g_slist_free_full and once by g_slist_remove.
>
> This fix calls successively discovery_free and lets it remove the list one
> by one.
> ---
> src/adapter.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index e553626..ac322de 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4272,8 +4272,11 @@ static void adapter_stop(struct btd_adapter *adapter)
> cancel_passive_scanning(adapter);
>
> if (adapter->discovery_list) {
> - g_slist_free_full(adapter->discovery_list, discovery_free);
> - adapter->discovery_list = NULL;
> + while (adapter->discovery_list) {
> + struct discovery_client *client =
> + adapter->discovery_list->data;
> + discovery_free(client);
> + }
>
> adapter->discovering = false;
> }
Good catch, but you could go even further and remove the discovery_free
function too since its only purpose was to match the expected type for
g_slist_free_full (which you no-longer use). Please add a code comment
though clarifying that g_dbus_remove_watch takes care of the freeing and
list element removal.
Also, I'd go ahead and remove one level of nesting here since the
if-statement before the while loop is a bit redundant (the setting of
discovering to false can be unconditional afterwards).
Johan
Hi Alex,
On Mon, Apr 01, 2013, Alex Deymo wrote:
> The discovery_list list has the list of current discovery clients and is
> removed on adapter_stop (for example due a "power off" command). The
> g_slist_free_full will call discovery_free on every element of the list
> and remove the nodes of the list, but discovery_destroy (called by
> discovery_free) will not only free the element, but also remove it from
> the list. This causes the list node to be freed twice, once by
> g_slist_free_full and once by g_slist_remove.
>
> This fix calls successively discovery_destroy and lets it remove the list's
> elements one by one.
> ---
> src/adapter.c | 20 ++++++--------------
> 1 file changed, 6 insertions(+), 14 deletions(-)
Applied (after a couple minor coding style changes). Thanks.
Johan
The discovery_list list has the list of current discovery clients and is
removed on adapter_stop (for example due a "power off" command). The
g_slist_free_full will call discovery_free on every element of the list
and remove the nodes of the list, but discovery_destroy (called by
discovery_free) will not only free the element, but also remove it from
the list. This causes the list node to be freed twice, once by
g_slist_free_full and once by g_slist_remove.
This fix calls successively discovery_destroy and lets it remove the list's
elements one by one.
---
src/adapter.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index e553626..9a3bc54 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1540,15 +1540,6 @@ static gboolean remove_temp_devices(gpointer user_data)
return FALSE;
}
-static void discovery_free(void *data)
-{
- struct discovery_client *client = data;
-
- DBG("owner %s", client->owner);
-
- g_dbus_remove_watch(dbus_conn, client->watch);
-}
-
static void discovery_destroy(void *user_data)
{
struct discovery_client *client = user_data;
@@ -4271,12 +4262,13 @@ static void adapter_stop(struct btd_adapter *adapter)
cancel_passive_scanning(adapter);
- if (adapter->discovery_list) {
- g_slist_free_full(adapter->discovery_list, discovery_free);
- adapter->discovery_list = NULL;
-
- adapter->discovering = false;
+ while (adapter->discovery_list) {
+ struct discovery_client *client = adapter->discovery_list->data;
+ /* g_dbus_remove_watch will remove the client from the adapter's
+ * list and free it using the discovery_destroy function. */
+ g_dbus_remove_watch(dbus_conn, client->watch);
}
+ adapter->discovering = false;
while (adapter->connections) {
struct btd_device *device = adapter->connections->data;
--
1.8.1.3