Currently it silently accepts both --session and --system, and just picks the last one. This tripped someone here up today!
Good in principle, but: > +static void > +only_one_type (dbus_bool_t *seen_bus_type, > + char *name) > +{ > + if (*seen_bus_type) > + { > + fprintf (stderr, "I only support monitoring one bus at a time!\n"); > + usage (name, 1); > + } @name here ought to be "dbus-monitor" or "/usr/bin/dbus-monitor" or something... > if (!strcmp (arg, "--system")) > - type = DBUS_BUS_SYSTEM; > + { > + only_one_type (&seen_bus_type, arg); ... so it should be argv[0] here. With the patch as-is, you'll print Usage: --session [--blah|--blah|--blah] instead of the intended Usage: dbus-monitor [--blah|--blah|--blah] Apart from that it looks fine, though.
Created attachment 45536 [details] [review] minotaur: bail if asked to monitor >1 bus A coworker was just tripped up by `dbus-monitor --session --system` only monitoring the system bus. This patch would have saved him reproducing a tricky bug several times! Fixes: <https://bugs.freedesktop.org/show_bug.cgi?id=26548>
Review of attachment 45536 [details] [review]: ::: tools/dbus-monitor.c @@ +222,3 @@ + if (*seen_bus_type) + { + fprintf (stderr, "I only support monitoring one bus at a time!\n"); I think we should avoid first person colloquial for error messages. How about: "error: Multiple bus or address specifications given"
Created attachment 45571 [details] [review] monitor: bail if asked to monitor >1 bus Fixes: <https://bugs.freedesktop.org/show_bug.cgi?id=26548>
Review of attachment 45571 [details] [review]: Looks good, thanks.
Merged for 1.4.10, 1.5.2
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.