Attachment 95717 Details for Bug 76112 – Use a better NoReply message for disconnection with reply pending

[patch] Use a better NoReply message for disconnection with reply pending

0001-Use-a-better-NoReply-message-for-disconnection-with-.patch (text/plain), 7.23 KB, created by Simon McVittie on 2014-03-13 12:08:14 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-03-13 12:08:14 UTC

Size: 7.23 KB

patch

obsolete

>From 47aec8389587584596c17e5f906022b3e3bb50c5 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Thu, 13 Mar 2014 12:06:07 +0000
>Subject: [PATCH] Use a better NoReply message for disconnection with reply
> pending
>
>As an implementation detail, dbus-daemon handles this situation by
>artificially triggering a timeout (even if its configured timeout for
>method calls is in fact infinite). However, using the same debug message
>for both is misleading, and can lead people who are debugging a service
>crash to blame dbus-daemon instead, wasting their time.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=76112
>---
> bus/connection.c                                   |  7 +-
> configure.ac                                       |  1 +
> test/Makefile.am                                   |  1 +
> .../data/valid-config-files/finite-timeout.conf.in | 19 +++++
> test/dbus-daemon.c                                 | 83 ++++++++++++++++++++++
> 5 files changed, 110 insertions(+), 1 deletion(-)
> create mode 100644 test/data/valid-config-files/finite-timeout.conf.in
>
>diff --git a/bus/connection.c b/bus/connection.c
>index ea2d155..af77bf0 100644
>--- a/bus/connection.c
>+++ b/bus/connection.c
>@@ -1554,7 +1554,12 @@ bus_pending_reply_send_no_reply (BusConnections  *connections,
>                                     DBUS_ERROR_NO_REPLY))
>     goto out;
> 
>-  errmsg = "Message did not receive a reply (timeout by message bus)";
>+  /* If you change these messages, adjust test/dbus-daemon.c to match */
>+  if (pending->will_send_reply == NULL)
>+    errmsg = "Message recipient disconnected from message bus without replying";
>+  else
>+    errmsg = "Message did not receive a reply (timeout by message bus)";
>+
>   dbus_message_iter_init_append (message, &iter);
>   if (!dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &errmsg))
>     goto out;
>diff --git a/configure.ac b/configure.ac
>index 13d1c87..2de7240 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -1775,6 +1775,7 @@ dbus-1.pc
> dbus-1-uninstalled.pc
> test/data/valid-config-files/debug-allow-all.conf
> test/data/valid-config-files/debug-allow-all-sha1.conf
>+test/data/valid-config-files/finite-timeout.conf
> test/data/valid-config-files/incoming-limit.conf
> test/data/valid-config-files-system/debug-allow-all-pass.conf
> test/data/valid-config-files-system/debug-allow-all-fail.conf
>diff --git a/test/Makefile.am b/test/Makefile.am
>index cec5cda..7f16414 100644
>--- a/test/Makefile.am
>+++ b/test/Makefile.am
>@@ -249,6 +249,7 @@ in_data = \
> 	data/valid-config-files-system/debug-allow-all-pass.conf.in \
> 	data/valid-config-files/debug-allow-all-sha1.conf.in \
> 	data/valid-config-files/debug-allow-all.conf.in \
>+	data/valid-config-files/finite-timeout.conf.in \
> 	data/valid-config-files/incoming-limit.conf.in \
> 	data/invalid-service-files-system/org.freedesktop.DBus.TestSuiteNoExec.service.in \
> 	data/invalid-service-files-system/org.freedesktop.DBus.TestSuiteNoService.service.in \
>diff --git a/test/data/valid-config-files/finite-timeout.conf.in b/test/data/valid-config-files/finite-timeout.conf.in
>new file mode 100644
>index 0000000..7d26d71
>--- /dev/null
>+++ b/test/data/valid-config-files/finite-timeout.conf.in
>@@ -0,0 +1,19 @@
>+<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-Bus Bus Configuration 1.0//EN"
>+ "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
>+<busconfig>
>+  
>+  <type>session</type>
>+  <listen>@TEST_LISTEN@</listen>
>+
>+  <policy context="default">
>+    
>+    <allow send_destination="*" eavesdrop="true"/>
>+    
>+    <allow eavesdrop="true"/>
>+    
>+    <allow own="*"/>
>+  </policy>
>+
>+  
>+  <limit name="reply_timeout">100</limit>
>+</busconfig>
>diff --git a/test/dbus-daemon.c b/test/dbus-daemon.c
>index c883425..fa5178f 100644
>--- a/test/dbus-daemon.c
>+++ b/test/dbus-daemon.c
>@@ -160,6 +160,10 @@ echo_filter (DBusConnection *connection,
>   if (dbus_message_get_type (message) != DBUS_MESSAGE_TYPE_METHOD_CALL)
>     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> 
>+  /* WaitForever() never replies, emulating a service that has got stuck */
>+  if (dbus_message_is_method_call (message, "com.example", "WaitForever"))
>+    return DBUS_HANDLER_RESULT_HANDLED;
>+
>   reply = dbus_message_new_method_return (message);
> 
>   if (reply == NULL)
>@@ -341,6 +345,77 @@ pending_call_store_reply (DBusPendingCall *pc,
> }
> 
> static void
>+test_no_reply (Fixture *f,
>+    gconstpointer context)
>+{
>+  const Config *config = context;
>+  DBusMessage *m;
>+  DBusPendingCall *pc;
>+  DBusMessage *reply = NULL;
>+  enum { TIMEOUT, DISCONNECT } mode;
>+  gboolean ok;
>+
>+  if (f->skip)
>+    return;
>+
>+  g_test_bug ("76112");
>+
>+  if (config != NULL && config->config_file != NULL)
>+    mode = TIMEOUT;
>+  else
>+    mode = DISCONNECT;
>+
>+  m = dbus_message_new_method_call (
>+      dbus_bus_get_unique_name (f->right_conn), "/",
>+      "com.example", "WaitForever");
>+
>+  add_echo_filter (f);
>+
>+  if (m == NULL)
>+    g_error ("OOM");
>+
>+  if (!dbus_connection_send_with_reply (f->left_conn, m, &pc,
>+                                        DBUS_TIMEOUT_INFINITE) ||
>+      pc == NULL)
>+    g_error ("OOM");
>+
>+  if (dbus_pending_call_get_completed (pc))
>+    pending_call_store_reply (pc, &reply);
>+  else if (!dbus_pending_call_set_notify (pc, pending_call_store_reply, &reply,
>+        NULL))
>+    g_error ("OOM");
>+
>+  dbus_pending_call_unref (pc);
>+  dbus_message_unref (m);
>+
>+  if (mode == DISCONNECT)
>+    {
>+      dbus_connection_remove_filter (f->right_conn, echo_filter, NULL);
>+      dbus_connection_close (f->right_conn);
>+      dbus_connection_unref (f->right_conn);
>+      f->right_conn = NULL;
>+    }
>+
>+  while (reply == NULL)
>+    test_main_context_iterate (f->ctx, TRUE);
>+
>+  /* using inefficient string comparison for better assertion message */
>+  g_assert_cmpstr (
>+      dbus_message_type_to_string (dbus_message_get_type (reply)), ==,
>+      dbus_message_type_to_string (DBUS_MESSAGE_TYPE_ERROR));
>+  ok = dbus_set_error_from_message (&f->e, reply);
>+  g_assert (ok);
>+  g_assert_cmpstr (f->e.name, ==, DBUS_ERROR_NO_REPLY);
>+
>+  if (mode == DISCONNECT)
>+    g_assert_cmpstr (f->e.message, ==,
>+        "Message recipient disconnected from message bus without replying");
>+  else
>+    g_assert_cmpstr (f->e.message, ==,
>+        "Message did not receive a reply (timeout by message bus)");
>+}
>+
>+static void
> test_creds (Fixture *f,
>     gconstpointer context)
> {
>@@ -501,6 +576,10 @@ static Config limited_config = {
>     "34393", 10000, "valid-config-files/incoming-limit.conf"
> };
> 
>+static Config finite_timeout_config = {
>+    NULL, 1, "valid-config-files/finite-timeout.conf"
>+};
>+
> int
> main (int argc,
>     char **argv)
>@@ -511,6 +590,10 @@ main (int argc,
>   g_test_add ("/echo/session", Fixture, NULL, setup, test_echo, teardown);
>   g_test_add ("/echo/limited", Fixture, &limited_config,
>       setup, test_echo, teardown);
>+  g_test_add ("/no-reply/disconnect", Fixture, NULL,
>+      setup, test_no_reply, teardown);
>+  g_test_add ("/no-reply/timeout", Fixture, &finite_timeout_config,
>+      setup, test_no_reply, teardown);
>   g_test_add ("/creds", Fixture, NULL, setup, test_creds, teardown);
> 
>   return g_test_run ();
>-- 
>1.9.0
>

Actions: View | Splinter Review

Attachments on bug 76112: 95717 | 106341

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.