Bug 92074 - destination= rules don't match anything when using BecomeMonitor
Summary: destination= rules don't match anything when using BecomeMonitor
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords: patch, regression
Depends on:
Blocks:
 
Reported: 2015-09-22 13:37 UTC by Lars Uebernickel
Modified: 2018-04-26 12:15 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
monitor: use the addressed_recipient to select matches (21.87 KB, patch)
2015-11-06 14:56 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Uebernickel 2015-09-22 13:37:12 UTC
Passing a rule with a destination= field to BecomeMonitor() doesn't match anything. For example,

  dbus-monitor destination=org.freedesktop.Notifications

doesn't print any calls on the notifications interface, but dbus-monitor without match rules does.

Calling AddMatch() with the same rule (and eavesdrop=true) works as expected.
Comment 1 Simon McVittie 2015-09-22 14:03:26 UTC
Are these messages actually sent to the destination "org.freedesktop.Notifications", or are they in fact sent to something like ":1.23" where :1.23 currently owns org.freedesktop.Notifications?
Comment 2 Lars Uebernickel 2015-09-22 14:10:49 UTC
(In reply to Simon McVittie from comment #1)
> Are these messages actually sent to the destination
> "org.freedesktop.Notifications", or are they in fact sent to something like
> ":1.23" where :1.23 currently owns org.freedesktop.Notifications?

In this particular case they were actually sent to the unique name, but the same problems occurs when I try to match on the unique name, and also when I send a message to the well-known name directly. 

However, it was my understanding that matching a well-known name matches messages to its owner as well. It definitely does with AddMatch().
Comment 3 Simon McVittie 2015-11-06 14:56:10 UTC
Created attachment 119441 [details] [review]
monitor: use the addressed_recipient to select matches

This means we respect the destination keyword in arguments to
BecomeMonitor.

In bus_dispatch(), this means that we need to defer capturing until
we have decided whether there is an addressed recipient; so instead
of capturing once, we capture at each leaf of the decision tree.

---

Intended for dbus-1.10. It's a little unfortunate that it introduces so many similar capture calls, but I think that's unavoidable.
Comment 4 Simon McVittie 2015-11-07 10:31:11 UTC
ssh://people.freedesktop.org/~smcv/dbus monitor-destination-92074
http://cgit.freedesktop.org/~smcv/dbus/log/?h=monitor-destination-92074
Comment 5 Philip Withnall 2015-11-07 11:03:04 UTC
Comment on attachment 119441 [details] [review]
monitor: use the addressed_recipient to select matches

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

I can't see anything obviously wrong here, but it would take me a long time to get familiar enough with this code to be entirely sure this is all correct and nothing's missing. Weak r+; seek further review.
Comment 6 Lars Uebernickel 2015-11-17 11:38:33 UTC
Your patch fixes the bug and looks good to me (though I'm in a similar situation as Philip with regards to knowing the code).

Thanks!
Comment 7 Simon McVittie 2015-11-17 15:00:53 UTC
(In reply to Philip Withnall from comment #5)
> Weak r+; seek further review.

(In reply to Lars Uebernickel from comment #6)
> Your patch fixes the bug and looks good to me (though I'm in a similar
> situation as Philip with regards to knowing the code).

I think the safest thing to do is to release 1.10.4 without this fix and a development release 1.11.0 with it, then backport to 1.10.x when either another maintainer can review it, or people have had more chance to test 1.11.0.
Comment 8 Simon McVittie 2016-02-08 13:58:54 UTC
(In reply to Simon McVittie from comment #7)
> I think the safest thing to do is to release 1.10.4 without this fix and a
> development release 1.11.0 with it

I did that.

Jonny, I hear you're looking at monitors: perhaps you could double-check this?
Comment 9 Simon McVittie 2017-09-25 16:08:45 UTC
I think this has been in master long enough to be reasonably confident about adding it to 1.10. Applied for 1.10.24.

For the record, the original fix was indeed in 1.11.0.


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.