[Pkg-telepathy-commits] [telepathy-glib-1] 18/212: tp_dbus_daemon_watch_name_owner: rewrite in terms of g_bus_watch_name_on_connection
Simon McVittie
smcv at debian.org
Wed May 14 12:08:46 UTC 2014
This is an automated email from the git hooks/post-receive script.
smcv pushed a commit to branch debian
in repository telepathy-glib-1.
commit e8a08f3600eee2643556cedbf58f6ec8997164d0
Author: Simon McVittie <simon.mcvittie at collabora.co.uk>
Date: Tue Mar 11 18:20:44 2014 +0000
tp_dbus_daemon_watch_name_owner: rewrite in terms of g_bus_watch_name_on_connection
There's more debug here than I'd normally use, because this is a
large, non-bisectable rewrite. We can delete the bits that are
excessively noisy after we know it all works :-)
---
telepathy-glib/dbus-daemon.c | 358 ++++++-------------------------------------
tests/suppressions/tpl.supp | 8 -
tools/telepathy-glib.supp | 8 -
3 files changed, 43 insertions(+), 331 deletions(-)
diff --git a/telepathy-glib/dbus-daemon.c b/telepathy-glib/dbus-daemon.c
index e22ab8b..31cc9cc 100644
--- a/telepathy-glib/dbus-daemon.c
+++ b/telepathy-glib/dbus-daemon.c
@@ -147,6 +147,7 @@ tp_dbus_daemon_new (GDBusConnection *connection)
typedef struct
{
+ guint id;
gchar *last_owner;
GArray *callbacks;
gsize invoking;
@@ -246,180 +247,33 @@ _tp_dbus_daemon_name_owner_changed (TpDBusDaemon *self,
g_object_unref (self);
}
-static dbus_int32_t daemons_slot = -1;
-
-typedef struct {
- DBusConnection *libdbus;
- DBusMessage *message;
-} NOCIdleContext;
-
-static NOCIdleContext *
-noc_idle_context_new (DBusConnection *libdbus,
- DBusMessage *message)
-{
- NOCIdleContext *context = g_slice_new (NOCIdleContext);
-
- context->libdbus = dbus_connection_ref (libdbus);
- context->message = dbus_message_ref (message);
- return context;
-}
-
static void
-noc_idle_context_free (gpointer data)
-{
- NOCIdleContext *context = data;
-
- dbus_connection_unref (context->libdbus);
- dbus_message_unref (context->message);
- g_slice_free (NOCIdleContext, context);
-}
-
-static gboolean
-noc_idle_context_invoke (gpointer data)
-{
- NOCIdleContext *context = data;
- const gchar *name;
- const gchar *old_owner;
- const gchar *new_owner;
- DBusError dbus_error = DBUS_ERROR_INIT;
- GSList **daemons;
-
- if (daemons_slot == -1)
- return FALSE;
-
- if (!dbus_message_get_args (context->message, &dbus_error,
- DBUS_TYPE_STRING, &name,
- DBUS_TYPE_STRING, &old_owner,
- DBUS_TYPE_STRING, &new_owner,
- DBUS_TYPE_INVALID))
- {
- DEBUG ("Couldn't unpack NameOwnerChanged(s, s, s): %s: %s",
- dbus_error.name, dbus_error.message);
- dbus_error_free (&dbus_error);
- return FALSE;
- }
-
- daemons = dbus_connection_get_data (context->libdbus, daemons_slot);
-
- DEBUG ("NameOwnerChanged(%s, %s -> %s)", name, old_owner, new_owner);
-
- /* should always be non-NULL, barring bugs */
- if (G_LIKELY (daemons != NULL))
- {
- GSList *iter;
-
- for (iter = *daemons; iter != NULL; iter = iter->next)
- {
- _tp_dbus_daemon_name_owner_changed (iter->data, name, new_owner);
- }
- }
-
- return FALSE;
-}
-
-static DBusHandlerResult
-_tp_dbus_daemon_name_owner_changed_filter (DBusConnection *libdbus,
- DBusMessage *message,
- void *unused G_GNUC_UNUSED)
-{
- /* We have to do the real work in an idle, so we don't break re-entrant
- * calls (the dbus-glib event source isn't re-entrant) */
- if (dbus_message_is_signal (message, DBUS_INTERFACE_DBUS,
- "NameOwnerChanged") &&
- dbus_message_has_sender (message, DBUS_SERVICE_DBUS))
- g_idle_add_full (G_PRIORITY_HIGH, noc_idle_context_invoke,
- noc_idle_context_new (libdbus, message),
- noc_idle_context_free);
-
- return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
-}
-
-typedef struct {
- TpDBusDaemon *self;
- gchar *name;
- DBusMessage *reply;
- gsize refs;
-} GetNameOwnerContext;
-
-static GetNameOwnerContext *
-get_name_owner_context_new (TpDBusDaemon *self,
- const gchar *name)
+_tp_dbus_daemon_name_appeared_cb (GDBusConnection *connection G_GNUC_UNUSED,
+ const gchar *name,
+ const gchar *name_owner,
+ gpointer user_data)
{
- GetNameOwnerContext *context = g_slice_new (GetNameOwnerContext);
-
- context->self = g_object_ref (self);
- context->name = g_strdup (name);
- context->reply = NULL;
- context->refs = 1;
- return context;
+ DEBUG ("%s is owned by %s", name, name_owner);
+ _tp_dbus_daemon_name_owner_changed (user_data, name, name_owner);
}
static void
-get_name_owner_context_unref (gpointer data)
-{
- GetNameOwnerContext *context = data;
-
- if (--context->refs == 0)
- {
- g_object_unref (context->self);
- g_free (context->name);
-
- if (context->reply != NULL)
- dbus_message_unref (context->reply);
-
- g_slice_free (GetNameOwnerContext, context);
- }
-}
-
-static gboolean
-_tp_dbus_daemon_get_name_owner_idle (gpointer data)
+_tp_dbus_daemon_name_vanished_cb (GDBusConnection *connection,
+ const gchar *name,
+ gpointer user_data)
{
- GetNameOwnerContext *context = data;
- const gchar *owner = "";
-
- if (context->reply == NULL)
+ if (tp_proxy_get_invalidated (user_data) != NULL)
{
- DEBUG ("Connection disconnected or no reply to GetNameOwner(%s)",
- context->name);
- }
- else if (dbus_message_get_type (context->reply) ==
- DBUS_MESSAGE_TYPE_METHOD_RETURN)
- {
- if (dbus_message_get_args (context->reply, NULL,
- DBUS_TYPE_STRING, &owner,
- DBUS_TYPE_INVALID))
- {
- DEBUG ("GetNameOwner(%s) -> %s", context->name, owner);
- }
- else
- {
- DEBUG ("Malformed reply from GetNameOwner(%s), assuming no owner",
- context->name);
- }
+ /* telepathy-glib has not traditionally called "name owner lost"
+ * callbacks when the D-Bus connection dropped, which applications
+ * might be relying on. tests/dbus/dbus.c certainly does. */
+ DEBUG ("%s (ignoring because %p has been invalidated)", name, user_data);
}
else
{
- if (DEBUGGING)
- {
- DBusError error = DBUS_ERROR_INIT;
-
- if (dbus_set_error_from_message (&error, context->reply))
- {
- DEBUG ("GetNameOwner(%s) raised %s: %s", context->name,
- error.name, error.message);
- dbus_error_free (&error);
- }
- else
- {
- DEBUG ("Unexpected message type from GetNameOwner(%s)",
- context->name);
- }
- }
+ DEBUG ("%s", name);
+ _tp_dbus_daemon_name_owner_changed (user_data, name, "");
}
-
- _tp_dbus_daemon_name_owner_changed (context->self, context->name, owner);
-
- return FALSE;
}
/**
@@ -435,38 +289,6 @@ _tp_dbus_daemon_get_name_owner_idle (gpointer data)
* Since: 0.7.1
*/
-static inline gchar *
-_tp_dbus_daemon_get_noc_rule (const gchar *name)
-{
- return g_strdup_printf ("type='signal',"
- "sender='" DBUS_SERVICE_DBUS "',"
- "path='" DBUS_PATH_DBUS "',"
- "interface='"DBUS_INTERFACE_DBUS "',"
- "member='NameOwnerChanged',"
- "arg0='%s'", name);
-}
-
-static void
-_tp_dbus_daemon_get_name_owner_notify (DBusPendingCall *pc,
- gpointer data)
-{
- GetNameOwnerContext *context = data;
-
- /* we recycle this function for the case where the connection is already
- * disconnected: in that case we use pc = NULL */
- if (pc != NULL)
- context->reply = dbus_pending_call_steal_reply (pc);
-
- /* We have to do the real work in an idle, so we don't break re-entrant
- * calls (the dbus-glib event source isn't re-entrant) */
- context->refs++;
- g_idle_add_full (G_PRIORITY_HIGH, _tp_dbus_daemon_get_name_owner_idle,
- context, get_name_owner_context_unref);
-
- if (pc != NULL)
- dbus_pending_call_unref (pc);
-}
-
/**
* tp_dbus_daemon_watch_name_owner:
* @self: The D-Bus daemon
@@ -483,6 +305,8 @@ _tp_dbus_daemon_get_name_owner_notify (DBusPendingCall *pc,
* If multiple watches are registered for the same @name, they will be called
* in the order they were registered.
*
+ * New code should use g_bus_watch_name() or similar instead.
+ *
* Since: 0.7.1
*/
void
@@ -501,13 +325,11 @@ tp_dbus_daemon_watch_name_owner (TpDBusDaemon *self,
TP_DBUS_NAME_TYPE_ANY, NULL));
g_return_if_fail (callback != NULL);
+ DEBUG ("%s", name);
+
if (watch == NULL)
{
- gchar *match_rule;
- DBusMessage *message;
- DBusPendingCall *pc = NULL;
- GetNameOwnerContext *context = get_name_owner_context_new (self, name);
-
+ DEBUG ("- new watch");
/* Allocate a new watch */
watch = g_slice_new0 (_NameOwnerWatch);
watch->last_owner = NULL;
@@ -517,46 +339,17 @@ tp_dbus_daemon_watch_name_owner (TpDBusDaemon *self,
g_hash_table_insert (self->priv->name_owner_watches, g_strdup (name),
watch);
- /* We want to be notified about name owner changes for this one.
- * Assume the match addition will succeed; there's no good way to cope
- * with failure here... */
- match_rule = _tp_dbus_daemon_get_noc_rule (name);
- DEBUG ("Adding match rule %s", match_rule);
- dbus_bus_add_match (self->priv->libdbus, match_rule, NULL);
- g_free (match_rule);
-
- message = dbus_message_new_method_call (DBUS_SERVICE_DBUS,
- DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
-
- if (message == NULL)
- ERROR ("Out of memory");
-
- /* We already checked that @name was in (a small subset of) UTF-8,
- * so OOM is the only thing that can go wrong. The use of &name here
- * is because libdbus is strange. */
- if (!dbus_message_append_args (message,
- DBUS_TYPE_STRING, &name,
- DBUS_TYPE_INVALID))
- ERROR ("Out of memory");
-
- if (!dbus_connection_send_with_reply (self->priv->libdbus,
- message, &pc, -1))
- ERROR ("Out of memory");
- /* pc is unreffed by _tp_dbus_daemon_get_name_owner_notify */
- dbus_message_unref (message);
-
- if (pc == NULL || dbus_pending_call_get_completed (pc))
- {
- /* pc can be NULL when the connection is already disconnected */
- _tp_dbus_daemon_get_name_owner_notify (pc, context);
- get_name_owner_context_unref (context);
- }
- else if (!dbus_pending_call_set_notify (pc,
- _tp_dbus_daemon_get_name_owner_notify,
- context, get_name_owner_context_unref))
- {
- ERROR ("Out of memory");
- }
+ watch->id = g_bus_watch_name_on_connection (
+ tp_proxy_get_dbus_connection (self),
+ name, G_BUS_NAME_WATCHER_FLAGS_NONE,
+ _tp_dbus_daemon_name_appeared_cb,
+ _tp_dbus_daemon_name_vanished_cb,
+ g_object_ref (self),
+ g_object_unref);
+ }
+ else
+ {
+ DEBUG ("- appending to existing watch");
}
g_array_append_val (watch->callbacks, tmp);
@@ -564,6 +357,7 @@ tp_dbus_daemon_watch_name_owner (TpDBusDaemon *self,
if (watch->last_owner != NULL)
{
/* FIXME: should avoid reentrancy? */
+ DEBUG ("- already owned by %s", watch->last_owner);
callback (self, name, watch->last_owner, user_data);
}
}
@@ -573,8 +367,6 @@ _tp_dbus_daemon_stop_watching (TpDBusDaemon *self,
const gchar *name,
_NameOwnerWatch *watch)
{
- gchar *match_rule;
-
/* Clean up any leftöver callbacks. */
if (watch->callbacks->len > 0)
{
@@ -590,14 +382,10 @@ _tp_dbus_daemon_stop_watching (TpDBusDaemon *self,
}
}
+ g_bus_unwatch_name (watch->id);
g_array_unref (watch->callbacks);
g_free (watch->last_owner);
g_slice_free (_NameOwnerWatch, watch);
-
- match_rule = _tp_dbus_daemon_get_noc_rule (name);
- DEBUG ("Removing match rule %s", match_rule);
- dbus_bus_remove_match (self->priv->libdbus, match_rule, NULL);
- g_free (match_rule);
}
/**
@@ -630,6 +418,8 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
g_return_val_if_fail (name != NULL, FALSE);
g_return_val_if_fail (callback != NULL, FALSE);
+ DEBUG ("%s", name);
+
if (watch != NULL)
{
/* Check to see whether this (callback, user_data) pair is in the watch's
@@ -639,6 +429,8 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
* we iterate in reverse to have "last in = first out" as documented. */
guint i;
+ DEBUG ("- %u watch(es) found", array->len);
+
for (i = array->len; i > 0; i--)
{
_NameOwnerSubWatch *entry = &g_array_index (array,
@@ -646,6 +438,7 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
if (entry->callback == callback && entry->user_data == user_data)
{
+ DEBUG ("- found matching callback and user data");
entry->callback = NULL;
tp_dbus_daemon_maybe_free_name_owner_watch (self, name, watch);
return TRUE;
@@ -654,6 +447,7 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
}
/* We haven't found it */
+ DEBUG ("- did not find matching callback and user data");
return FALSE;
}
@@ -1373,59 +1167,19 @@ tp_dbus_daemon_list_activatable_names (TpDBusDaemon *self,
callback, user_data, destroy, weak_object);
}
-static void
-free_daemon_list (gpointer p)
-{
- GSList **slistp = p;
-
- g_slist_free (*slistp);
- g_slice_free (GSList *, slistp);
-}
-
-/* If you add more slice-allocation in this function, make the suppression
- * "tp_dbus_daemon_constructor @daemons once per DBusConnection" in
- * telepathy-glib.supp more specific. */
static GObject *
tp_dbus_daemon_constructor (GType type,
guint n_params,
GObjectConstructParam *params)
{
- GObjectClass *object_class =
- (GObjectClass *) tp_dbus_daemon_parent_class;
+ GObjectClass *object_class = G_OBJECT_CLASS (tp_dbus_daemon_parent_class);
+
TpDBusDaemon *self = TP_DBUS_DAEMON (object_class->constructor (type,
n_params, params));
- GSList **daemons;
g_assert (!tp_strdiff (tp_proxy_get_bus_name (self), DBUS_SERVICE_DBUS));
g_assert (!tp_strdiff (tp_proxy_get_object_path (self), DBUS_PATH_DBUS));
- self->priv->libdbus = dbus_connection_ref (
- dbus_g_connection_get_connection (
- tp_proxy_get_dbus_connection (self)));
-
- /* one ref per TpDBusDaemon, released in finalize */
- if (!dbus_connection_allocate_data_slot (&daemons_slot))
- ERROR ("Out of memory");
-
- daemons = dbus_connection_get_data (self->priv->libdbus, daemons_slot);
-
- if (daemons == NULL)
- {
- /* This slice is never freed; it's a one-per-DBusConnection leak. */
- daemons = g_slice_new (GSList *);
-
- *daemons = NULL;
- dbus_connection_set_data (self->priv->libdbus, daemons_slot, daemons,
- free_daemon_list);
-
- /* we add this filter at most once per DBusConnection */
- if (!dbus_connection_add_filter (self->priv->libdbus,
- _tp_dbus_daemon_name_owner_changed_filter, NULL, NULL))
- ERROR ("Out of memory");
- }
-
- *daemons = g_slist_prepend (*daemons, self);
-
return (GObject *) self;
}
@@ -1443,7 +1197,6 @@ static void
tp_dbus_daemon_dispose (GObject *object)
{
TpDBusDaemon *self = TP_DBUS_DAEMON (object);
- GSList **daemons;
if (self->priv->name_owner_watches != NULL)
{
@@ -1467,28 +1220,6 @@ tp_dbus_daemon_dispose (GObject *object)
g_hash_table_unref (tmp);
}
- if (self->priv->libdbus != NULL)
- {
- /* remove myself from the list to be notified on NoC */
- daemons = dbus_connection_get_data (self->priv->libdbus, daemons_slot);
-
- /* should always be non-NULL, barring bugs */
- if (G_LIKELY (daemons != NULL))
- {
- *daemons = g_slist_remove (*daemons, self);
-
- if (*daemons == NULL)
- {
- /* this results in a call to free_daemon_list (daemons) */
- dbus_connection_set_data (self->priv->libdbus, daemons_slot,
- NULL, NULL);
- }
- }
-
- dbus_connection_unref (self->priv->libdbus);
- self->priv->libdbus = NULL;
- }
-
G_OBJECT_CLASS (tp_dbus_daemon_parent_class)->dispose (object);
}
@@ -1497,9 +1228,6 @@ tp_dbus_daemon_finalize (GObject *object)
{
GObjectFinalizeFunc chain_up = G_OBJECT_CLASS (tp_dbus_daemon_parent_class)->finalize;
- /* one ref per TpDBusDaemon, from constructor */
- dbus_connection_free_data_slot (&daemons_slot);
-
if (chain_up != NULL)
chain_up (object);
}
diff --git a/tests/suppressions/tpl.supp b/tests/suppressions/tpl.supp
index 573d339..737bf33 100644
--- a/tests/suppressions/tpl.supp
+++ b/tests/suppressions/tpl.supp
@@ -258,14 +258,6 @@
}
{
- tp_dbus_daemon_constructor filter not freed til we fall off the bus
- Memcheck:Leak
- ...
- fun:dbus_connection_add_filter
- fun:tp_dbus_daemon_constructor
-}
-
-{
Leak in tp-glib 0.11.16 (Fedora 14)
Memcheck:Leak
...
diff --git a/tools/telepathy-glib.supp b/tools/telepathy-glib.supp
index bd6708e..f79c1cd 100644
--- a/tools/telepathy-glib.supp
+++ b/tools/telepathy-glib.supp
@@ -309,14 +309,6 @@
}
{
- tp_dbus_daemon_constructor filter not freed til we fall off the bus
- Memcheck:Leak
- ...
- fun:dbus_connection_add_filter
- fun:tp_dbus_daemon_constructor
-}
-
-{
tp_g_socket_address_from_variant reffing GNIO types
Memcheck:Leak
...
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-telepathy/telepathy-glib-1.git
More information about the Pkg-telepathy-commits
mailing list