If there's a signal watch that's also watching for name
(data->name_watch) currently we are trying to remove the message_filter
twice since we may have the following call chain:
filter_data_remove_callback()
filter_data_free()
g_dbus_remove_watch()
filter_data_remove_callback()
filter_data_free()
dbus_connection_remove_filter()
dbus_connection_remove_filter()
Because of this we can't currently watch for signals passing the bus
name. After this patch we don't have this issue anymore.
We fix this by checking if we are the last filter_data before calling
filter_data_free and thus not calling dbus_connection_remove_filter()
twice.
---
gdbus/watch.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 9a716b0..6fed264 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -350,6 +350,7 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
struct filter_callback *cb)
{
DBusConnection *connection;
+ struct filter_data *data_next;
data->callbacks = g_slist_remove(data->callbacks, cb);
data->processed = g_slist_remove(data->processed, cb);
@@ -376,12 +377,17 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
connection = dbus_connection_ref(data->connection);
listeners = g_slist_remove(listeners, data);
+
+ /*
+ * filter_data_free() may remove the latest data - we need to check
+ * before it runs if it's our duty to remove the filter
+ */
+ data_next = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL);
filter_data_free(data);
/* Remove filter if there are no listeners left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
- NULL);
- if (data == NULL)
+ if (data_next == NULL)
dbus_connection_remove_filter(connection, message_filter,
NULL);
--
1.7.11
Hi Lucas,
On Mon, Jun 25, 2012, Lucas De Marchi wrote:
> If there's a signal watch that's also watching for name
> (data->name_watch) currently we are trying to remove the message_filter
> twice since we may have the following call chain:
>
> filter_data_remove_callback()
> filter_data_free()
> g_dbus_remove_watch()
> filter_data_remove_callback()
> filter_data_free()
> dbus_connection_remove_filter()
> dbus_connection_remove_filter()
>
> Because of this we can't currently watch for signals passing the bus
> name. After this patch we don't have this issue anymore.
>
> We fix it by removing the filter before calling filter_data_free() if we
> are the last filter_data and thus avoid calling
> dbus_connection_remove_filter() twice.
> ---
> gdbus/watch.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
I've applied this first patch to bluez.git and obexd.git. I ignored the
other too since the telephony drivers and the maemo6 plugin are just
about to be removed from the tree in preparation for BlueZ 5.
Johan
Hi Lucas,
On Mon, Jun 25, 2012 at 6:44 PM, Lucas De Marchi
<[email protected]> wrote:
> If there's a signal watch that's also watching for name
> (data->name_watch) currently we are trying to remove the message_filter
> twice since we may have the following call chain:
>
> filter_data_remove_callback()
> ?filter_data_free()
> ? ?g_dbus_remove_watch()
> ? ? ?filter_data_remove_callback()
> ? ? ? ?filter_data_free()
> ? ? ? ?dbus_connection_remove_filter()
> ?dbus_connection_remove_filter()
>
> Because of this we can't currently watch for signals passing the bus
> name. After this patch we don't have this issue anymore.
>
> We fix it by removing the filter before calling filter_data_free() if we
> are the last filter_data and thus avoid calling
> dbus_connection_remove_filter() twice.
> ---
> ?gdbus/watch.c | 17 ++++++++---------
> ?1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index 9a716b0..d749176 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -376,15 +376,14 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
>
> ? ? ? ?connection = dbus_connection_ref(data->connection);
> ? ? ? ?listeners = g_slist_remove(listeners, data);
> - ? ? ? filter_data_free(data);
>
> ? ? ? ?/* Remove filter if there are no listeners left for the connection */
> - ? ? ? data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> - ? ? ? if (data == NULL)
> + ? ? ? if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL) == NULL)
> ? ? ? ? ? ? ? ?dbus_connection_remove_filter(connection, message_filter,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL);
>
> + ? ? ? filter_data_free(data);
> ? ? ? ?dbus_connection_unref(connection);
>
> ? ? ? ?return TRUE;
> @@ -537,15 +536,15 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
> ? ? ? ?remove_match(data);
>
> ? ? ? ?listeners = g_slist_remove(listeners, data);
> - ? ? ? filter_data_free(data);
>
> - ? ? ? /* Remove filter if there no listener left for the connection */
> - ? ? ? data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> - ? ? ? if (data == NULL)
> + ? ? ? /* Remove filter if there are no listeners left for the connection */
> + ? ? ? if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL) == NULL)
> ? ? ? ? ? ? ? ?dbus_connection_remove_filter(connection, message_filter,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL);
>
> + ? ? ? filter_data_free(data);
> +
> ? ? ? ?return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> ?}
>
> --
> 1.7.11
Ack.
--
Luiz Augusto von Dentz
Hi Luiz,
On Mon, Jun 25, 2012 at 12:55 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Lucas,
>
> On Mon, Jun 25, 2012 at 6:44 PM, Lucas De Marchi
> <[email protected]> wrote:
>> We should listen for signals only on MCE bus name.
>> ---
>> ?plugins/maemo6.c | 7 ++++---
>> ?1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/plugins/maemo6.c b/plugins/maemo6.c
>> index 4819af4..a8c9bed 100644
>> --- a/plugins/maemo6.c
>> +++ b/plugins/maemo6.c
>> @@ -224,12 +224,13 @@ static int mce_probe(struct btd_adapter *adapter)
>>
>> ? ? ? ?DBG("path %s", adapter_get_path(adapter));
>>
>> - ? ? ? watch_id = g_dbus_add_signal_watch(conn, NULL, MCE_SIGNAL_PATH,
>> + ? ? ? watch_id = g_dbus_add_signal_watch(conn, MCE_SERVICE, MCE_SIGNAL_PATH,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?MCE_SIGNAL_IF, MCE_RADIO_STATES_SIG,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mce_signal_callback, adapter, NULL);
>>
>> - ? ? ? tklock_watch_id = g_dbus_add_signal_watch(conn, NULL, MCE_SIGNAL_PATH,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MCE_SIGNAL_IF, MCE_TKLOCK_MODE_SIG,
>> + ? ? ? tklock_watch_id = g_dbus_add_signal_watch(conn, MCE_SERVICE,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MCE_SIGNAL_PATH, MCE_SIGNAL_IF,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MCE_TKLOCK_MODE_SIG,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mce_tklock_mode_cb, adapter, NULL);
>>
>> ? ? ? ?btd_adapter_register_powered_callback(adapter, adapter_powered);
>> --
>> 1.7.11
>
> Don't bother with telephony drivers, they are about to be removed.
I only fixed all possible users in bluez. It's ok to just ignore this patch.
Lucas De Marchi
Hi Lucas,
On Mon, Jun 25, 2012 at 6:44 PM, Lucas De Marchi
<[email protected]> wrote:
> We should listen for signals only on MCE bus name.
> ---
> ?plugins/maemo6.c | 7 ++++---
> ?1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/plugins/maemo6.c b/plugins/maemo6.c
> index 4819af4..a8c9bed 100644
> --- a/plugins/maemo6.c
> +++ b/plugins/maemo6.c
> @@ -224,12 +224,13 @@ static int mce_probe(struct btd_adapter *adapter)
>
> ? ? ? ?DBG("path %s", adapter_get_path(adapter));
>
> - ? ? ? watch_id = g_dbus_add_signal_watch(conn, NULL, MCE_SIGNAL_PATH,
> + ? ? ? watch_id = g_dbus_add_signal_watch(conn, MCE_SERVICE, MCE_SIGNAL_PATH,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?MCE_SIGNAL_IF, MCE_RADIO_STATES_SIG,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mce_signal_callback, adapter, NULL);
>
> - ? ? ? tklock_watch_id = g_dbus_add_signal_watch(conn, NULL, MCE_SIGNAL_PATH,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MCE_SIGNAL_IF, MCE_TKLOCK_MODE_SIG,
> + ? ? ? tklock_watch_id = g_dbus_add_signal_watch(conn, MCE_SERVICE,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MCE_SIGNAL_PATH, MCE_SIGNAL_IF,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MCE_TKLOCK_MODE_SIG,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mce_tklock_mode_cb, adapter, NULL);
>
> ? ? ? ?btd_adapter_register_powered_callback(adapter, adapter_powered);
> --
> 1.7.11
Don't bother with telephony drivers, they are about to be removed.
--
Luiz Augusto von Dentz
We should listen for signals only on MCE bus name.
---
plugins/maemo6.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/plugins/maemo6.c b/plugins/maemo6.c
index 4819af4..a8c9bed 100644
--- a/plugins/maemo6.c
+++ b/plugins/maemo6.c
@@ -224,12 +224,13 @@ static int mce_probe(struct btd_adapter *adapter)
DBG("path %s", adapter_get_path(adapter));
- watch_id = g_dbus_add_signal_watch(conn, NULL, MCE_SIGNAL_PATH,
+ watch_id = g_dbus_add_signal_watch(conn, MCE_SERVICE, MCE_SIGNAL_PATH,
MCE_SIGNAL_IF, MCE_RADIO_STATES_SIG,
mce_signal_callback, adapter, NULL);
- tklock_watch_id = g_dbus_add_signal_watch(conn, NULL, MCE_SIGNAL_PATH,
- MCE_SIGNAL_IF, MCE_TKLOCK_MODE_SIG,
+ tklock_watch_id = g_dbus_add_signal_watch(conn, MCE_SERVICE,
+ MCE_SIGNAL_PATH, MCE_SIGNAL_IF,
+ MCE_TKLOCK_MODE_SIG,
mce_tklock_mode_cb, adapter, NULL);
btd_adapter_register_powered_callback(adapter, adapter_powered);
--
1.7.11
We should listen for signals only on oFono bus name.
---
audio/telephony-ofono.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/audio/telephony-ofono.c b/audio/telephony-ofono.c
index 961fedd..67eb87b 100644
--- a/audio/telephony-ofono.c
+++ b/audio/telephony-ofono.c
@@ -742,7 +742,7 @@ static struct voice_call *call_new(const char *path, DBusMessageIter *properties
vc = g_new0(struct voice_call, 1);
vc->obj_path = g_strdup(path);
- vc->watch = g_dbus_add_signal_watch(connection, NULL, path,
+ vc->watch = g_dbus_add_signal_watch(connection, OFONO_BUS_NAME, path,
OFONO_VC_INTERFACE, "PropertyChanged",
handle_vc_property_changed, vc, NULL);
--
1.7.11
If there's a signal watch that's also watching for name
(data->name_watch) currently we are trying to remove the message_filter
twice since we may have the following call chain:
filter_data_remove_callback()
filter_data_free()
g_dbus_remove_watch()
filter_data_remove_callback()
filter_data_free()
dbus_connection_remove_filter()
dbus_connection_remove_filter()
Because of this we can't currently watch for signals passing the bus
name. After this patch we don't have this issue anymore.
We fix it by removing the filter before calling filter_data_free() if we
are the last filter_data and thus avoid calling
dbus_connection_remove_filter() twice.
---
gdbus/watch.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 9a716b0..d749176 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -376,15 +376,14 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
connection = dbus_connection_ref(data->connection);
listeners = g_slist_remove(listeners, data);
- filter_data_free(data);
/* Remove filter if there are no listeners left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
- NULL);
- if (data == NULL)
+ if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL) == NULL)
dbus_connection_remove_filter(connection, message_filter,
NULL);
+ filter_data_free(data);
dbus_connection_unref(connection);
return TRUE;
@@ -537,15 +536,15 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
remove_match(data);
listeners = g_slist_remove(listeners, data);
- filter_data_free(data);
- /* Remove filter if there no listener left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
- NULL);
- if (data == NULL)
+ /* Remove filter if there are no listeners left for the connection */
+ if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL) == NULL)
dbus_connection_remove_filter(connection, message_filter,
NULL);
+ filter_data_free(data);
+
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
--
1.7.11
Hi Lucas,
On Mon, Jun 25, 2012 at 4:26 PM, Lucas De Marchi
<[email protected]> wrote:
>
> Probably... this is the stock libdbus from Archlinux. I also found
> interest that all possible users of g_dbus_add_signal_watch() with a
> bus name were passing a NULL, which is clearly wrong. That made me
> think it was like this to workaround this bug.
Ive changed in a few places but I guess nobody really care when
g_dbus_add_signal_watch was introduced.
>
>>
>> Anyway I couldn't reproduce by changing ofono with stock libdbus from
>> FC 17, although I see the problem just by looking at the code, but
>> there other places where this need to be checked as well and the code
>> could be simplified:
>>
>> diff --git a/gdbus/watch.c b/gdbus/watch.c
>> index 9a716b0..d749176 100644
>> --- a/gdbus/watch.c
>> +++ b/gdbus/watch.c
>> @@ -376,15 +376,14 @@ static gboolean
>> filter_data_remove_callback(struct filter_data *data,
>>
>> ? ? ? ?connection = dbus_connection_ref(data->connection);
>> ? ? ? ?listeners = g_slist_remove(listeners, data);
>> - ? ? ? filter_data_free(data);
>>
>> ? ? ? ?/* Remove filter if there are no listeners left for the connection */
>> - ? ? ? data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
>> - ? ? ? if (data == NULL)
>> + ? ? ? if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL) == NULL)
>> ? ? ? ? ? ? ? ?dbus_connection_remove_filter(connection, message_filter,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL);
>>
>> + ? ? ? filter_data_free(data);
>> ? ? ? ?dbus_connection_unref(connection);
>>
>> ? ? ? ?return TRUE;
>
> ahn... right. We already removed data from listeners list so we can
> call dbus_connection_remove_filter() before filter_data_free().
Please just send the patch with this changes, you probably have look
much more in details than I did.
--
Luiz Augusto von Dentz
On Mon, Jun 25, 2012 at 5:51 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Lucas,
>
> On Sat, Jun 23, 2012 at 5:02 PM, Lucas De Marchi
> <[email protected]> wrote:
>> If there's a signal watch that's also watching for name
>> (data->name_watch) currently we are trying to remove the message_filter
>> twice since we may have the following call chain:
>>
>> filter_data_remove_callback()
>> ?filter_data_free()
>> ? ?g_dbus_remove_watch()
>> ? ? ?filter_data_remove_callback()
>> ? ? ? ?filter_data_free()
>> ? ? ? ?dbus_connection_remove_filter()
>> ?dbus_connection_remove_filter()
>>
>> Because of this we can't currently watch for signals passing the bus
>> name. After this patch we don't have this issue anymore.
>>
>> We fix this by checking if we are the last filter_data before calling
>> filter_data_free and thus not calling dbus_connection_remove_filter()
>> twice.
>> ---
>> ?gdbus/watch.c | 12 +++++++++---
>> ?1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdbus/watch.c b/gdbus/watch.c
>> index 9a716b0..6fed264 100644
>> --- a/gdbus/watch.c
>> +++ b/gdbus/watch.c
>> @@ -350,6 +350,7 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct filter_callback *cb)
>> ?{
>> ? ? ? ?DBusConnection *connection;
>> + ? ? ? struct filter_data *data_next;
>>
>> ? ? ? ?data->callbacks = g_slist_remove(data->callbacks, cb);
>> ? ? ? ?data->processed = g_slist_remove(data->processed, cb);
>> @@ -376,12 +377,17 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
>>
>> ? ? ? ?connection = dbus_connection_ref(data->connection);
>> ? ? ? ?listeners = g_slist_remove(listeners, data);
>> +
>> + ? ? ? /*
>> + ? ? ? ?* filter_data_free() may remove the latest data - we need to check
>> + ? ? ? ?* before it runs if it's our duty to remove the filter
>> + ? ? ? ?*/
>> + ? ? ? data_next = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
>> ? ? ? ?filter_data_free(data);
>>
>> ? ? ? ?/* Remove filter if there are no listeners left for the connection */
>> - ? ? ? data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
>> - ? ? ? if (data == NULL)
>> + ? ? ? if (data_next == NULL)
>> ? ? ? ? ? ? ? ?dbus_connection_remove_filter(connection, message_filter,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL);
>>
>> --
>> 1.7.11
>
> Hmm do you have checks enabled in libdbus? It seems this is cause by:
>
> #ifndef DBUS_DISABLE_CHECKS
> ?if (filter == NULL)
> ? ?{
> ? ? ?_dbus_warn_check_failed ("Attempt to remove filter function %p
> user data %p, but no such filter has been added\n",
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? function, user_data);
> ? ? ?return;
> ? ?}
> #endif
Probably... this is the stock libdbus from Archlinux. I also found
interest that all possible users of g_dbus_add_signal_watch() with a
bus name were passing a NULL, which is clearly wrong. That made me
think it was like this to workaround this bug.
>
> Anyway I couldn't reproduce by changing ofono with stock libdbus from
> FC 17, although I see the problem just by looking at the code, but
> there other places where this need to be checked as well and the code
> could be simplified:
>
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index 9a716b0..d749176 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -376,15 +376,14 @@ static gboolean
> filter_data_remove_callback(struct filter_data *data,
>
> ? ? ? ?connection = dbus_connection_ref(data->connection);
> ? ? ? ?listeners = g_slist_remove(listeners, data);
> - ? ? ? filter_data_free(data);
>
> ? ? ? ?/* Remove filter if there are no listeners left for the connection */
> - ? ? ? data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> - ? ? ? if (data == NULL)
> + ? ? ? if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL) == NULL)
> ? ? ? ? ? ? ? ?dbus_connection_remove_filter(connection, message_filter,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL);
>
> + ? ? ? filter_data_free(data);
> ? ? ? ?dbus_connection_unref(connection);
>
> ? ? ? ?return TRUE;
ahn... right. We already removed data from listeners list so we can
call dbus_connection_remove_filter() before filter_data_free().
Ack.
Lucas De Marchi
Hi Lucas,
On Sat, Jun 23, 2012 at 5:02 PM, Lucas De Marchi
<[email protected]> wrote:
> If there's a signal watch that's also watching for name
> (data->name_watch) currently we are trying to remove the message_filter
> twice since we may have the following call chain:
>
> filter_data_remove_callback()
> ?filter_data_free()
> ? ?g_dbus_remove_watch()
> ? ? ?filter_data_remove_callback()
> ? ? ? ?filter_data_free()
> ? ? ? ?dbus_connection_remove_filter()
> ?dbus_connection_remove_filter()
>
> Because of this we can't currently watch for signals passing the bus
> name. After this patch we don't have this issue anymore.
>
> We fix this by checking if we are the last filter_data before calling
> filter_data_free and thus not calling dbus_connection_remove_filter()
> twice.
> ---
> ?gdbus/watch.c | 12 +++++++++---
> ?1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index 9a716b0..6fed264 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -350,6 +350,7 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct filter_callback *cb)
> ?{
> ? ? ? ?DBusConnection *connection;
> + ? ? ? struct filter_data *data_next;
>
> ? ? ? ?data->callbacks = g_slist_remove(data->callbacks, cb);
> ? ? ? ?data->processed = g_slist_remove(data->processed, cb);
> @@ -376,12 +377,17 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
>
> ? ? ? ?connection = dbus_connection_ref(data->connection);
> ? ? ? ?listeners = g_slist_remove(listeners, data);
> +
> + ? ? ? /*
> + ? ? ? ?* filter_data_free() may remove the latest data - we need to check
> + ? ? ? ?* before it runs if it's our duty to remove the filter
> + ? ? ? ?*/
> + ? ? ? data_next = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> ? ? ? ?filter_data_free(data);
>
> ? ? ? ?/* Remove filter if there are no listeners left for the connection */
> - ? ? ? data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> - ? ? ? if (data == NULL)
> + ? ? ? if (data_next == NULL)
> ? ? ? ? ? ? ? ?dbus_connection_remove_filter(connection, message_filter,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL);
>
> --
> 1.7.11
Hmm do you have checks enabled in libdbus? It seems this is cause by:
#ifndef DBUS_DISABLE_CHECKS
if (filter == NULL)
{
_dbus_warn_check_failed ("Attempt to remove filter function %p
user data %p, but no such filter has been added\n",
function, user_data);
return;
}
#endif
Anyway I couldn't reproduce by changing ofono with stock libdbus from
FC 17, although I see the problem just by looking at the code, but
there other places where this need to be checked as well and the code
could be simplified:
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 9a716b0..d749176 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -376,15 +376,14 @@ static gboolean
filter_data_remove_callback(struct filter_data *data,
connection = dbus_connection_ref(data->connection);
listeners = g_slist_remove(listeners, data);
- filter_data_free(data);
/* Remove filter if there are no listeners left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
- NULL);
- if (data == NULL)
+ if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL) == NULL)
dbus_connection_remove_filter(connection, message_filter,
NULL);
+ filter_data_free(data);
dbus_connection_unref(connection);
return TRUE;
@@ -537,15 +536,15 @@ static DBusHandlerResult
message_filter(DBusConnection *connection,
remove_match(data);
listeners = g_slist_remove(listeners, data);
- filter_data_free(data);
- /* Remove filter if there no listener left for the connection */
- data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
- NULL);
- if (data == NULL)
+ /* Remove filter if there are no listeners left for the connection */
+ if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+ NULL) == NULL)
dbus_connection_remove_filter(connection, message_filter,
NULL);
+ filter_data_free(data);
+
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
--
Luiz Augusto von Dentz