Bug 34140 - add a message-spamming tool, and a service that replies to it
Summary: add a message-spamming tool, and a service that replies to it
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low enhancement
Assignee: Alban Crequy
QA Contact: D-Bus Maintainers
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-02-10 09:01 UTC by Simon McVittie
Modified: 2014-10-14 13:07 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
proposed initial implementation (15.10 KB, patch)
2011-02-14 11:08 UTC, Simon McVittie
Details | Splinter Review
[patch v2] Add dbus-spam, dbus-echo tools (16.71 KB, patch)
2011-02-25 08:53 UTC, Simon McVittie
Details | Splinter Review
[PATCH 2/4] dbus-spam: add support for sending pseudorandomly-sized messages (5.52 KB, patch)
2011-06-22 11:08 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/4] dbus-echo, dbus-spam: merge into a single binary, dbus-test-tool (15.84 KB, patch)
2011-06-22 11:08 UTC, Simon McVittie
Details | Splinter Review
[PATCH 4/4] dbus-spam: use the DBUS_TIMEOUT_INFINITE constant now that it exists (983 bytes, patch)
2011-06-22 11:09 UTC, Simon McVittie
Details | Splinter Review
dbus-spam: fix --random-size to not clobber argument parsing (1.61 KB, patch)
2011-08-05 05:43 UTC, Will Thompson
Details | Splinter Review
test-spam: don't try to demarshal a NULL buffer (901 bytes, patch)
2011-08-05 05:54 UTC, Will Thompson
Details | Splinter Review
add dbus-test-tool (cumulative) (25.92 KB, patch)
2011-09-26 03:07 UTC, Simon McVittie
Details | Splinter Review
dbus-echo, dbus-spam: remove unused variables (1.09 KB, patch)
2011-09-26 03:18 UTC, Simon McVittie
Details | Splinter Review
dbus-spam: use random seed as intended (614 bytes, patch)
2011-09-26 03:18 UTC, Simon McVittie
Details | Splinter Review
[PATCH 1/7] Add dbus-test-tool, currently with "echo" and "spam" modes (26.31 KB, patch)
2014-07-02 15:49 UTC, Alban Crequy
Details | Splinter Review
[PATCH 6/7] dbus-test-tool echo: add --no-reply and --no-listen (3.50 KB, patch)
2014-07-02 15:50 UTC, Alban Crequy
Details | Splinter Review
[PATCH 7/7] dbus-test-tool spam: add --new-conn (4.92 KB, patch)
2014-07-02 15:51 UTC, Alban Crequy
Details | Splinter Review
[PATCH 1/3] Add dbus-test-tool, currently with "echo" and "spam" modes (26.69 KB, patch)
2014-10-01 16:50 UTC, Alban Crequy
Details | Splinter Review
[PATCH 2/3] dbus-test-tool spam: add --messages-per-conn=N (6.38 KB, patch)
2014-10-01 16:51 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/3] dbus-test-tool: add black-hole mode (6.63 KB, patch)
2014-10-01 16:52 UTC, Alban Crequy
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-02-10 09:01:35 UTC
Everyone who tries to profile D-Bus seems to end up writing their own equivalent of this, with varying overhead from using dbus-glib, QtDBus, etc.

To profile the bus daemon under load with varying levels of parallelization, it'd be useful to have a minimal message-flooding tool which can send messages in a serial or parallel way, and a message-replying service which can have multiple instances.

For Bug #33524 it'd also be nice to be able to compare UTF-8 and byte-array payloads; I haven't implemented byte-array payloads yet.

It didn't take long to get these:

Usage: dbus-spam [OPTIONS]

Repeatedly call com.example.Spam() on the given D-Bus service.

Options:

    --session     use the session bus (default)
    --system      use the system bus

    --dest=NAME   call methods on NAME (default org.freedesktop.DBus)

    --count=N     send N messages (default 1)
    --queue=N     queue up N messages at a time (default 1)
    --flood       send all messages immediately


    --payload=S   use S as payload (default "hello, world!")
    --stdin       read payload from stdin, until EOF

Usage: dbus-echo [OPTIONS]

Respond to all method calls with an empty reply.

Options:

    --name=NAME   claim this well-known name first

    --session     use the session bus (default)
    --system      use the system bus
Comment 1 Simon McVittie 2011-02-14 11:08:45 UTC
Created attachment 43349 [details] [review]
proposed initial implementation
Comment 2 Simon McVittie 2011-02-25 08:53:01 UTC
Created attachment 43810 [details] [review]
[patch v2] Add dbus-spam, dbus-echo tools
Comment 3 Colin Walters 2011-03-24 08:43:33 UTC
Comment on attachment 43810 [details] [review]
[patch v2] Add dbus-spam, dbus-echo tools

=
>-bin_PROGRAMS=dbus-launch dbus-send dbus-monitor $(extra_bin_programs)
>+bin_PROGRAMS = \
>+	dbus-launch \
>+	dbus-send \
>+	dbus-monitor \
>+	dbus-echo \
>+	dbus-spam \

So, my feelings on this are similar to the Stats interface; basically we're installing them by default, but they're only useful for development/benchmarking (not even debugging in this case; I can't think why you'd use either to debug).

I grant you dbus-monitor is installed by default too, but it was sort of a selling point, and anyways it's just one tool.

We could just put them up on the wiki, or add a separate git repository of dbus-tools (what about putting them in bustle?).

Failing all that, I think I'd prefer a single dbus-debug-tool or something which took --echo-service and --spam arguments (and similar for any other future things people think of), rather than someone running Yocto having to guess at which binaries they can remove to save space.

This may not be too annoying to implement if you agree; just rename the main() to main_spam() and main_echo, and hook off the first arg for which to call.

>+  if (reply == NULL)
>+    {
>+      fprintf (stderr, "OOM allocating reply!\n");
>+      exit (1);
>+    }

Copy your oom() function from the other tool?

>+static void
>+pc_notify (DBusPendingCall *pc,
>+           void            *data)
>+{
>+  int *received_p = data;
>+
>+  VERBOSE (stderr, "received message #%d\n", *received_p);
>+
>+  (*received_p)++;

Hm, do we have to unref the pending call here?

>+            {
>+              DBusPendingCall *pc;
>+
>+              if (!dbus_connection_send_with_reply (connection,
>+                                                    message,
>+                                                    &pc,
>+                                                    0x7FFFFFFF))

INT_MAX?
Comment 4 Simon McVittie 2011-06-22 11:07:08 UTC
(In reply to comment #3)
> We could just put them up on the wiki, or add a separate git repository of
> dbus-tools (what about putting them in bustle?).

I see these as useful for profiling/performance testing, more than anything else; I think it's valuable to have small extra stuff like this in the dbus source tree and compiled by default, so they'll get compiled/packaged/ported/looked at, and to encourage people not to invent their own performance test-cases using various bindings.

http://alban.apinc.org/blog/2010/09/15/d-bus-in-the-kernel-faster/ links to a similar thing written by Alban (using dbus-glib, so not ideal for inclusion here). test/core/test-profile.c in dbus-glib might be useful, although again it uses dbus-glib. I'm also aware of at least one similar thing for which source isn't available, which I'd like to make obsolete :-)

Having said that, this tool only uses the shared-library interface, so it doesn't strictly need to be in the main dbus repository.

> Failing all that, I think I'd prefer a single dbus-debug-tool or something
> which took --echo-service and --spam arguments (and similar for any other
> future things people think of)

That's a good idea, I've done that. "dbus-test-tool echo --session" does what "dbus-echo --session" used to do, and so on.

> Copy your oom() function from the other tool?

Even better: I moved it into common code, tool_oom().

> >+static void
> >+pc_notify (DBusPendingCall *pc,
> ...
> Hm, do we have to unref the pending call here?

No, dbus-spam releases its ref as soon as the notify callback has been attached. The DBusConnection holds another ref from the time the pending call is attached until after the notify callback has been called.

> >+                                                    0x7FFFFFFF))
> 
> INT_MAX?

No, we specifically need INT32_MAX; but since I originally wrote these tools I added DBUS_TIMEOUT_INFINITE, so we can use that now.
Comment 5 Simon McVittie 2011-06-22 11:08:38 UTC
Created attachment 48295 [details] [review]
[PATCH 2/4] dbus-spam: add support for sending pseudorandomly-sized messages
Comment 6 Simon McVittie 2011-06-22 11:08:59 UTC
Created attachment 48296 [details] [review]
[PATCH 3/4] dbus-echo, dbus-spam: merge into a single binary, dbus-test-tool

This is installed by default, but easy to filter out for embedded systems
or whatever.
Comment 7 Simon McVittie 2011-06-22 11:09:27 UTC
Created attachment 48297 [details] [review]
[PATCH 4/4] dbus-spam: use the DBUS_TIMEOUT_INFINITE constant now that it exists
Comment 8 Will Thompson 2011-08-05 05:43:28 UTC
Created attachment 49953 [details] [review]
dbus-spam: fix --random-size to not clobber argument parsing

'i' is used to iterate across command-line arguments: unless there
happen to be exactly the right number of body lengths, re-using it to
iterate whitespace-separated integers from stdin is not going to work.
:)
Comment 9 Will Thompson 2011-08-05 05:54:13 UTC
Created attachment 49954 [details] [review]
test-spam: don't try to demarshal a NULL buffer

Previously, --message-stdin would hit an assertion immediately, because
payload was initialized to NULL and never set.
Comment 10 Simon McVittie 2011-08-05 06:45:22 UTC
(In reply to comment #8)
> dbus-spam: fix --random-size to not clobber argument parsing

(In reply to comment #9)
> test-spam: don't try to demarshal a NULL buffer

Both r+ from me.
Comment 11 Simon McVittie 2011-09-26 03:07:07 UTC
Created attachment 51612 [details] [review]
add dbus-test-tool (cumulative)

Cumulative version, with Makefile.am rebased onto master. Making this non-buggy also requires Attachment #49953 [details] and Attachment #49954 [details] to be applied.

Opinions?
Comment 12 Simon McVittie 2011-09-26 03:18:16 UTC
Created attachment 51613 [details] [review]
dbus-echo, dbus-spam: remove unused variables
Comment 13 Simon McVittie 2011-09-26 03:18:37 UTC
Created attachment 51614 [details] [review]
dbus-spam: use random seed as intended
Comment 14 Alban Crequy 2014-07-02 11:26:47 UTC
Comment on attachment 51612 [details] [review]
add dbus-test-tool (cumulative)

Review of attachment 51612 [details] [review]:
-----------------------------------------------------------------

::: tools/dbus-echo.c
@@ +84,5 @@
> +typedef enum {
> +    CONN_WELL_KNOWN,
> +    CONN_BUS,
> +    CONN_RAW
> +} ConnectionMode;

ConnectionMode is not used.

::: tools/dbus-spam.c
@@ +74,5 @@
> +           void            *data)
> +{
> +  int *received_p = data;
> +
> +  VERBOSE (stderr, "received message #%d\n", *received_p);

Please say something when the received message is an error. Maybe something like:

DBusMessage message = dbus_pending_call_steal_reply (pc)
if (dbus_message_is_error (message))
  {
    /* print something... */
  }
dbus_message_unref (message);


When I use --dest with a mistake, I get (according to strace):
org.freedesktop.DBus.Error.ServiceUnknown The name org.foo.bar was not provided by any .service files

I would like to know when the message was not forwarded to the intended service.
Comment 15 Alban Crequy 2014-07-02 15:49:14 UTC
Created attachment 102143 [details] [review]
[PATCH 1/7] Add dbus-test-tool, currently with "echo" and "spam" modes

Update Simon's patch with the changes mentioned in Comment #14.
Comment 16 Alban Crequy 2014-07-02 15:50:48 UTC
Created attachment 102144 [details] [review]
[PATCH 6/7] dbus-test-tool echo: add --no-reply and --no-listen
Comment 17 Alban Crequy 2014-07-02 15:51:38 UTC
Created attachment 102145 [details] [review]
[PATCH 7/7] dbus-test-tool spam: add --new-conn
Comment 18 Alban Crequy 2014-09-10 13:18:06 UTC
For an easier review, here is the branch I am using with the 7 patches:

http://cgit.collabora.com/git/user/alban/dbus/log/?h=message-spamming-tool-v2
Comment 19 Simon McVittie 2014-09-10 13:30:03 UTC
Comment on attachment 102143 [details] [review]
[PATCH 1/7] Add dbus-test-tool, currently with "echo" and "spam" modes

Review of attachment 102143 [details] [review]:
-----------------------------------------------------------------

::: tools/dbus-echo.c
@@ +1,5 @@
> +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
> +/* dbus-echo.c - a plain libdbus echo server
> + *
> + * Copyright © 2003 Philip Blundell <philb@gnu.org>
> + * Copyright © 2011 Nokia Corporation

Presumably also © 2014 Collabora Ltd., or whoever you wrote this for (if different).

::: tools/dbus-spam.c
@@ +84,5 @@
> +  if (dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_ERROR)
> +    {
> +      dbus_set_error_from_message (&error, message);
> +      fprintf (stderr, "Failed to receive reply #%d: %s: %s\n", *received_p,
> +               error.name, error.message);

Design review:

My original idea was that this is a bit like XMPP Ping <xmpp.org/extensions/xep-0199.html>: you can ping any service, and it doesn't matter whether we get a successful reply, a "no such method" error, a "permission denied" error, or what, as long as we get *something*. All we use the reply for is to schedule when to send the next message, if not using --flood or --no-reply.

I'd be OK with reporting errors by default, because that's fine if your spam victim is "dbus-test-tool echo" or you're using a correctly crafted template message; but if you're going to do this, I'd like an --ignore-errors flag that would silence it.

Functional review:

You're leaking @error. Free it.

@@ +197,5 @@
> +      else if (strcmp (arg, "--message-stdin") == 0)
> +        {
> +          consume_stdin (&payload_buf, &payload_len);
> +
> +          template = dbus_message_demarshal (payload, payload_len, &error);

Needs Attachment #49954 [details] for correctness.

@@ +234,5 @@
> +
> +          if (random_sizes == NULL)
> +            tool_oom ("allocating array of message lengths");
> +
> +          for (p = payload_buf, i = 0;

Needs Attachment #49953 [details] for correctness.

I would be inclined to squash this and the four patches that correct it into one big patch and say "From: Alban Crequy" and "based on earlier work by Simon McVittie and Will Thompson", tbh.

::: tools/tool-common.h
@@ +25,5 @@
> +
> +#include <dbus/dbus.h>
> +
> +#if 1
> +#define VERBOSE fprintf

I feel as though this should be #if 0. The spamming tool is not going to be as spammy as it could be if it spends time writing to stdout every time it sends a message, which somewhat defeats the point :-)
Comment 20 Simon McVittie 2014-09-10 13:39:00 UTC
Comment on attachment 102144 [details] [review]
[PATCH 6/7] dbus-test-tool echo: add --no-reply and --no-listen

Review of attachment 102144 [details] [review]:
-----------------------------------------------------------------

Good in principle, but...

::: tools/dbus-echo.c
@@ +49,5 @@
>             "    --name=NAME   claim this well-known name first\n"
>             "\n"
>             "    --sleep=N     sleep N milliseconds before sending each reply\n"
> +           "    --no-reply    discard received request without replying\n"
> +           "    --no-listen   don't listen on the D-Bus socket\n"

If you don't reply, you can't really claim to be an echo service any more.

In other words, --no-reply mode should not be "dbus-test-tool echo", this should be a new "dbus-test-tool black-hole" or "dbus-test-tool absorb" or something. It can be implemented in the same file if you want.

Similarly, "dbus-test-tool echo" should not accept a --no-listen" option, but the new mode may.

When D-Bus people say "listen" I immediately think listen(), which this is not; so I would prefer "dbus-test-tool black-hole --no-listen" to be renamed to "dbus-test-tool black-hole --no-dispatch".
Comment 21 Simon McVittie 2014-09-10 14:18:41 UTC
Comment on attachment 102145 [details] [review]
[PATCH 7/7] dbus-test-tool spam: add --new-conn

Review of attachment 102145 [details] [review]:
-----------------------------------------------------------------

::: tools/dbus-spam.c
@@ +342,5 @@
>  
>    while (no_reply ? sent < count : received < count)
>      {
> +      if (new_conn && sent < count &&
> +          (queue_len == -1 || sent < queue_len + received))

This logic is a bit of a tangle, and is handling queue_len != 1 even though you've documented that that is impossible. I haven't yet verified that it is correct.

I feel as though it ought to be possible to generalize new_conn to "after sending messages_per_conn, wait for the pending replies if any, then reconnect", with the constraint that queue_len > messages_per_conn is not useful. It might end up clearer like that, too.

I also feel as though changing the "shape" from

if (!new_conn)
  connect

while (...)
  {
    if (new_conn)
      connect;

    if (new_conn)
      disconnect;
  }

if (!new_conn)
  disconnect

to

connect

while (...)
  {
    if (new_conn && some condition)
      {
        disconnect;
        connect;
      }
  }

disconnect

might help clarity. I'm not sure.
Comment 22 Alban Crequy 2014-10-01 16:50:35 UTC
Created attachment 107198 [details] [review]
[PATCH 1/3] Add dbus-test-tool, currently with "echo" and "spam" modes

Fixes from the review on Comment #19.
Comment 23 Alban Crequy 2014-10-01 16:51:25 UTC
Created attachment 107199 [details] [review]
[PATCH 2/3] dbus-test-tool spam: add --messages-per-conn=N

Fixes from the review on Comment #21.
Comment 24 Alban Crequy 2014-10-01 16:52:15 UTC
Created attachment 107200 [details] [review]
[PATCH 3/3] dbus-test-tool: add black-hole mode

Fixes from the review on Comment #20.
Comment 25 Simon McVittie 2014-10-14 13:07:26 UTC
Merged for 1.9.2, thanks for your work on this


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.