From 14a756ae97ab530a2167051ac0c02717841ddb94 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 11 Dec 2024 13:33:55 +0000 Subject: [PATCH] gdbusconnection: Add g_dbus_connection_register_object_with_closures2() API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This replaces `g_dbus_connection_register_object_with_closures()`, and becomes the new binding-friendly version of `g_dbus_connection_register_object()`. The problem with `g_dbus_connection_register_object_with_closures()` is that the `method_call_closure` kept the reference counting semantics of `GDBusInterfaceMethodCallFunc`, in that the `invocation` argument was `(transfer full)`, even though it was wrapped in a `GClosure`. This couldn’t be described in introspection annotations, so the `GDBusMethodInvocation` was being leaked by bindings. Some bindings added workarounds to fix the leak at our direction (see https://gitlab.gnome.org/GNOME/glib/-/issues/2600#note_1385050), which meant we could no longer change the reference counting behaviour without breaking those bindings (see #3559). So let’s start afresh with `g_dbus_connection_register_object_with_closures2()`, with correctly defined reference counting semantics (the `GDBusMethodInvocation` is `(transfer none)`) from the start. Unfortunately we can’t add a `(rename-to)` annotation to the new API, as that would effectively be an API break for existing binding code which uses the old API via that rename. Signed-off-by: Philip Withnall Fixes: #3560 --- gio/gdbusconnection.c | 114 +++++++++++++++++++++++++++++++++++++++ gio/gdbusconnection.h | 10 +++- gio/tests/gdbus-export.c | 51 ++++++++++++++---- 3 files changed, 165 insertions(+), 10 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 1f6ca5745..eacabe103 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -6099,6 +6099,9 @@ register_with_closures_on_set_property (GDBusConnection *connection, * that can be used with g_dbus_connection_unregister_object() . * * Since: 2.46 + * Deprecated: 2.84: Deprecated in favour of + * [method@Gio.DBusConnection.register_object_with_closures2], which has more + * binding-friendly reference counting semantics. */ guint g_dbus_connection_register_object_with_closures (GDBusConnection *connection, @@ -6129,6 +6132,117 @@ g_dbus_connection_register_object_with_closures (GDBusConnection *connection error); } +static void +register_with_closures_on_method_call2 (GDBusConnection *connection, + const gchar *sender, + const gchar *object_path, + const gchar *interface_name, + const gchar *method_name, + GVariant *parameters, + GDBusMethodInvocation *invocation, + gpointer user_data) +{ + RegisterObjectData *data = user_data; + GValue params[] = { G_VALUE_INIT, G_VALUE_INIT, G_VALUE_INIT, G_VALUE_INIT, G_VALUE_INIT, G_VALUE_INIT, G_VALUE_INIT }; + + g_value_init (¶ms[0], G_TYPE_DBUS_CONNECTION); + g_value_set_object (¶ms[0], connection); + + g_value_init (¶ms[1], G_TYPE_STRING); + g_value_set_string (¶ms[1], sender); + + g_value_init (¶ms[2], G_TYPE_STRING); + g_value_set_string (¶ms[2], object_path); + + g_value_init (¶ms[3], G_TYPE_STRING); + g_value_set_string (¶ms[3], interface_name); + + g_value_init (¶ms[4], G_TYPE_STRING); + g_value_set_string (¶ms[4], method_name); + + g_value_init (¶ms[5], G_TYPE_VARIANT); + g_value_set_variant (¶ms[5], parameters); + + g_value_init (¶ms[6], G_TYPE_DBUS_METHOD_INVOCATION); + /* NOTE: This is deliberately *not* g_value_set_object(), in contrast with the + * deprecated implementation in register_with_closures_on_method_call(). + * + * A reference to `invocation` is transferred in to this function, but + * bindings don’t expect a `GClosure` to provide any (transfer full) + * arguments, so consume the reference here. Bindings need to add an + * additional reference to the `GDBusMethodInvocation` before calling any + * `g_dbus_method_invocation_return_*()` methods on it. They can do this + * automatically based on the introspection annotations for those methods. */ + g_value_take_object (¶ms[6], invocation); + + g_closure_invoke (data->method_call_closure, NULL, G_N_ELEMENTS (params), params, NULL); + + g_value_unset (params + 0); + g_value_unset (params + 1); + g_value_unset (params + 2); + g_value_unset (params + 3); + g_value_unset (params + 4); + g_value_unset (params + 5); + g_value_unset (params + 6); +} + +/** + * g_dbus_connection_register_object_with_closures2: + * @connection: A [class@Gio.DBusConnection]. + * @object_path: The object path to register at. + * @interface_info: Introspection data for the interface. + * @method_call_closure: (nullable): [type@GObject.Closure] for handling incoming method calls. + * @get_property_closure: (nullable): [type@GObject.Closure] for getting a property. + * @set_property_closure: (nullable): [type@GObject.Closure] for setting a property. + * @error: Return location for error or `NULL`. + * + * Version of [method@Gio.DBusConnection.register_object] using closures instead + * of a [type@Gio.DBusInterfaceVTable] for easier binding in other languages. + * + * In contrast to [method@Gio.DBusConnection.register_object] and + * [method@Gio.DBusConnection.register_object_with_closures], the reference + * counting semantics of the function wrapped by @method_call_closure are *not* + * the same as those of [callback@Gio.DBusInterfaceMethodCallFunc]. Ownership of + * a reference to the [class@Gio.DBusMethodInvocation] is *not* transferred to + * the function. Bindings must ensure that they add a reference to the + * [class@Gio.DBusMethodInvocation] before calling any + * `g_dbus_method_invocation_return_*()` methods on it. This should be automatic + * as a result of the introspection annotations on those methods. + * + * Returns: `0` if @error is set, otherwise a registration ID (never `0`) + * that can be used with [method@Gio.DBusConnection.unregister_object]. + * + * Since: 2.84 + */ +guint +g_dbus_connection_register_object_with_closures2 (GDBusConnection *connection, + const gchar *object_path, + GDBusInterfaceInfo *interface_info, + GClosure *method_call_closure, + GClosure *get_property_closure, + GClosure *set_property_closure, + GError **error) +{ + RegisterObjectData *data; + GDBusInterfaceVTable vtable = + { + method_call_closure != NULL ? register_with_closures_on_method_call2 : NULL, + get_property_closure != NULL ? register_with_closures_on_get_property : NULL, + set_property_closure != NULL ? register_with_closures_on_set_property : NULL, + { 0 } + }; + + data = register_object_data_new (method_call_closure, get_property_closure, set_property_closure); + + return g_dbus_connection_register_object (connection, + object_path, + interface_info, + &vtable, + g_steal_pointer (&data), + register_object_free_func, + error); +} + /* ---------------------------------------------------------------------------------------------------- */ /** diff --git a/gio/gdbusconnection.h b/gio/gdbusconnection.h index fe31ea0f4..1feb9bd7d 100644 --- a/gio/gdbusconnection.h +++ b/gio/gdbusconnection.h @@ -421,7 +421,7 @@ guint g_dbus_connection_register_object (GDBusConnection gpointer user_data, GDestroyNotify user_data_free_func, GError **error); -GIO_AVAILABLE_IN_2_46 +GIO_DEPRECATED_IN_2_84_FOR(g_dbus_connection_register_object_with_closures2) guint g_dbus_connection_register_object_with_closures (GDBusConnection *connection, const gchar *object_path, GDBusInterfaceInfo *interface_info, @@ -429,6 +429,14 @@ guint g_dbus_connection_register_object_with_closures (GDBusConnectio GClosure *get_property_closure, GClosure *set_property_closure, GError **error); +GIO_AVAILABLE_IN_2_84 +guint g_dbus_connection_register_object_with_closures2 (GDBusConnection *connection, + const gchar *object_path, + GDBusInterfaceInfo *interface_info, + GClosure *method_call_closure, + GClosure *get_property_closure, + GClosure *set_property_closure, + GError **error); GIO_AVAILABLE_IN_ALL gboolean g_dbus_connection_unregister_object (GDBusConnection *connection, guint registration_id); diff --git a/gio/tests/gdbus-export.c b/gio/tests/gdbus-export.c index 599df5bb5..f2f3b610a 100644 --- a/gio/tests/gdbus-export.c +++ b/gio/tests/gdbus-export.c @@ -161,6 +161,23 @@ foo_method_call (GDBusConnection *connection, } } +static void +foo_method_call_with_closure (GDBusConnection *connection, + const gchar *sender, + const gchar *object_path, + const gchar *interface_name, + const gchar *method_name, + GVariant *parameters, + GDBusMethodInvocation *invocation, + gpointer user_data) +{ + /* The call below takes ownership of the invocation but ownership is not + * passed into the callback so get an additional reference here */ + g_object_ref (invocation); + + foo_method_call (connection, sender, object_path, interface_name, method_name, parameters, invocation, user_data); +} + static GVariant * foo_get_property (GDBusConnection *connection, const gchar *sender, @@ -1427,8 +1444,9 @@ test_object_registration (void) } static void -test_object_registration_with_closures (void) +test_object_registration_with_closures (gconstpointer test_data) { + gboolean use_new_api = GPOINTER_TO_INT (test_data); GError *error; guint registration_id; @@ -1437,13 +1455,27 @@ test_object_registration_with_closures (void) g_assert_no_error (error); g_assert_nonnull (c); - registration_id = g_dbus_connection_register_object_with_closures (c, - "/foo/boss", - (GDBusInterfaceInfo *) &foo_interface_info, - g_cclosure_new (G_CALLBACK (foo_method_call), NULL, NULL), - g_cclosure_new (G_CALLBACK (foo_get_property), NULL, NULL), - g_cclosure_new (G_CALLBACK (foo_set_property), NULL, NULL), - &error); + G_GNUC_BEGIN_IGNORE_DEPRECATIONS + + if (use_new_api) + registration_id = g_dbus_connection_register_object_with_closures2 (c, + "/foo/boss", + (GDBusInterfaceInfo *) &foo_interface_info, + g_cclosure_new (G_CALLBACK (foo_method_call_with_closure), NULL, NULL), + g_cclosure_new (G_CALLBACK (foo_get_property), NULL, NULL), + g_cclosure_new (G_CALLBACK (foo_set_property), NULL, NULL), + &error); + else + registration_id = g_dbus_connection_register_object_with_closures (c, + "/foo/boss", + (GDBusInterfaceInfo *) &foo_interface_info, + g_cclosure_new (G_CALLBACK (foo_method_call), NULL, NULL), + g_cclosure_new (G_CALLBACK (foo_get_property), NULL, NULL), + g_cclosure_new (G_CALLBACK (foo_set_property), NULL, NULL), + &error); + + G_GNUC_END_IGNORE_DEPRECATIONS + g_assert_no_error (error); g_assert_cmpuint (registration_id, >, 0); @@ -1978,7 +2010,8 @@ main (int argc, loop = g_main_loop_new (NULL, FALSE); g_test_add_func ("/gdbus/object-registration", test_object_registration); - g_test_add_func ("/gdbus/object-registration-with-closures", test_object_registration_with_closures); + g_test_add_data_func ("/gdbus/object-registration-with-closures", GINT_TO_POINTER (FALSE), test_object_registration_with_closures); + g_test_add_data_func ("/gdbus/object-registration-with-closures2", GINT_TO_POINTER (TRUE), test_object_registration_with_closures); g_test_add_func ("/gdbus/registered-interfaces", test_registered_interfaces); g_test_add_func ("/gdbus/async-properties", test_async_properties); g_test_add_data_func ("/gdbus/threaded-unregistration/object", GINT_TO_POINTER (FALSE), test_threaded_unregistration);