|
Description
Simon McVittie
2011-02-10 09:01:35 UTC
Created attachment 43349 [details] [review] proposed initial implementation Created attachment 43810 [details] [review] [patch v2] Add dbus-spam, dbus-echo tools 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? (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. Created attachment 48295 [details] [review] [PATCH 2/4] dbus-spam: add support for sending pseudorandomly-sized messages 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. Created attachment 48297 [details] [review] [PATCH 4/4] dbus-spam: use the DBUS_TIMEOUT_INFINITE constant now that it exists 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. :) 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. (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. 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? Created attachment 51613 [details] [review] dbus-echo, dbus-spam: remove unused variables Created attachment 51614 [details] [review] dbus-spam: use random seed as intended 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. 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. Created attachment 102144 [details] [review] [PATCH 6/7] dbus-test-tool echo: add --no-reply and --no-listen Created attachment 102145 [details] [review] [PATCH 7/7] dbus-test-tool spam: add --new-conn 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 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 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 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. 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. Created attachment 107199 [details] [review] [PATCH 2/3] dbus-test-tool spam: add --messages-per-conn=N Fixes from the review on Comment #21. Created attachment 107200 [details] [review] [PATCH 3/3] dbus-test-tool: add black-hole mode Fixes from the review on Comment #20. 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.