Bug 36216

Summary: dbus_g_proxy_add_signal docs make false claims of introspection support
Product: dbus Reporter: Simon McVittie <smcv>
Component: GLibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: rob.taylor
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/dbus-glib/log/?h=doc-signals-36216
Whiteboard:
i915 platform: i915 features:
Attachments: dbus_g_proxy_add_signal: stop falsely claiming that we read introspection
dbus_g_proxy_add_signal: document error cases
dbus_g_proxy_add_signal: document error cases (patch v2)

Description Simon McVittie 2011-04-13 11:03:40 UTC
18:25 < neal> the service supports introspection
18:25 < smcv> that's not relevant
18:25 < smcv> dbus-glib never uses introspection
...
18:26 < neal> then dbus_g_proxy_add_signal () is required?
18:26 < smcv> oh yeah, there used to be a documentation bug where it claimed it 
              was unnecessary
18:26 < smcv> is that still true? :-(
...
18:27 < smcv> if that documentation was actually true I'd consider it to be a 
              critical bug, since that would let the signal-emitter crash 
              dbus-glib by providing wrong arguments
18:27 < smcv> possibly with arbitrary code execution
18:27 < smcv> we should fix the documentation though
Comment 1 Simon McVittie 2011-04-13 11:05:13 UTC
Created attachment 45582 [details] [review]
dbus_g_proxy_add_signal: stop falsely claiming that we read introspection

If we believed the introspection, services could change their
introspection and remote-crash us, so it's good that we don't. We
shouldn't claim that we do, though.

The second sentence is subtle: for D-Bus types that dbus-glib can map
into more than one GLib type, you must currently use the one that
dbus-glib would "naturally" produce. The only example I can find is that
object paths must be DBUS_TYPE_G_OBJECT_PATH, even though dbus-glib can
also (in principle) unmarshal object-paths as DBUS_TYPE_G_PROXY.
Comment 2 Simon McVittie 2011-04-13 11:05:30 UTC
Created attachment 45583 [details] [review]
dbus_g_proxy_add_signal: document error cases
Comment 3 Colin Walters 2011-04-13 11:10:58 UTC
Review of attachment 45582 [details] [review]:

Looks great.
Comment 4 Simon McVittie 2011-04-14 04:42:54 UTC
(In reply to comment #3)
> Review of attachment 45582 [details] [review]:
> 
> Looks great.

Fixed in git for 0.93, thanks. How about the other one?
Comment 5 Simon McVittie 2011-04-19 03:04:53 UTC
Created attachment 45804 [details] [review]
dbus_g_proxy_add_signal: document error cases (patch v2)

Same patch, adjusted to apply now that Bug #23616 has been fixed.
Comment 6 Guillaume Desmottes 2011-05-30 07:35:13 UTC
Review of attachment 45804 [details] [review]:

Looks good (assuming that's actually an error :)
Comment 7 Simon McVittie 2011-05-30 07:37:13 UTC
Thanks; fixed in git for 0.94 based on Guillaume's review + no objections from the reviewer group

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.