Bug 39140

Created attachment 49009 [details] [review]
Add eavesdrop=true to bustle-dbus-monitor.c

Same fix dbus-monitor.c received in Bug #37890 fix.

Just found a bug in the patch, I'll submit a new one ASAP

Created attachment 49012 [details] [review]
add eavesdrop=true when supported

It checks for support adding the rule once, and then selects what to use depending on AddMatch error status.

From the prev. patch, it fixes the case with no filter passed at command line, adding the rule also under this case.

Created attachment 49013 [details] [review]
handle OOM

Since I was working on it and I fixed the same problem in dbus-monitor.

Created attachment 49014 [details] [review]
filters needs to be freed

It also seems that filters need to be freed as well.

Review of attachment 49012 [details] [review]:

::: bustle-dbus-monitor.c
@@ +369,3 @@
 
+  /* check support for eavesdrop=true in filter */
+  dbus_bus_add_match (connection, EAVESDROPPING_RULE, &error);

This is an interesting way to check for support, but it has the flaw that bustle will receive any messages between this temporary match being added, and the same match being removed, even if they shouldn't have matched.

One way to make this not a practical problem would be to use "eavesdrop='true',destination=':1.23'" (replacing :1.23 with our own unique name), which only matches messages that we ought to be receiving anyway.

However, I'd personally do this:

- provide a function bustle_add_match() and use it to replace
  all calls to dbus_bus_add_match()

- in that function:

  - have a static variable eavesdrop_support taking values
    EAVESDROP_SUPPORTED, EAVESDROP_UNSUPPORTED, EAVESDROP_UNKNOWN
    or something

  - first try prepending "eavesdrop='true'," and using that,
    unless we already know it's not supported

  - if that succeeds, set EAVESDROP_SUPPORTED

  - if that fails with MatchRuleInvalid and EAVESDROP_SUPPORTED
    is not set, try again without the prefix; if that works,
    set EAVESDROP_UNSUPPORTED

  - on any other failure, just fail

@@ +375,3 @@
+      /* make it re-usable */
+      dbus_error_free (&error);
+      dbus_error_init (&error);

dbus_error_free is enough: its documentation says that it "reinitializes the error as in dbus_error_init".

@@ +377,3 @@
+      dbus_error_init (&error);
+
+      /* using a dbus version witout this feature, too bad, but still OK */

"without"

@@ +392,3 @@
+      if (dbus_error_is_set (&error))
+        {
+          fprintf (stderr, "Failed to removed rule \"%s\": %s\n",

"Failed to remove rule"

Review of attachment 49013 [details] [review]:

Looks good to me (although I'm not wjt)

Review of attachment 49014 [details] [review]:

Looks fine, although leaking O(1) memory isn't really a problem.

Created attachment 49396 [details] [review]
OOM

Created attachment 49397 [details] [review]
Add eavesdrop=true when supported

Use bustle_add_match() as Simon suggested

Created attachment 49398 [details] [review]
Free filters too

useless but since has been spotted... :)

Merged to master:

  http://cgit.freedesktop.org/bustle/commit/?id=5cbf70f
  http://cgit.freedesktop.org/bustle/commit/?id=c95d0ce
  http://cgit.freedesktop.org/bustle/commit/?id=ea29b55

And I added an equivalent fix to the new pcap-based logger, which is the standard one henceforth:

  http://cgit.freedesktop.org/bustle/commit/?id=69e7e63

I haven't got dbus 1.5.x on this system; Cosimo, could you cast your eye over it to see if you think it's right? I'm pretty sure it should work, from reading the logs. The fallback certainly works. :)

Anyway, these fixes will be included in Bustle 0.3.2. Thanks!