Bug 37890

> <davidz> he wants to emit a signal to a unique name, say :1.42

Signals don't have destinations. The fact that the reference implementation allows this is a bug. It should deliver the signal to all clients with matching rules.

Can we get the actual match rules and example messages in here?  Is it like:

match type='signal',interface='org.gtk.Private.GPhoto2VolumeMonitor'

and then gvfs sends out a signal with that interface and destination of 1.42, but dbus says "hey, this other client has a match rule and eavesdropping is globally enabled, so i'll also send it to 1.43"?

I think that makes sense, and yeah, we should probably add an opt-in mechanism for eavesdropping.

However - clients can also work around this by adding destination=$self_unique_name in the match rules (yes, this bloats them and probably slows down the bus processing, but).

(In reply to comment #1)
> > <davidz> he wants to emit a signal to a unique name, say :1.42
> 
> Signals don't have destinations. 

Of course they do. Any D-Bus message can have the DESTINATION header field set. Furthermore, the spec even says "Signals normally do not specify a destination; they are sent to all applications with message matching rules that match the message."

> The fact that the reference implementation
> allows this is a bug. 

No. FWIW, there are valid use cases for this. For example, delivering a signal to another peer without having that peer subscribing to it via a match rule [1]. In fact, you _need_ this functionality to easily do certain things in a race-free way.

[1] : from the spec "The message bus must send messages (of any type) with the DESTINATION field set to the specified recipient, regardless of whether the recipient has set up a match rule matching the message."

> It should deliver the signal to all clients with matching
> rules.

No. The spec clearly says

 When the message bus receives a method call, if the DESTINATION
 field is absent, the call is taken to be a standard one-to-one
 message and interpreted by the message bus itself. For example,
 sending an org.freedesktop.DBus.Peer.Ping message with no
 DESTINATION will cause the message bus itself to reply to the
 ping immediately; the message bus will not make this message
 visible to other applications.

Either way, I'm not disputing how it works (it seems to be working correctly), I'm only complaining that we ship with eavesdropping turned off by default.

(In reply to comment #2)
> Can we get the actual match rules and example messages in here?  Is it like:
> 
> match type='signal',interface='org.gtk.Private.GPhoto2VolumeMonitor'

Essentially, yes - it's 

 type='signal',
 interface='org.gtk.Private.RemoteVolumeMonitor',
 sender='org.gtk.Private.GPhoto2VolumeMonitor',

> and then gvfs sends out a signal with that interface and destination of 1.42,
> but dbus says "hey, this other client has a match rule and eavesdropping is
> globally enabled, so i'll also send it to 1.43"?

Yes, that is what appears to be happening.

> I think that makes sense, and yeah, we should probably add an opt-in mechanism
> for eavesdropping.

I'm not saying it's *wrong* what the bus is doing. I'm just asking for eavesdropping to be turned off by default. OK, this is inconvenient so that's why I'm asking for a way to turn it on/off at runtime. So dbus-monitor(1) and Bustle can do that.

> However - clients can also work around this by adding
> destination=$self_unique_name in the match rules (yes, this bloats them and
> probably slows down the bus processing, but).

They can, yeah.

(In reply to comment #3)
> No. The spec clearly says
> 
>  When the message bus receives a method call, if the DESTINATION
>  field is absent, the call is taken to be a standard one-to-one
>  message and interpreted by the message bus itself. For example,
>  sending an org.freedesktop.DBus.Peer.Ping message with no
>  DESTINATION will cause the message bus itself to reply to the
>  ping immediately; the message bus will not make this message
>  visible to other applications.

I was quoting the wrong part of the spec here, sorry.

Btw, note that dbus-send(1) does not work (at least not the one from dbus-1.4.6-4.fc15.x86_64) - I used the program in [1] to reproduce

If you run

 ./emitsignal session ":1.3"

then you only see something with 'dbus-monitor --session' if eavesdropping is enabled.

[1] :

#include <gio/gio.h>

int
main (int argc, char *argv[])
{
  GDBusConnection *c;
  g_type_init ();
  c = g_bus_get_sync (g_strcmp0 (argv[1], "system") == 0 ? G_BUS_TYPE_SYSTEM : G_BUS_TYPE_SESSION, NULL, NULL);
  g_dbus_connection_emit_signal (c, argv[2],
                                 "/org/example/object",
                                 "org.example.iface",
                                 "Signal",
                                 g_variant_new("(s)", "foo"),
                                 NULL);
  g_dbus_connection_flush_sync (c, NULL, NULL);
  g_object_unref (c);
  return 0;
}

OK, researched this some more and

 1. This is not mentioned by the spec at all.

 2. It is mentioned in the dbus-daemon(1) man page - here the
    concept of "eavesdropping" is introduced as (basically)
    policy on how to deal with messages where the DESTINATION
    header is set

 3. The man page contradicts the code by saying "it [eavesdropping]
    does not apply to signals". We should fix the man page.

Again, not proposing any wild changes here. Just trying to explain.

I'd say then the behaviour is undefined. Nothing in the spec says that signals should have destinations. It says that they usually don't -- which would imply that they sometimes do.

But as a design principle, they shouldn't have destinations. Signals are broadcasts and should be received by all.

If we want signals to have destinations, let's specify that. Add to the spec, add a dbus_message_new_signal_with_destination that sets the destination when creating and the bindings should add a similar function.

Otherwise, I maintain that the current eavesdropping behaviour is the correct one: the destination set by the sender is ignored.

(In reply to comment #8)
> I'd say then the behaviour is undefined. Nothing in the spec says that signals
> should have destinations. It says that they usually don't -- which would imply
> that they sometimes do.
> 
> But as a design principle, they shouldn't have destinations. Signals are
> broadcasts and should be received by all.

As I said in comment 3 there are valid reasons for wanting to set the DESTINATION field. I'm not sure why you are ignoring that, it's not really helping the discussion.

> If we want signals to have destinations, let's specify that. Add to the spec,

No, need, the spec already allows signals with the DESTINATION header field set.

> add a dbus_message_new_signal_with_destination that sets the destination when
> creating and the bindings should add a similar function.

Uhm, no need for this, you can already use dbus_message_set_destination() on a DBusMessage created with dbus_message_new() or dbus_message_new_signal().

(Btw, if you didn't know, as a general libdbus design principle, things like dbus_message_new_signal() are only convenience constructors.)

> Otherwise, I maintain that the current eavesdropping behaviour is the correct
> one: the destination set by the sender is ignored.

I think you are confused: "destination set by the sender is ignored" is _not_ the current behavior. It is only the current behavior when eavesdropping is enabled. Which is how the session bus works. Notably, this is _not_ what happens on the system bus.

(In reply to comment #9)
> As I said in comment 3 there are valid reasons for wanting to set the
> DESTINATION field. I'm not sure why you are ignoring that, it's not really
> helping the discussion.

I'm not ignoring that. I'm saying that the spec doesn't require that and my reading is that it shouldn't have.

You mentioned twice now that there are reasons for having it. I haven't ever encountered one and, before today, I didn't know anyone even wanted it.

> > If we want signals to have destinations, let's specify that. Add to the spec,
> 
> No, need, the spec already allows signals with the DESTINATION header field
> set.

It doesn't say what we should do. You're seeing a lot of contradictions in the man page, the implementations, etc. because it doesn't specify completely. Don't treat this as a bug: treat this as the need to specify undefined/underdefined behaviour.

> > Otherwise, I maintain that the current eavesdropping behaviour is the correct
> > one: the destination set by the sender is ignored.
> 
> I think you are confused: "destination set by the sender is ignored" is _not_
> the current behavior. It is only the current behavior when eavesdropping is
> enabled. Which is how the session bus works. Notably, this is _not_ what
> happens on the system bus.

Then let's be 100% clear: write it in the spec. 

Another thing to be noted is that applications should not rely on the behaviour of eavesdropping being enabled or disabled. Most importantly, if we say that directed signals are permitted (rather than being undefined behaviour), then well-written applications that aren't meant to eavesdrop shouldn't be affected by the flag. Do you have a fix in mind for this?

(In reply to comment #10)
> (In reply to comment #9)
> > As I said in comment 3 there are valid reasons for wanting to set the
> > DESTINATION field. I'm not sure why you are ignoring that, it's not really
> > helping the discussion.
> 
> I'm not ignoring that. I'm saying that the spec doesn't require that and my
> reading is that it shouldn't have.

The spec says "Signals normally do not specify a destination [...]" which implies that they sometimes do. Additionally, the DESTINATION header field is allowed in a message for a signal. Additionally, the reference implementation specifically allows it and it works according to spec including the eavesdropping feature. I don't know how this can be much clearer.

> You mentioned twice now that there are reasons for having it. I haven't ever
> encountered one and, before today, I didn't know anyone even wanted it.

OK, two examples

1. As a non-snoopable transport mechanism - e.g. just like how it's suitable to include a password in method call parameters on the system bus [a] - no one can snoop it because eavesdropping is just not allowed on the system bus.

[a]: and we indeed have software doing this for unlocking e.g. LUKS encrypted block devices - you don't want uid 501 to snoop the password provided by uid 500

2. Consider a method

  Foo (IN String whatever, OUT ObjectPath some_path);

where the Foo() method creates an object that only the caller of Foo() cares about. Assume that signals on the created object has DESTINATION set to the unique name of whoever invoked Foo(). Since the spec says

  The message bus must send messages (of any type) with
  the DESTINATION field set to the specified recipient,
  regardless of whether the recipient has set up a match rule
  matching the message

then the app does not need to call AddMatch() to catch signals from the @some_path. Bingo, we just avoided a race condition.

Btw, this is not some far-fetched example - think of Foo() as creating a view (the created object) into some data. Or think of the created object as a Job object for tracking progress.

Additionally, since DESTINATION is set for all signals, we can include confidential data (e.g. data that cannot be snooped). Again, very handy on e.g. the system bus.
 
> > > If we want signals to have destinations, let's specify that. Add to the spec,
> > 
> > No, need, the spec already allows signals with the DESTINATION header field
> > set.
> 
> It doesn't say what we should do. You're seeing a lot of contradictions in the
> man page, the implementations, etc. because it doesn't specify completely.
> Don't treat this as a bug: treat this as the need to specify
> undefined/underdefined behaviour.

As I said, I don't think there are no bugs in the implementation! Just a bug in the man page (it should mention that signals are fine), plus some overhead which is what I like to remove.

I'm pretty sure Havoc had the two uses I sketched out above in mind. Of course something like that (confidentiality; avoiding race conditions) needs to be possible.

> Then let's be 100% clear: write it in the spec. 

I think the spec is fine - in my view it 100% allows signal messages to have a destination. Additionally, libdbus-1 allows it. And my own impl in GLib allows it too.

The spec *could* mention "eavesdropping" but then again it's probably also fine that it's just a dbus-daemon(1) implementation detail. I think the spec should say something like "some message bus implementations may prohibit eavesdropping" (of course defining what eavesdropping means) but nothing more than that. Maybe the system message bus section of the spec should mention that apps are guaranteed that non-privileged processes can eavesdrop. The thing is that we don't want the spec to limit too much how our code should work - it's about finding that balance.

> Another thing to be noted is that applications should not rely on the behaviour
> of eavesdropping being enabled or disabled. Most importantly, if we say that
> directed signals are permitted (rather than being undefined behaviour), then
> well-written applications that aren't meant to eavesdrop shouldn't be affected
> by the flag. Do you have a fix in mind for this?

If an app relies on eavesdropping, then, well, fix the app if it doesn't work when eavesdropping is turned off. I don't think any existing app has that problem though.

And I disagree it's currently "undefined behavior" - heck, the spec directly mentions that a signal message may have the DESTINATION field set... the bus allows it.. libdbus-1 allows it... gdbus has explicit support for it via the @destination parameter in g_dbus_connection_emit_signal().

So I still maintain you are not reading the spec correctly if you think it's "undefined behavior".... what do you think spec needs in order to make you read it as I do?

(In reply to comment #11)
> 1. As a non-snoopable transport mechanism - e.g. just like how it's suitable to
> include a password in method call parameters on the system bus [a] - no one can
> snoop it because eavesdropping is just not allowed on the system bus.
> 
> [a]: and we indeed have software doing this for unlocking e.g. LUKS encrypted
> block devices - you don't want uid 501 to snoop the password provided by uid
> 500

Ok, that is a valid use-case. Assuming that signals can have destinations, then signals can be used to transport confidential data. But that's a consequence, not a requirement: to be a requirement, you'd have to tell me why the data needs to be transported as a signal.

> 2. Consider a method
> 
>   Foo (IN String whatever, OUT ObjectPath some_path);
> 
> where the Foo() method creates an object that only the caller of Foo() cares
> about. Assume that signals on the created object has DESTINATION set to the
> unique name of whoever invoked Foo(). Since the spec says
> 
>   The message bus must send messages (of any type) with
>   the DESTINATION field set to the specified recipient,
>   regardless of whether the recipient has set up a match rule
>   matching the message
> 
> then the app does not need to call AddMatch() to catch signals from the
> @some_path. Bingo, we just avoided a race condition.

I see. You can't call AddMatch before Foo because you don't have the path yet. And you adding a match based on other properties (say, the interface) would be sub-optimal, since you may get signals that aren't meant for you and you'd have to ignore.

Ok, this is a valid use-case.

> > Then let's be 100% clear: write it in the spec. 
> 
> I think the spec is fine - in my view it 100% allows signal messages to have a
> destination. Additionally, libdbus-1 allows it. And my own impl in GLib allows
> it too.

And I'm telling you that I didn't read it like that. I have 12 years of experience with signals inside Qt and 5½ years of D-Bus and it never crossed my mind that they should be legal until yesterday. The API also suggests that they aren't meant to be used since dbus_message_new_signal doesn't have it as an optional parameter, unlike the g_dbus_connection_emit_signal counterpart of your comment 6.

So take this again as a suggestion: make it 100% clear.

> > Another thing to be noted is that applications should not rely on the behaviour
> > of eavesdropping being enabled or disabled. Most importantly, if we say that
> > directed signals are permitted (rather than being undefined behaviour), then
> > well-written applications that aren't meant to eavesdrop shouldn't be affected
> > by the flag. Do you have a fix in mind for this?
> 
> If an app relies on eavesdropping, then, well, fix the app if it doesn't work
> when eavesdropping is turned off. I don't think any existing app has that
> problem though.

I'm saying that well-written apps may have unintended side-effects due to eavesdropping being enabled. If we say that the default is "off" and that signals with destinations will be received by only one connection, then applications may be written to expect that the signals they receive are intended for it. After all, in the process of development, the developer of said application may only have seen that behaviour.

However, if eavesdropping happens to be on for whatever valid reason on someone else's workstation, applications may suddenly start receiving signals that aren't meant for it. Its match rules aren't meant to eavesdrop, they are meant to receive signal broadcasts, but suddenly non-broadcasts are received too. They may start mis-behaving because of this.

I am asking of you: do you have a suggestion on how to handle this?

> I am asking of you: do you have a suggestion on how to handle this?

Here's my suggestion: we make the regular match rules work as if eavesdropping were off, even if it's on in the configuration. In order to turn eavesdropping on in a rule, something special must be added.

For the signals case, we can change the specification to say that the absence of "destination=" means the default rules for destinations: the message is either a broadcast or directed at this connection, To receive messages to other destinations, applications would need to write "destination=*".

Or, another alternative, it's to add an "eavesdrop=yes" option to the match rule. This actually preferred, as it makes clear that eavesdropping was intended and dbus-monitor can simply prepend it to all rules it sends to the bus, without having to parse them. It's probably easier to implement too.

(In reply to comment #13)

> Or, another alternative, it's to add an "eavesdrop=yes" option to the match
> rule. This actually preferred, as it makes clear that eavesdropping was
> intended and dbus-monitor can simply prepend it to all rules it sends to the
> bus, without having to parse them. It's probably easier to implement too.

I like this suggestion.

(In reply to comment #2)
> However - clients can also work around this by adding
> destination=$self_unique_name in the match rules (yes, this bloats them and
> probably slows down the bus processing, but).

In the short term, I think GVFS should do that. If quantifiable benchmarks prove that it adds significant extra cost for the bus daemon, let us know. (I doubt it does - it's basically a single strcmp.)

(In reply to comment #13)
> Here's my suggestion: we make the regular match rules work as if eavesdropping
> were off, even if it's on in the configuration. In order to turn eavesdropping
> on in a rule, something special must be added.

For master, I do like this idea: it removes a lot of potential for shooting yourself in the foot, without removing the developer convenience of having eavesdropping available on your session bus. However, I don't think we can say that this is a compatible change with a straight face: it breaks existing behaviour (even if only debug tools like Bustle should have been using it).

I think this sort of minor breakage is OK in master (if announced properly in NEWS), but not in stable branches!

If we offer eavesdropping (in its current form) at all, I think it should continue to be allowed-by-default for the session bus, even if processes need to adjust their match rules to opt in.

Rationale:

* Having things behave differently in "debug mode" is Not Right; it
  leads to Heisenbugs. If developers are going to have eavesdropping
  switched on, at least while they debug something, and if it doesn't
  result in performance or security problems in "production" usage
  (dbus-monitor not running), then we should just leave it on all the time.

* The bus security policy is a security policy, not a performance policy;
  the session bus isn't a security boundary (except on specialized
  platforms like OLPC and Maemo, which get to keep both pieces tbh),
  and as such, it has no security policy.

* Applications can already set up match rules to get optimal performance
  (in terms of avoiding undesired messages - although fewer match rules can
  probably also be an improvement), so the only performance problem is that
  the most obvious way to add a match rule (omitting destination) is secretly
  not the best; but if we add an eavesdrop='yes' switch on match rules,
  we've fixed that.

Cosimo is working on a patch for 1.5, to implement Thiago's suggestion.

Created attachment 48962 [details] [review]
Add eavesdrop=true|false to match rules.

Add "eavesdrop=true|false" to match rules, implementing Thiago's suggestion on Comment #13

The patch acts on the rule structure, adding a flag.

One issue I see with this approach is that the added flag is actually a meta information about how the rule should act (ie it rules about the rule), but it's expressed within the same rule.

Created attachment 48963 [details] [review]
Enables dbus-monitor to automatically use eavesdrop=true

Dbus-monitor wants to eavesdrop (by definition of monitor).

In the exceptional case the user does not want to eavesdrop, he/she can always use eavesdrop=false in the passed filter, which will disable it.
(collateral effect for this feature: a match rule is allowed to have any number of "eavesdrop" keyword occurrences)

Implementation-wise this looks fine, just some nitpicking:

> +      else if (strcmp (key, "eavesdrop") == 0)
> +        {
> +          /* do not detect "eavesdrop" being used more than once in rule:
> ...
> +          _dbus_assert ((rule->flags & BUS_MATCH_CLIENT_IS_EAVESDROPPING) == FALSE);

The assertion contradicts the comment: please remove it.

(Also, (rule->flags & ANY_FLAG) is flag-valued, not boolean-valued, so
comparing to 0 would have been clearer. Conceptually, it takes values in the
set { 0, BUS_MATCH_CLIENT_IS_EAVESDROPPING }, rather than { FALSE, TRUE }.)

> +                              "Eavesdropper client value '%s' is invalid\n",

A better message might be "eavesdrop='%s' is invalid, should be true or false"
or something?

> +  /* BUS_MATCH_CLIENT_IS_EAVESDROPPING: we already matched flags */
> +

I don't think this comment really adds anything?

> +void bus_match_rule_set_client_is_eavesdropping (BusMatchRule     *rule,
> +                                                 dbus_bool_t is_eavesdropping);

Couldn't this be static?

> @@ -50,6 +52,7 @@
> #define _IONBF 0x04
> #endif
>
>+
> void
> GetSystemTimeAsFileTime (LPFILETIME ftp)
> {

Unnecessary whitespace change

> +        /* append a rule (and a comma) to enable the monitor to eavesdrop */

You're prepending, not appending, so please change the comment.

(It is correct and significant that you prepend, since it lets the rule
itself override eavesdrop back to false - I think that might deserve
mentioning.)

> +     filters = (char **)realloc(filters, numFilters * sizeof(char *));
> +     filters[j] = (char *)malloc(filter_len * sizeof(char *));

Pre-existing bug, but you should do something about malloc/realloc failures.
Something simple like

    if (filters == NULL)
      oom ();

(where oom() just complains to stderr and calls exit (1)) would be enough.

Pre-existing bug, but I'd prefer the lines you've touched to have this
spacing:

    (char **) realloc (filters, numFilters * sizeof (char *));

and be indented with spaces instead of tabs. (General principle: if the line
is going to be changed anyway, you might as well make it look nice.)

The not quoted suggestion have been fixed.

(In reply to comment #20)
> > +  /* BUS_MATCH_CLIENT_IS_EAVESDROPPING: we already matched flags */
> I don't think this comment really adds anything?

I usually like to leave similar hints for future code readers/bug hunters, so they won't waste time to understand why that specific flag has not been checked explicitly: "Wait, there is a flag not checked..... oh, that's why".

No problem to remove it, though.

> > +void bus_match_rule_set_client_is_eavesdropping (BusMatchRule     *rule,
> > +                                                 dbus_bool_t is_eavesdropping);
> 
> Couldn't this be static?

It can, other BusMatchRule methods are public and I assumed that it's to allow  someone to create a rule using the given setters instead of passing a string.
If it's not the case, it should be static, I agree.

Currently it's used only in bus/signal.c

> Pre-existing bug, but I'd prefer the lines you've touched to have this
> spacing:
> 
>     (char **) realloc (filters, numFilters * sizeof (char *));
> 
> and be indented with spaces instead of tabs. (General principle: if the line
> is going to be changed anyway, you might as well make it look nice.)


done, I'll leave the rest of the previous "if" branches untouched.
Let me know if I'm supposed to fix them as well. Tabs have been used massively in dbus-monitor.c.

Created attachment 48971 [details]
[test] first part of the test

code mostly copied from example-emitter/receiver.

The emitter emits three signals:
- broadcast
- unicast to the emitter itself (ie not client should be an actual destination for this signal, but the emitter itself)
- unicast to the caller of the method triggering the signal emission

this way, using the receiver we can test that:
- broadcast messages are always received either with eavesdrop=true or eavesdrop=false
- unicast signals are received by the rule owner (the method caller) when eavesdrop=false (default case for non-monitors)
- unicast signals are not received by non rule owners (the other unicast msg sent to <self>)


then using a patched version of dbus-monitor we can see:
- that by default (eavesdrop=true) dbus-monitor will see the three signals
- if eavesdrop=false is added to the rule, only broadcast msg is seen, and the other two will be not received, since dbus-monitor is not the rule owner for any of the rules associated with the emitted signals

for testing purposes, the old (non-patched) version of dbus-monitor used against a patched dbus-daemon will act as eavesdrop=false is passed.

Created attachment 48972 [details]
[test] second part of the test

against a session of a patched dbus-daemon
$ python ./test_emitter &

on two other another terminals using a patched version of dbus-monitor:

$ dbus-monitor member=HelloSignal
signal sender=org.freedesktop.DBus -> dest=:1.159 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.159"
signal sender=:1.158 -> dest=(null destination) serial=4 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello broadcasted"
signal sender=:1.158 -> dest=com.example.TestService serial=5 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello for com.example.TestService (unicast)"
signal sender=:1.158 -> dest=:1.160 serial=6 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello for :1.160 (unicast)"






$ dbus-monitor member=HelloSignal,eavesdrop=false
signal sender=org.freedesktop.DBus -> dest=:1.161 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.161"
signal sender=:1.162 -> dest=(null destination) serial=4 path=/com/example/TestService/object; interface=com.example.TestService; member=HelloSignal
   string "s"
   string "hello broadcasted"


The monitor output will be produced when the received is called:

$ python test_receiver.py 
Received a hello signal for 'None' and it says: hello broadcasted
Received signal (by connecting using remote object) for 'None' and it says: hello broadcasted
Received a hello signal for ':1.163' and it says: hello for :1.163 (unicast)
Received signal (by connecting using remote object) for ':1.163' and it says: hello for :1.163 (unicast)
Signal emitted

(In reply to comment #21)
> The not quoted suggestion have been fixed.
> 
> (In reply to comment #20)
> > > +  /* BUS_MATCH_CLIENT_IS_EAVESDROPPING: we already matched flags */
> > I don't think this comment really adds anything?
> 
> I usually like to leave similar hints for future code readers/bug hunters, so
> they won't waste time to understand why that specific flag has not been checked
> explicitly: "Wait, there is a flag not checked..... oh, that's why".

I'd prefer something like this, then:

    /* we already compared the value of flags, and
     * BUS_MATCH_CLIENT_IS_EAVESDROPPING does not have another struct member */

> > > +void bus_match_rule_set_client_is_eavesdropping (BusMatchRule     *rule,
> > > +                                                 dbus_bool_t is_eavesdropping);
> > 
> > Couldn't this be static?
> 
> It can, other BusMatchRule methods are public

*shrug* leave it, then.

> done, I'll leave the rest of the previous "if" branches untouched.

That's fine, I'm only asking you to fix whitespace in lines you're already changing :-)

Created attachment 48975 [details] [review]
Add eavesdrop=true|false to match rules

Fixed patch for addeavesdrop=true|false, obsoleting attachment #48962 [details] [review]

Created attachment 48976 [details] [review]
Add eavesdrop=true to dbus-monitor

fixed patch for adding eavesdrop=false to dbus-monitor, obsoletes attachment #48963 [details] [review]

Comment on attachment 48975 [details] [review]
Add eavesdrop=true|false to match rules

>+              dbus_set_error (error, DBUS_ERROR_MATCH_RULE_INVALID,
>+                              "'eavesdropper=%s' is invalid, "
>+                              "it should be 'true' or 'false'\n",

Nitpicking: s/eavesdropper/eavesdrop/

(An eavesdropper is a person or thing that eavesdrops.)

(In reply to comment #25)
> Add eavesdrop=true|false to match rules

Applied, with the typo fix I suggested. 1.5.6.

(In reply to comment #26)
> Add eavesdrop=true to dbus-monitor

Applied. 1.5.6.

Leaving this bug open while Cosimo works on an automated test.

Created attachment 49015 [details] [review]
handle no filter at cmdline case

I forgot to handle the case on which no filters are passed at command line.

Here's the fix.

(In reply to comment #29)
> handle no filter at cmdline case

Oops, yes. Also merged.

Created attachment 49076 [details] [review]
Add autometed tests for eavesdrop=true|false

Review of attachment 49076 [details] [review]:

Some typos and stuff, but I'm going to merge it anyway, because test > no test :-)

::: test/dbus-daemon-eavesdrop.c
@@ +49,3 @@
+
+/* This rule is equivalend to the one added to a proxy connecting to
+ * SENDER_NAME+SENDER_IFACE, plus restructubg on signal name.

"equivalent", "added by a proxy", "restricting"

@@ +64,3 @@
+/* a connection received a signal to whom? */
+typedef enum {
+  UNKNOWN,

From its usage, NONE_YET would be a more accurate description?

@@ +244,3 @@
+}
+
+/* Send special broadcast signal to indicate that conncetion can "stop"

connection

@@ +246,3 @@
+/* Send special broadcast signal to indicate that conncetion can "stop"
+ * listening and check their results.
+ * DBus does not re-order msgs, so when the three connection will receive this

Please don't abbreviate unnecessarily: "messages"

Grammar: when the three connections have received this... message sent before it

@@ +269,3 @@
+
+/* Ignore NameAcquired, then depending on the signal received:
+ * - updateds f-><conn>_dst based on the destination of the msg

updates, message

@@ +327,3 @@
+    }
+
+  return DBUS_HANDLER_RESULT_HANDLED;

It doesn't actually matter here, but in general signals should be NOT_YET_HANDLED

@@ +502,3 @@
+
+
+  if (f->receiver != NULL)

Unnecessary double blank line just above here

Fixed in git for 1.5.6; incompatible change, so should not be fixed in 1.4