After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 656325 - Make GDBusInterfaceVTable binding friendly
Make GDBusInterfaceVTable binding friendly
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gdbus
2.29.x
Other Linux
: Normal normal
: ---
Assigned To: Martin Pitt
gtkdev
: 655051 674762 675814 (view as bug list)
Depends on: 656554
Blocks: 656330
 
 
Reported: 2011-08-11 08:17 UTC by Martin Pitt
Modified: 2015-08-19 11:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
first patch (with some debugging enabled) (12.00 KB, patch)
2011-08-11 08:43 UTC, Martin Pitt
reviewed Details | Review
Python test script (4.21 KB, text/plain)
2011-08-11 08:47 UTC, Martin Pitt
  Details
Make GDBusInterfaceVTable binding friendly (12.05 KB, patch)
2011-08-11 12:51 UTC, Martin Pitt
rejected Details | Review
incomplete and non-working closures patch (6.26 KB, patch)
2011-08-15 10:23 UTC, Martin Pitt
rejected Details | Review
Add g_dbus_connection_register_object_with_closures function. (15.11 KB, patch)
2014-09-01 19:42 UTC, Janusz Lewandowski
needs-work Details | Review
[PATCH 2/2] Add support for reporting errors from {s,g}et_property_closure. (5.10 KB, patch)
2014-09-01 19:43 UTC, Janusz Lewandowski
none Details | Review
Update g_dbus_connection_register_object_with_closures docs. (1.89 KB, patch)
2014-09-01 21:56 UTC, Janusz Lewandowski
reviewed Details | Review
Add g_dbus_connection_register_object_with_closures function. (15.89 KB, patch)
2014-10-10 21:08 UTC, Janusz Lewandowski
committed Details | Review

Description Martin Pitt 2011-08-11 08:17:00 UTC
A central function for implementing GDBus server objects is 
g_dbus_connection_register_object(). This requires a GDBusInterfaceVTable
parameter, which is a bare struct.

For using that in Python and other GI bindings we need to box GDBusInterfaceVTable to allow the bindings to create and 
clean up GDBusInterfaceVTable structs.

This is not enough, however, as you currently cannot do struct member assignments for callbacks. This might be a limitation of pygobject, but it's generally tricky as they are lacking a GDestroyNotify for cleaning up the 
internally used closures.

One way to get around this would be to introduce something like (scope forever), which would tell pygobject and friends to never clean up the internal closures for the callbacks. However, after a discussion with Tomeu and Colin Walters we think that it's better to use the usual (scope notified) and provide proper GDestroyNotifys.

So we also need to provide accessor methods for the three 
fields (method_call, get_property, set_property callbacks) which get an extra 
GDestroyNotify/user_data pair. This allows bindings to clean up the callback 
resources properly when they go out of scope or when the DBus object gets 
unregistered.

When using the boxing and accessor methods we need some extra data to keep track of reference counting and the GDestroyNotify/user_data pairs. We can use one of the unused padding pointers for this and introduce a new private struct for this. It is only used when calling the wrapper constructor; the original C API/ABI, behaviour, and performance is unchanged and does not need to do any of these extra housekeeping.
Comment 1 Martin Pitt 2011-08-11 08:43:47 UTC
Created attachment 193614 [details] [review]
first patch (with some debugging enabled)

This is the first version of the patch, which is functionally complete. Together with my recent fixes in glib (crashes due to missing/wrong transfer annotations) and pygobject (GVariant support in callbacks) I can now use the full GDBus API from Python.

This patch still has three g_debug() statements which show the reference counting and free'ing of the VTable structs, for checking that the cleanup and gdestroynotify calls actually happen. For some weird reason I get a crash when  I comment the g_debug()s, I'll debug this now. But I'd appreciate some feedback about the general structure of the patch.
Comment 2 Martin Pitt 2011-08-11 08:47:47 UTC
Created attachment 193615 [details]
Python test script

This is my Python script for testing. It registers a de.piware.Demo namespace on the session bus with a "hello" method and a "number" property, and then runs gdbus a couple of times to call the method and get/set the number property:

$ jhbuild run python ~/src/test/python/gdbus-server.py 
bus_own_name done: 1
bus_acquired: <DBusConnection object at 0x7fcf4cfa2e60 (GDBusConnection at 0x1790820)> de.piware.Demo
--- XML: ---
    <node>
      <interface name="de.piware.Demo.Hello">
        <method name="hello">
          <arg type="s" name="greeting" direction="in">
          </arg>
          <arg type="s" name="response" direction="out">
          </arg>
        </method>
        <property type="i" name="number" access="readwrite">
        </property>
      </interface>
    </node>

-------
(process:12608): GLib-GIO-DEBUG: g_dbus_interface_vtable_copy: shallow with wrapper data, refcount now 2
registered object id 1
(process:12608): GLib-GIO-DEBUG: g_dbus_interface_vtable_free: with wrapper data, refcount 2
name_acquired: <DBusConnection object at 0x7fcf4cfa2e60 (GDBusConnection at 0x1790820)> de.piware.Demo

----- sending request hello("Martin") -------
method call: / de.piware.Demo.Hello.hello(<GLib.Variant(('Martin',))>)
('Good day Martin',)

----- sending request hello("error") -------
method call: / de.piware.Demo.Hello.hello(<GLib.Variant(('error',))>)
Fehler: GDBus.Error:org.freedesktop.DBus.Error.IOError: Raising an error as requested
(According to introspection data, you need to pass `s')

----- sending property get -------
get_property: / de.piware.Demo.Hello number
user data: None
(<(42,)>,)

----- sending property set -------
set_property: / de.piware.Demo.Hello number = <GLib.Variant(99)>
user data: None
method return sender=:1.3754 -> dest=:1.3758 reply_serial=2

----- sending property get -------
get_property: / de.piware.Demo.Hello number
user data: None
(<(99,)>,)
unregistering object...
(process:12608): GLib-GIO-DEBUG: g_dbus_interface_vtable_free: with wrapper data, refcount 1
(process:12608): GLib-GIO-DEBUG: g_dbus_interface_vtable_free: with wrapper data, refcount zero, releasing
unowning bus name...




To run this, you need to have the latest version of glib, gobject-introspection, and pygobject (I use jhbuild), and regenerate g-i's Gio header copy after building/installing glib, with something like
cd misc; ./update-glib-annotations.py ~/upstream/glib
  cd misc; ./update-glib-annotations.py ~/upstream/glib

in g-i's source tree.
Comment 3 Martin Pitt 2011-08-11 10:06:56 UTC
OK, turns out the crash that I see with the g_debug()s gone are not related to this patch, but are related to the GString usage in the test script:

    s = GLib.String()
    node_info.generate_xml(4, s)
    print '--- XML: ---\n%s\n-------' % s.str
    s.free(True)

when I comment out the s.free() call, things work perfectly even without the g_debug() lines. Strange heisenbug..

So I renamed the patch attachment, it's good for review.
Comment 4 Johan (not receiving bugmail) Dahlin 2011-08-11 10:24:44 UTC
Review of attachment 193614 [details] [review]:

::: gio/gdbusconnection.c
@@ +3638,3 @@
+ 
+  vtable = g_malloc0 (sizeof (GDBusInterfaceVTable));
+  vtable->wrapper_data = g_malloc0 (sizeof (struct __GDBusInterfaceVTableWrapperData));

Consider using g_slice() for these

@@ +3685,3 @@
+	  g_debug("g_dbus_interface_vtable_free: with wrapper data, refcount zero, releasing");
+	  g_free (vtable->wrapper_data);
+

Remove the last free...

@@ +3688,3 @@
+	}
+    }
+g_dbus_interface_vtable_free (GDBusInterfaceVTable *vtable)

and kill the else

@@ +3715,3 @@
+                                         GDestroyNotify notify)
+{
+	  if (vtable->wrapper_data->set_property_destroy_notify != NULL)

g_return_if_fail for vtable + mehod_call

@@ +3741,3 @@
+                                          GDestroyNotify notify)
+{
+	g_dbus_interface_vtable_copy, g_dbus_interface_vtable_free);

g_return_if_fail

@@ +3767,3 @@
+                                          GDestroyNotify notify)
+{
+ *

g_return_if_fail
Comment 5 Colin Walters 2011-08-11 10:24:52 UTC
Review of attachment 193614 [details] [review]:

Two minor comments, otherwise looks good.  Only the first is needed.

::: gio/gdbusconnection.c
@@ +3613,3 @@
+ * language bindings. The C API does not need to be concerned about it. */
+struct __GDBusInterfaceVTableWrapperData {
+  int ref_count;

One tricky problem is that we actually need boxed refcounting to be threadsafe as a general rule.  At least in gjs for example, the garbage collector runs in a separate thread, and will in fact free boxeds from that thread.

So this should be "volatile int refcount".  I just did this in gnome-menus/libmenu/gmenu-tree.c for GMenuTreeItem if you want a recent example.

Oh nevermind, you were using atomic ops, just need a volatile here.

@@ +3670,3 @@
+{
+  if (vtable->wrapper_data != NULL)
+    {

You could avoid excessive indentation by changing this to:

if (vtable->wrapper_data == NULL)
  {
    g_free (vtable);
    return;
  }

@@ +3672,3 @@
+    {
+      g_debug("g_dbus_interface_vtable_free: with wrapper data, refcount %i", vtable->wrapper_data->ref_count);
+      if (g_atomic_int_dec_and_test (&vtable->wrapper_data->ref_count))

Same here.
Comment 6 Martin Pitt 2011-08-11 12:13:13 UTC
Comment on attachment 193615 [details]
Python test script

The GString crash is fixed with glib commit c3fd789bb759. As I figure that there will be some more changes to the test script, I keep it up to date at

http://people.canonical.com/~pitti/tmp/gdbus-server.py
Comment 7 Martin Pitt 2011-08-11 12:42:41 UTC
Johan,

thanks for the review!

(In reply to comment #4)
> Consider using g_slice() for these

Ah, will do that. However, I'll keep the original g_memdup() in g_dbus_interface_vtable_copy() in case there is no wrapper data, to not break the C ABI. Or ar g_slice()s guaranteed to be compatible with the normal g_memdup() etc. functions?

> Remove the last free...
> [...]
> and kill the else

I changed the patch to Colin's suggestion now, and with the g_slice conversion it's not actually the same g_free() call any more.

> @@ +3715,3 @@
> +                                         GDestroyNotify notify)
> +{
> +      if (vtable->wrapper_data->set_property_destroy_notify != NULL)
> 
> g_return_if_fail for vtable + mehod_call

Actually, we don't even need to generate a critial message there; you can use these wrappers without the _new() constructor, and in this case they should just set the field and nothing else.
Comment 8 Martin Pitt 2011-08-11 12:51:30 UTC
Created attachment 193636 [details] [review]
Make GDBusInterfaceVTable binding friendly

Revised patch which addresses Colin's and Johan's review comments, thanks!

This also comments out the three remaining g_debug()s.
Comment 9 David Zeuthen (not reading bugmail) 2011-08-11 14:13:10 UTC
Two thoughts

First, not sure why you just can't write your own version of g_dbus_connection_register_object() in C that does memory management the way you want it... I mean, sooner or later you will need that anyway for some other library that you can't control... right? In fact, don't you already _need_ to have C code to properly handle e.g. GVariant? I remember Colin musing about "it's fine to have manual binding glue at the GLib level, but, preferably, not above".

(IIRC, manual C code is how gjs-gdbus copes with it.)

Second, instead of the proposed patch, I'd much rather introduce a single C function, let's call it

 g_dbus_connection_register_object_with_callbacks1()

that just takes the function pointers instead of the vtable struct.

(Of course, the whole idea of using a VTable is that we can add function pointers in the future so if that happens we need another version of this newly introduced function (why I proposed to suffix its name with _with_callbacks1))
Comment 10 Martin Pitt 2011-08-11 14:24:55 UTC
(In reply to comment #9)
> First, not sure why you just can't write your own version of
> g_dbus_connection_register_object() in C that does memory management the way
> you want it...

pygobject could do that, similar to gjs. It would just introduce Gio specifics into pygobject, not sure whether Tomeu/John would be too happy about it. Right now there is nothing similar like that in pygobject, and in fact master just dropped all the remaining static bindings. But of course it would be doable in principle.

The main drawback of this is that each binding would have to re-do this implementation. It's done in GJS (although that uses dbus-glib right now), and can be done for pygobject, but vala, C# etc. would have to do that as well. In the spirit of GI (fix and annotate the libraries once), we thought it would be better to change GLib to be binding friendly, as everything else is.

> In fact, don't you already _need_ to have C code to properly handle e.g. GVariant?

> Second, instead of the proposed patch, I'd much rather introduce a single C
> function, let's call it
> 
>  g_dbus_connection_register_object_with_callbacks1()
> 
> that just takes the function pointers instead of the vtable struct.

That was indeed my first idea, to avoid exposing the vtable at all. Drawbacks:

 - pygobject can't handle more than one callback per function at the moment, so we still need individual accessors. Those are better anyway in the case when VTable grows another field; then we just need a new accessor instead of a _callbacks2().

 - The patch wouldn't get significantly easier, as we would still need to get, track, and call the GDestroyNotifies with the user_data.

But if you think it's syntactically easier, I'm fine with hiding the vtable stuff and changing the accessors to g_dbus_connection_set_method_call() which take an additional registration_id argument.
Comment 11 David Zeuthen (not reading bugmail) 2011-08-11 15:01:39 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > First, not sure why you just can't write your own version of
> > g_dbus_connection_register_object() in C that does memory management the way
> > you want it...
> 
> pygobject could do that, similar to gjs. It would just introduce Gio specifics
> into pygobject, not sure whether Tomeu/John would be too happy about it. Right
> now there is nothing similar like that in pygobject, and in fact master just
> dropped all the remaining static bindings. But of course it would be doable in
> principle.
> 
> The main drawback of this is that each binding would have to re-do this
> implementation. It's done in GJS (although that uses dbus-glib right now), and
> can be done for pygobject, but vala, C# etc. would have to do that as well. In
> the spirit of GI (fix and annotate the libraries once), we thought it would be
> better to change GLib to be binding friendly, as everything else is.

I can't speak for Tomeu/John, obviously, but I don't think it's a big problem to add this to pygobject... it's a single C function you have to write to make this work. And I'd rather do that than to add a lot of confusing functions/fields to some C code that is already hard to grasp.

Btw, you need to add language-specific code _anyway_ ... to take advantage of language-specific features like Python decorators to convey that a Python method should be exported. And, FWIW, the code is already written for gjs (it was recently ported to also use GDBus) and IIRC Vala has working GDBus support too.

Interesting fact: GDBus was never _intended_ to be bindable - the idea was always that languages would provide their own D-Bus bindings based on libdbus-1. That was the thinking at the time GDBus was designed. And the main reason that GVariant was used (as GVariant wasn't designed to be bound either, IIRC.. at least not without a lot of work.) It's more an accident that mostly everything is easily bindable...

> > In fact, don't you already _need_ to have C code to properly handle e.g. GVariant?

Does pygobject have code to handle GVariant already?
Comment 12 Colin Walters 2011-08-11 15:18:25 UTC
(In reply to comment #9)
> I remember Colin musing about
> "it's fine to have manual binding glue at the GLib level, but, preferably, not
> above".

We want Gio to be 100% introspectable by default.  GLib and GObject are more special cases.
Comment 13 Martin Pitt 2011-08-11 15:57:05 UTC
Result from IRC discussion: We want the API to be 100% introspectable ideally, but this patch was deemed too complex by David. Instead we should add a new method g_dbus_connection_register_object_with_closures() and pass the three callbacks directly. This would then look similar to g_bus_own_name_with_closures() and friends.

We can then mark g_dbus_connection_register_object() as (skip) to avoid putting it into language specific documentation.
Comment 14 Martin Pitt 2011-08-15 10:20:51 UTC
For the record, I have most of the revised patch ready (using a _with_closures()) variant, but this is blocked on bug 656554 now (pygobject doesn't handle gvariant arguments with closures).
Comment 15 Martin Pitt 2011-08-15 10:23:19 UTC
Created attachment 193854 [details] [review]
incomplete and non-working closures patch

This is the current version of the rewritten patch. It still doesn't have handlers for {set,get}_property(), but these are easy to add once the pygobject gvariant bug is fixed.

Attaching it here in case someone wants to comment on the general structure, and also that I don't accidentally lose it :)
Comment 16 David Zeuthen (not reading bugmail) 2011-09-13 16:00:46 UTC
Did you work around the GDBus problem or is this still something that should be kept open? Thanks
Comment 17 Martin Pitt 2011-09-13 17:23:52 UTC
Hey David,

there is no workaround for it, this still needs to be kept open. It does work with the first patch (making the VTable bindable), but the solution we agreed on later (adding a new _with_closures()) is blocked by bug	656554.
Comment 18 Martin Pitt 2012-04-25 05:35:25 UTC
*** Bug 674762 has been marked as a duplicate of this bug. ***
Comment 19 Martin Pitt 2012-06-01 08:36:58 UTC
*** Bug 675814 has been marked as a duplicate of this bug. ***
Comment 20 Guido Günther 2012-09-11 15:42:05 UTC
Any update on this? I'd be great if we could do server side dbus without falling back to dbus-glib.
Comment 21 Martin Pitt 2012-09-12 03:51:09 UTC
Sorry, I didn't get to this again since the last time. The blocking bug (GVariant handling in closures in pygobject) has been fixed some time ago, but as dbus-python speaks Python 3 now, the pressing need for it has abated quite a bit. However, the second attached patch here should be a good start.
Comment 22 Joanmarie Diggs (IRC: joanie) 2012-11-23 18:56:37 UTC
(In reply to comment #21)
> Sorry, I didn't get to this again since the last time. The blocking bug
> (GVariant handling in closures in pygobject) has been fixed some time ago, but
> as dbus-python speaks Python 3 now, the pressing need for it has abated quite a
> bit. However, the second attached patch here should be a good start.

I thought we were being discouraged from using dbus-python as per:
https://bugzilla.gnome.org/show_bug.cgi?id=658834#c0
Comment 23 Simon Feltman 2013-09-12 07:51:18 UTC
*** Bug 655051 has been marked as a duplicate of this bug. ***
Comment 24 Janusz Lewandowski 2014-09-01 19:42:33 UTC
Created attachment 285068 [details] [review]
Add g_dbus_connection_register_object_with_closures function.

Hi,

I've took Martin's patch and finished it. And modified gdbus-example-server.c to test it.

This patch does not support passing GError from closures, because GError cannot be stored in GValue. I've developed a workaround for that, it's in the second patch. But I'm not sure if it's worth it to use it, because GLib seems to discard that GError anyway.

Full patch history is at https://github.com/LEW21/glib/commits/register-with-closures-errors, this one has commits 1-5 squashed.
Comment 25 Janusz Lewandowski 2014-09-01 19:43:12 UTC
Created attachment 285069 [details] [review]
[PATCH 2/2] Add support for reporting errors from {s,g}et_property_closure.
Comment 26 Simon Feltman 2014-09-01 21:13:57 UTC
(In reply to comment #24)
> This patch does not support passing GError from closures, because GError cannot
> be stored in GValue. I've developed a workaround for that, it's in the second
> patch. But I'm not sure if it's worth it to use it, because GLib seems to
> discard that GError anyway.

It depends, generally GErrors do not show up as explicit arguments in gir/typelib files but rather the function is marked with throws="1" which implies the function or callback takes a tail end GError. The exception here is callback fields for vfuncs which currently suffer from bug 729543.
Comment 27 Janusz Lewandowski 2014-09-01 21:54:50 UTC
(In reply to comment #26)
> It depends, generally GErrors do not show up as explicit arguments in
> gir/typelib files but rather the function is marked with throws="1" which
> implies the function or callback takes a tail end GError. The exception here is
> callback fields for vfuncs which currently suffer from bug 729543.

The signatures of those closures are not exposed to gir. That's done like g_bus_watch_name_with_closures() - g_dbus_connection_register_object_with_closures() simply takes a GClosure object as a parameter, and then invokes it multiple times later.

The problem is that there is no way to tell g_closure_invoke that the last parameter of the closure is a GError. And I don't see any similar code in GLib to take inspiration from.
Comment 28 Janusz Lewandowski 2014-09-01 21:56:21 UTC
Created attachment 285103 [details] [review]
Update g_dbus_connection_register_object_with_closures docs.

I've forgotten about this in the initial patch.
Comment 29 Simon Feltman 2014-09-02 01:06:56 UTC
(In reply to comment #27)
> (In reply to comment #26)
> > It depends, generally GErrors do not show up as explicit arguments in
> > gir/typelib files but rather the function is marked with throws="1" which
> > implies the function or callback takes a tail end GError. The exception here is
> > callback fields for vfuncs which currently suffer from bug 729543.
> 
> The signatures of those closures are not exposed to gir. That's done like
> g_bus_watch_name_with_closures() -
> g_dbus_connection_register_object_with_closures() simply takes a GClosure
> object as a parameter, and then invokes it multiple times later.

Ah right, GClosure as an argument doesn't have any kind of annotation support for its own arguments AFAICT (basically what we hit with bug 690394).

> The problem is that there is no way to tell g_closure_invoke that the last
> parameter of the closure is a GError. And I don't see any similar code in GLib
> to take inspiration from.

You could conceivably use a boxed GValue typed as G_TYPE_ERROR. But it would still probably be useless without GI and bindings interpreting it as "throws" somehow.
Comment 30 Janusz Lewandowski 2014-09-02 10:44:17 UTC
Hmm...

After applying the first patch (and the doc one), the signature of method_call_closure seen by Python is:

(DBusConnection, str sender, str object_path, str interface_name, str property_name) -> GVariant

If we would implement the ideal solution - make the bindings know, that it supports throwing - then it would not change. As the ideal solution requires a lot of work, and this patch is quite useful as is (I hope it finally makes it possible to expose objects from PyGI, and therefore to deprecate dbus-python), I suggest merging it as is.

Maybe should I add a note to the docs, that this function is designed for bindings, and it's not guaranteed to preserve C ABI compatibility, so it will be possible to add GError** to the C signature in future without resorting to g_dbus_connection_register_object_with_closures2?
Comment 31 Janusz Lewandowski 2014-09-02 10:45:12 UTC
Sorry, not method_call_closure, but get_property_closure.
Comment 32 Martin Pitt 2014-09-16 15:46:14 UTC
Review of attachment 285068 [details] [review]:

Thanks Janusz for your work here!

The patch looks good to me in general (although, note that I'm not a glib developer and thus cannot give an authoritative +1 here).

Could you please add a new test for this function to gio/tests/gdbus-export.c ? It doesn't need to fully mirror test_object_registration(), you can trim away most of the special cases as the new _with_closures() just wraps the existing function.

I'd rather keep gdbus-example-server.c as it is and add a new gdbus-example-server-closures.c to demonstrate either usage. Or leave out this part of the patch completely -- in C you pretty much always want to use the existing API, the _with_closures() is only really interesting for bindings. Once this lands, I'd rather add an example (and tests) to pygobject.
Comment 33 Martin Pitt 2014-09-16 15:48:23 UTC
Comment on attachment 285103 [details] [review]
Update g_dbus_connection_register_object_with_closures docs.

The doc patch looks good, but please fold it into the other one.
Comment 34 Janusz Lewandowski 2014-10-10 21:08:13 UTC
Created attachment 288258 [details] [review]
Add g_dbus_connection_register_object_with_closures function.

Hi,

I've merged the base and doc patch, added a test to gio/tests/gdbus-export.c and removed the changes to gdbus-example-server.c.
Comment 35 Tony Asleson 2014-12-15 22:17:46 UTC
I tried this patch with 2.42.0 (Fedora 21) and I was successful in using:

Gio.DBusConnection.register_object_with_closures

in python to register callbacks.

What else do we need to get this committed?

Thanks,
Tony
Comment 36 Stephen Gallagher 2015-08-18 12:11:58 UTC
The rolekit project (a key component of Fedora Server) is considering migrating to gdbus from python-dbus, but this bug is a blocker to that effort. Could someone please have another look at this and preferably get it finished up to commit?
Comment 37 Emmanuele Bassi (:ebassi) 2015-08-18 12:35:25 UTC
Review of attachment 288258 [details] [review]:

::: gio/gdbusconnection.c
@@ +5327,3 @@
+
+  if (data->method_call_closure != NULL)
+  if (get_property_closure != NULL)

Could be replaced by:

  g_clear_pointer (&data->method_call_closure, g_closure_unref);
  g_clear_pointer (&data->get_property_closure, g_closure_unref);
  g_clear_pointer (&data->set_property_closure, g_closure_unref);

  g_free (data);

@@ +5372,3 @@
+  g_value_set_object (&params[6], invocation);
+
+

The sizeof/sizeof could be replaced by G_N_ELEMENTS (params).

@@ +5414,3 @@
+  g_value_init (&result_value, G_TYPE_VARIANT);
+
+                                       gpointer               user_data)

Same as above: sizeof/sizeof can be replaced by G_N_ELEMENTS.

@@ +5418,3 @@
+  result = g_value_get_variant (&result_value);
+  if (result)
+  RegisterObjectData *data = user_data;

Coding style: missing space between function name and parenthesis.

@@ +5428,3 @@
+
+  if (!result)
+

The error should be reworded, at least:

  Unable to retrieve property %s.%s

The GError should also be marked as translatable, if we expect this to be user visible.

@@ +5468,3 @@
+  g_value_init (&result_value, G_TYPE_BOOLEAN);
+
+

Same as above: sizeof/sizeof → G_N_ELEMENTS

@@ +5481,3 @@
+
+  if (!result)
+  g_value_unset (params + 5);

The GError message should be:

  "Unable to set property %s.%s"

and it should be marked for translation.

@@ +5491,3 @@
+ * @object_path: The object path to register at.
+ * @interface_info: Introspection data for the interface.
+register_with_closures_on_get_property (GDBusConnection *connection,

The (allow-none) annotation has been replaced by the (nullable) one.

@@ +5513,3 @@
+                                                 GError              **error)
+{
+  GVariant *result;

Coding style: missing space between function name and parenthesis.

@@ +5523,3 @@
+
+    return g_dbus_connection_register_object (connection,
+  g_value_init (&params[1], G_TYPE_STRING);

Coding style: arguments should be all aligned vertically.

::: gio/gdbusconnection.h
@@ +392,3 @@
                                                                GDestroyNotify              user_data_free_func,
                                                                GError                    **error);
 GLIB_AVAILABLE_IN_ALL

The annotation needs to be changed to reflect the version.

::: gio/tests/gdbus-export.c
@@ +736,3 @@
+typedef struct
+{
+	const gchar *object_path;

Coding style: wrong indentation.

@@ +738,3 @@
+	const gchar *object_path;
+	gboolean check_remote_errors;
+} test_dispatch_thread_func_args;

Coding style: use camel case for structure types.

@@ +906,3 @@
   g_assert (value == NULL);
+  if (args->check_remote_errors)
+  {

Coding style: wrong indentation level.

@@ +907,3 @@
+  if (args->check_remote_errors)
+  {
+    // _with_closures variant doesn't support customizing error data.

No C++ style comments.

@@ +1419,3 @@
+  
+  registration_id = g_dbus_connection_register_object_with_closures (c,
+

Coding style:

 * arguments should be aligned
 * missing space between function/macro name and parenthesis
Comment 38 Matthias Clasen 2015-08-18 20:42:08 UTC
Committed with the fixups pointed out by Emmanuele
Comment 39 Stephen Gallagher 2015-08-19 11:51:29 UTC
Thanks for the very fast turnaround on this! It's highly appreciated.