From e8ff82340d94da977a72e59fa09e0b6082312e3e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 9 Jan 2013 12:05:13 +0000 Subject: [PATCH 2/2] Communicate the detailed error from McdConnection to McdAccount In particular, this propagates the error from RequestConnection failing correctly, so add a regression test for that. --- src/mcd-account.c | 10 +- src/mcd-connection.c | 115 ++++++++++++++++++++--- tests/twisted/Makefile.am | 1 + tests/twisted/account-manager/req-conn-fails.py | 63 +++++++++++++ 4 files changed, 169 insertions(+), 20 deletions(-) create mode 100644 tests/twisted/account-manager/req-conn-fails.py diff --git a/src/mcd-account.c b/src/mcd-account.c index 2d7f130..b1b20d0 100644 --- a/src/mcd-account.c +++ b/src/mcd-account.c @@ -4275,16 +4275,10 @@ on_conn_status_changed (McdConnection *connection, TpConnectionStatus status, TpConnectionStatusReason reason, TpConnection *tp_conn, + const gchar *dbus_error, + GHashTable *details, McdAccount *account) { - const gchar *dbus_error = NULL; - const GHashTable *details = NULL; - - if (tp_conn != NULL) - { - dbus_error = tp_connection_get_detailed_error (tp_conn, &details); - } - _mcd_account_set_connection_status (account, status, reason, tp_conn, dbus_error, details); } diff --git a/src/mcd-connection.c b/src/mcd-connection.c index 739d1a4..93b81d5 100644 --- a/src/mcd-connection.c +++ b/src/mcd-connection.c @@ -44,6 +44,7 @@ #include #include +#include #include #include @@ -658,7 +659,7 @@ on_connection_status_changed (TpConnection *tp_conn, GParamSpec *pspec, { case TP_CONNECTION_STATUS_CONNECTING: g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, - conn_status, conn_reason, tp_conn); + conn_status, conn_reason, tp_conn, NULL, NULL); priv->abort_reason = TP_CONNECTION_STATUS_REASON_NONE_SPECIFIED; priv->connected = FALSE; break; @@ -666,7 +667,7 @@ on_connection_status_changed (TpConnection *tp_conn, GParamSpec *pspec, case TP_CONNECTION_STATUS_CONNECTED: { g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, - conn_status, conn_reason, tp_conn); + conn_status, conn_reason, tp_conn, NULL, NULL); if (priv->probation_timer == 0) { @@ -1430,6 +1431,49 @@ mcd_connection_early_get_interfaces_cb (TpConnection *tp_conn, mcd_connection_done_task_before_connect (self); } +static gchar * +translate_g_error (GQuark domain, + gint code, + const gchar *message) +{ + if (domain == TP_ERROR) + { + return g_strdup (tp_error_get_dbus_name (code)); + } + else if (domain == TP_DBUS_ERRORS) + { + switch (code) + { + case TP_DBUS_ERROR_UNKNOWN_REMOTE_ERROR: + { + const gchar *p = strchr (message, ':'); + + if (p != NULL) + { + gchar *tmp = g_strndup (message, message - p); + + /* The syntactic restrictions for error names are the same + * as for interface names. */ + if (g_dbus_is_interface_name (tmp)) + return tmp; + + g_free (tmp); + } + } + break; + + case TP_DBUS_ERROR_NO_INTERFACE: + return g_strdup (DBUS_ERROR_UNKNOWN_INTERFACE); + + case TP_DBUS_ERROR_NAME_OWNER_LOST: + return g_strdup (DBUS_ERROR_NAME_HAS_NO_OWNER); + } + } + + /* catch-all */ + return g_strdup (DBUS_ERROR_FAILED); +} + static void request_connection_cb (TpConnectionManager *proxy, const gchar *bus_name, const gchar *obj_path, const GError *tperror, @@ -1468,7 +1512,8 @@ request_connection_cb (TpConnectionManager *proxy, const gchar *bus_name, { g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, TP_CONNECTION_STATUS_DISCONNECTED, - TP_CONNECTION_STATUS_REASON_REQUESTED, NULL); + TP_CONNECTION_STATUS_REASON_REQUESTED, + NULL, "", NULL); } goto finally; @@ -1478,12 +1523,21 @@ request_connection_cb (TpConnectionManager *proxy, const gchar *bus_name, if (tperror) { + gchar *dbus_error = translate_g_error (tperror->domain, + tperror->code, tperror->message); + GHashTable *details = tp_asv_new ( + "debug-message", G_TYPE_STRING, tperror->message, + NULL); + g_warning ("%s: RequestConnection failed: %s", G_STRFUNC, tperror->message); g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, TP_CONNECTION_STATUS_DISCONNECTED, - TP_CONNECTION_STATUS_REASON_NETWORK_ERROR, NULL); + TP_CONNECTION_STATUS_REASON_NONE_SPECIFIED, NULL, + dbus_error, details); + g_hash_table_unref (details); + g_free (dbus_error); goto finally; } @@ -1524,7 +1578,7 @@ _mcd_connection_connect_with_params (McdConnection *connection, g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, TP_CONNECTION_STATUS_CONNECTING, - TP_CONNECTION_STATUS_REASON_REQUESTED, NULL); + TP_CONNECTION_STATUS_REASON_REQUESTED, NULL, NULL, NULL); /* If the McdConnection gets aborted (which results in it being freed!), * we need to kill off the Connection. So, we can't use connection as the @@ -1560,9 +1614,27 @@ _mcd_connection_release_tp_connection (McdConnection *connection) g_signal_emit (connection, signals[SELF_PRESENCE_CHANGED], 0, TP_CONNECTION_PRESENCE_TYPE_OFFLINE, "offline", ""); - g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, - TP_CONNECTION_STATUS_DISCONNECTED, - priv->abort_reason, priv->tp_conn); + if (priv->abort_reason == TP_CONNECTION_STATUS_REASON_REQUESTED) + { + g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, + TP_CONNECTION_STATUS_DISCONNECTED, + priv->abort_reason, priv->tp_conn, "", NULL); + } + else + { + const gchar *dbus_error = NULL; + const GHashTable *details = NULL; + + if (priv->tp_conn != NULL) + { + dbus_error = tp_connection_get_detailed_error (priv->tp_conn, + &details); + } + + g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, + TP_CONNECTION_STATUS_DISCONNECTED, + priv->abort_reason, priv->tp_conn, dbus_error, details); + } if (priv->tp_conn) { @@ -1937,11 +2009,19 @@ mcd_connection_class_init (McdConnectionClass * klass) NULL, NULL, NULL, G_TYPE_NONE, 3, G_TYPE_UINT, G_TYPE_STRING, G_TYPE_STRING); + /** + * @status: + * @status_reason: + * @connection: + * @dbus_error: a D-Bus error name, or %NULL + * @details: a #GHashTable from string to #GValue, or %NULL + */ signals[CONNECTION_STATUS_CHANGED] = g_signal_new ( "connection-status-changed", G_OBJECT_CLASS_TYPE (klass), G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED, 0, NULL, NULL, NULL, - G_TYPE_NONE, 3, G_TYPE_UINT, G_TYPE_UINT, TP_TYPE_CONNECTION); + G_TYPE_NONE, 5, G_TYPE_UINT, G_TYPE_UINT, TP_TYPE_CONNECTION, + G_TYPE_STRING, G_TYPE_HASH_TABLE); signals[READY] = g_signal_new ("ready", G_OBJECT_CLASS_TYPE (klass), @@ -2228,8 +2308,10 @@ _mcd_connection_set_tp_connection (McdConnection *connection, TP_CONNECTION_FEATURE_CONNECTED, 0 }; + GError *inner_error = NULL; g_return_if_fail (MCD_IS_CONNECTION (connection)); + g_return_if_fail (error != NULL); priv = connection->priv; if (priv->tp_conn != NULL) @@ -2249,14 +2331,23 @@ _mcd_connection_set_tp_connection (McdConnection *connection, g_assert (priv->tp_conn == NULL); priv->tp_conn = tp_connection_new (priv->dbus_daemon, bus_name, - obj_path, error); + obj_path, &inner_error); DEBUG ("new connection is %p", priv->tp_conn); if (!priv->tp_conn) { + GHashTable *details = tp_asv_new ( + "debug-message", inner_error->message, + NULL); + + /* Constructing a TpConnection can only fail from invalid arguments, + * which would mean either MC or the connection manager is confused. */ g_signal_emit (connection, signals[CONNECTION_STATUS_CHANGED], 0, TP_CONNECTION_STATUS_DISCONNECTED, - TP_CONNECTION_STATUS_REASON_NETWORK_ERROR, - NULL); + TP_CONNECTION_STATUS_REASON_NONE_SPECIFIED, + NULL, TP_ERROR_STR_CONFUSED, details); + + g_hash_table_unref (details); + g_propagate_error (error, inner_error); return; } /* FIXME: need some way to feed the status into the Account, but we don't diff --git a/tests/twisted/Makefile.am b/tests/twisted/Makefile.am index 2e46546..67960b5 100644 --- a/tests/twisted/Makefile.am +++ b/tests/twisted/Makefile.am @@ -16,6 +16,7 @@ TWISTED_BASIC_TESTS = \ account-manager/presence.py \ account-manager/reconnect.py \ account-manager/recover-from-disconnect.py \ + account-manager/req-conn-fails.py \ account-manager/request-online.py \ account-manager/service.py \ account-manager/update-parameters.py \ diff --git a/tests/twisted/account-manager/req-conn-fails.py b/tests/twisted/account-manager/req-conn-fails.py new file mode 100644 index 0000000..bc3f01a --- /dev/null +++ b/tests/twisted/account-manager/req-conn-fails.py @@ -0,0 +1,63 @@ +# vim: set fileencoding=utf-8 : +# +# Copyright © 2011-2012 Collabora Ltd. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA +# 02110-1301 USA + +import dbus +from servicetest import ( + tp_name_prefix, tp_path_prefix, assertEquals, +) +from mctest import ( + exec_test, create_fakecm_account, SimulatedConnection, +) +import constants as cs + +def test(q, bus, mc): + params = dbus.Dictionary( + {"account": "someguy@example.com", + "password": "secrecy", + }, signature='sv') + (cm_name_ref, account) = create_fakecm_account(q, bus, mc, params) + + account.Properties.Set(cs.ACCOUNT, 'Enabled', True) + + # Set online presence + presence = dbus.Struct( + (dbus.UInt32(cs.PRESENCE_TYPE_BUSY), 'busy', 'Fixing MC bugs'), + signature='uss') + account.Properties.Set(cs.ACCOUNT, 'RequestedPresence', presence) + + e = q.expect('dbus-method-call', method='RequestConnection', + args=['fakeprotocol', params], + destination=tp_name_prefix + '.ConnectionManager.fakecm', + path=tp_path_prefix + '/ConnectionManager/fakecm', + interface=tp_name_prefix + '.ConnectionManager', + handled=False) + + q.dbus_raise(e.message, cs.NOT_IMPLEMENTED, "CM is broken") + + # MC should report the connection dying. + e = q.expect('dbus-signal', signal='AccountPropertyChanged', + predicate=lambda e: 'ConnectionError' in e.args[0]) + changed, = e.args + assertEquals('/', changed['Connection']) + assertEquals(cs.CONN_STATUS_DISCONNECTED, changed['ConnectionStatus']) + assertEquals(cs.CONN_STATUS_REASON_NONE, changed['ConnectionStatusReason']) + assertEquals(cs.NOT_IMPLEMENTED, changed['ConnectionError']) + +if __name__ == '__main__': + exec_test(test) -- 1.7.10.4