Created attachment 21749 [details] [review]
don't send unrequested replies

I don't think I'm the right person to review this, but since mbiebl asked me to look at it...

The variable name "use_thread" is completely untrue. "A" in the method data stands for asynchronous - "use_thread" would be better named as "reply_later" or the current "call_only". dbus-glib never invents extra threads.

Methods with "A" in the method data use a different calling convention, where the method implementation takes an extra parameter (a pointer to a DBusGMethodInvocation) and returns nothing. Instead of the return value from the method implementation being used to send the reply, the reply is only sent when a reply function (dbus_g_method_return() or dbus_g_method_return_error(), iirc) is called on the DBusGMethodInvocation (this can be done before or after returning from the method implementation).

As a result, for a complete fix, the call_only code path needs to save whether the message was no-reply (or perhaps just the entire message) into the DBusGMethodInvocation, and use that information to avoid emitting a message from dbus_g_method_return() or dbus_g_method_return_error().

telepathy-glib exclusively uses the "async" calling convention, since some of our methods need to wait for some network traffic (returning to the main loop to process more messages while waiting for it) and *then* reply, and for the rest we might as well be consistent - (the "async" calling convention is no harder to use than the default one).

Thanks for the review; you're absolutely right.  The thread naming was me being confused with the patch in http://bugs.freedesktop.org/show_bug.cgi?id=18972 .

I've fixed that up, added a comment, and also fixed the async case by carrying through the send_reply boolean into DBusGMethodInvocation.

Created attachment 22015 [details] [review]
updated for comments

I don't think you've covered dbus_g_method_return_error(), which is the other way to emit an async reply message for a DBusGMethodInvocation.

Created attachment 22034 [details] [review]
also handle error case

Thanks, fixed.

commit d92a44109e3fdc766e34b53f7ec5329e98e13909
Author: Colin Walters <walters@verbum.org>
Date:   Tue Jan 27 17:00:37 2009 -0500

    Bug 19441: Don't send replies for messages explicitly not requesting one
    
    In sending a reply when a message has the dbus_message_set_no_reply
    flag set, we can cause spurious denials logged on the system bus, aside
    from being inefficient.

*** Bug 7211 has been marked as a duplicate of this bug. ***

(I just saw this bug referenced on IRC so late to the party etc. etc.)

Anyway, here's my view: If the system bus is causing "spurious denials" then it is the system bus that is broken. The specification *specifically* allows for such replies to be sent. I quote

 This message does not expect method return replies or error
 replies; the reply can be omitted as an optimization. However,
 it is compliant with this specification to return the reply
 despite this flag and the only harm from doing so is extra
 network traffic.

An example of where this extra traffic is useful is message tracing and other tools where a 3rd party wants to listen in.

Frankly, I'd rather we'd fix the bus to behave properly than trying to make our bindings overly "clever" and "optimized".

David, the main issue I see is that it's problematic for debugging if the bus just silently drops messages.

I'd be fine with special casing no logging for an unrequested reply denial though.

(In reply to comment #10)
> David, the main issue I see is that it's problematic for debugging if the bus
> just silently drops messages.

In what situation would the bus silently drop the message? I mean, if someone really replies to a NO_REPLY_REQUESTED message.. then the bus should honor that and deliver the reply. The bus must not second guess.

(In reply to comment #11)
> In what situation would the bus silently drop the message? I mean, if someone
> really replies to a NO_REPLY_REQUESTED message.. then the bus should honor that
> and deliver the reply. The bus must not second guess.

Conceptually, NO_REPLY_REQUESTED is an optimization, which can have any or all of the following effects. Ideally, it has *all* of these effects:

- The client process doesn't allocate memory to track the reply (e.g. DBusPendingCall, or some similar entry in a serial number => closure hash table)
- The bus daemon doesn't allocate memory to track the reply and allow it through
- The bus daemon doesn't set a timeout and, when it expires, synthesize a "timed out" message back to the sender
- The server process doesn't reply

If the bus daemon wants to allow replies, it needs to remember which serial number from which sender is waiting for a reply, and it needs to associate timeouts with these so they don't pile up forever if the reply never arrives. Requiring it to do this for NO_REPLY_REQUESTED messages would defeat (a relatively large) part of the optimization value of the flag.

(In reply to comment #11)
> (In reply to comment #10)
> > David, the main issue I see is that it's problematic for debugging if the bus
> > just silently drops messages.
> 
> In what situation would the bus silently drop the message?

This comes out of the generic security policy checking code.  The system bus configuration has:

    <allow send_requested_reply="true" send_type="method_return"/>
    <allow send_requested_reply="true" send_type="error"/>

Now, we could brute force this and simply not log denials for method_return or error type messages.  Clearly the main problems people run into are method calls, because those are what are denied by default.