From: Luiz Augusto von Dentz <[email protected]>
Calling g_dbus_client_new followed by g_dbus_client_set_proxy_handlers
cause two calls to GetManagedObjects in a row as GetNameOwner reply is
asyncronously it triggers the second call because the handlers have
been set by g_dbus_client_set_proxy_handlers.
---
gdbus/client.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/gdbus/client.c b/gdbus/client.c
index c2d2346..55f1d89 100644
--- a/gdbus/client.c
+++ b/gdbus/client.c
@@ -40,6 +40,7 @@ struct GDBusClient {
char *base_path;
GPtrArray *match_rules;
DBusPendingCall *pending_call;
+ DBusPendingCall *get_objects_call;
GDBusWatchFunction connect_func;
void *connect_data;
GDBusWatchFunction disconn_func;
@@ -992,6 +993,8 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
DBusMessage *reply = dbus_pending_call_steal_reply(call);
DBusError error;
+ g_dbus_client_ref(client);
+
dbus_error_init(&error);
if (dbus_set_error_from_message(&error, reply) == TRUE) {
@@ -1004,19 +1007,24 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
done:
dbus_message_unref(reply);
+ dbus_pending_call_unref(client->get_objects_call);
+ client->get_objects_call = NULL;
+
g_dbus_client_unref(client);
}
static void get_managed_objects(GDBusClient *client)
{
DBusMessage *msg;
- DBusPendingCall *call;
if (!client->proxy_added && !client->proxy_removed) {
refresh_properties(client);
return;
}
+ if (client->get_objects_call != NULL)
+ return;
+
msg = dbus_message_new_method_call(client->service_name, "/",
DBUS_INTERFACE_DBUS ".ObjectManager",
"GetManagedObjects");
@@ -1026,16 +1034,14 @@ static void get_managed_objects(GDBusClient *client)
dbus_message_append_args(msg, DBUS_TYPE_INVALID);
if (dbus_connection_send_with_reply(client->dbus_conn, msg,
- &call, -1) == FALSE) {
+ &client->get_objects_call, -1) == FALSE) {
dbus_message_unref(msg);
return;
}
- g_dbus_client_ref(client);
-
- dbus_pending_call_set_notify(call, get_managed_objects_reply,
- client, NULL);
- dbus_pending_call_unref(call);
+ dbus_pending_call_set_notify(client->get_objects_call,
+ get_managed_objects_reply,
+ client, NULL);
dbus_message_unref(msg);
}
@@ -1285,6 +1291,11 @@ void g_dbus_client_unref(GDBusClient *client)
dbus_pending_call_unref(client->pending_call);
}
+ if (client->get_objects_call != NULL) {
+ dbus_pending_call_cancel(client->get_objects_call);
+ dbus_pending_call_unref(client->get_objects_call);
+ }
+
for (i = 0; i < client->match_rules->len; i++) {
modify_match(client->dbus_conn, "RemoveMatch",
g_ptr_array_index(client->match_rules, i));
--
1.8.1.4
On Mon, Apr 29, 2013 at 2:34 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Lucas,
>
> On Mon, Apr 29, 2013 at 7:39 PM, Lucas De Marchi
> <[email protected]> wrote:
>> On Mon, Apr 29, 2013 at 11:45 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi,
>>>
>>> On Thu, Apr 18, 2013 at 11:47 PM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> Calling g_dbus_client_new followed by g_dbus_client_set_proxy_handlers
>>>> cause two calls to GetManagedObjects in a row as GetNameOwner reply is
>>>> asyncronously it triggers the second call because the handlers have
>>>> been set by g_dbus_client_set_proxy_handlers.
>>>> ---
>>>> gdbus/client.c | 25 ++++++++++++++++++-------
>>>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/gdbus/client.c b/gdbus/client.c
>>>> index c2d2346..55f1d89 100644
>>>> --- a/gdbus/client.c
>>>> +++ b/gdbus/client.c
>>>> @@ -40,6 +40,7 @@ struct GDBusClient {
>>>> char *base_path;
>>>> GPtrArray *match_rules;
>>>> DBusPendingCall *pending_call;
>>>> + DBusPendingCall *get_objects_call;
>>>> GDBusWatchFunction connect_func;
>>>> void *connect_data;
>>>> GDBusWatchFunction disconn_func;
>>>> @@ -992,6 +993,8 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
>>>> DBusMessage *reply = dbus_pending_call_steal_reply(call);
>>>> DBusError error;
>>>>
>>>> + g_dbus_client_ref(client);
>>>> +
>>>> dbus_error_init(&error);
>>>>
>>>> if (dbus_set_error_from_message(&error, reply) == TRUE) {
>>>> @@ -1004,19 +1007,24 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
>>>> done:
>>>> dbus_message_unref(reply);
>>>>
>>>> + dbus_pending_call_unref(client->get_objects_call);
>>>> + client->get_objects_call = NULL;
>>>> +
>>>> g_dbus_client_unref(client);
>>>> }
>>>>
>>>> static void get_managed_objects(GDBusClient *client)
>>>> {
>>>> DBusMessage *msg;
>>>> - DBusPendingCall *call;
>>>>
>>>> if (!client->proxy_added && !client->proxy_removed) {
>>>> refresh_properties(client);
>>>> return;
>>>> }
>>>>
>>>> + if (client->get_objects_call != NULL)
>>>> + return;
>>>> +
>>>> msg = dbus_message_new_method_call(client->service_name, "/",
>>>> DBUS_INTERFACE_DBUS ".ObjectManager",
>>>> "GetManagedObjects");
>>>> @@ -1026,16 +1034,14 @@ static void get_managed_objects(GDBusClient *client)
>>>> dbus_message_append_args(msg, DBUS_TYPE_INVALID);
>>>>
>>>> if (dbus_connection_send_with_reply(client->dbus_conn, msg,
>>>> - &call, -1) == FALSE) {
>>>> + &client->get_objects_call, -1) == FALSE) {
>>>> dbus_message_unref(msg);
>>>> return;
>>>> }
>>>>
>>>> - g_dbus_client_ref(client);
>>>> -
>>>> - dbus_pending_call_set_notify(call, get_managed_objects_reply,
>>>> - client, NULL);
>>>> - dbus_pending_call_unref(call);
>>>> + dbus_pending_call_set_notify(client->get_objects_call,
>>>> + get_managed_objects_reply,
>>>> + client, NULL);
>>
>> Why don't you let the unref here, so you don't need to do it in
>> g_dbus_client_unref() and get_managed_objects_reply(). This way the
>> only ref is owned by libdbus which will free it when it's done.
>
> But that means we have no control over any reference and from libdbus
> perspective it can free whenever it likes which I think could break in
> case where the callback is not called so I find it safer not to abuse
> it here. Note that in get_name_owner we do the same, although some
> other parts of the code it seems to not get this right and would
> probably break in case the user data is freed before the pending call
> completes.
Well... in the cases it does this user data is freed only when the
pending call completes. It's not "not getting this right".
Anyway, for this part it seems ok, it's just 1 or 2 lines more.
Ack
Lucas De Marchi
Hi Lucas,
On Mon, Apr 29, 2013 at 7:39 PM, Lucas De Marchi
<[email protected]> wrote:
> On Mon, Apr 29, 2013 at 11:45 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi,
>>
>> On Thu, Apr 18, 2013 at 11:47 PM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> Calling g_dbus_client_new followed by g_dbus_client_set_proxy_handlers
>>> cause two calls to GetManagedObjects in a row as GetNameOwner reply is
>>> asyncronously it triggers the second call because the handlers have
>>> been set by g_dbus_client_set_proxy_handlers.
>>> ---
>>> gdbus/client.c | 25 ++++++++++++++++++-------
>>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/gdbus/client.c b/gdbus/client.c
>>> index c2d2346..55f1d89 100644
>>> --- a/gdbus/client.c
>>> +++ b/gdbus/client.c
>>> @@ -40,6 +40,7 @@ struct GDBusClient {
>>> char *base_path;
>>> GPtrArray *match_rules;
>>> DBusPendingCall *pending_call;
>>> + DBusPendingCall *get_objects_call;
>>> GDBusWatchFunction connect_func;
>>> void *connect_data;
>>> GDBusWatchFunction disconn_func;
>>> @@ -992,6 +993,8 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
>>> DBusMessage *reply = dbus_pending_call_steal_reply(call);
>>> DBusError error;
>>>
>>> + g_dbus_client_ref(client);
>>> +
>>> dbus_error_init(&error);
>>>
>>> if (dbus_set_error_from_message(&error, reply) == TRUE) {
>>> @@ -1004,19 +1007,24 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
>>> done:
>>> dbus_message_unref(reply);
>>>
>>> + dbus_pending_call_unref(client->get_objects_call);
>>> + client->get_objects_call = NULL;
>>> +
>>> g_dbus_client_unref(client);
>>> }
>>>
>>> static void get_managed_objects(GDBusClient *client)
>>> {
>>> DBusMessage *msg;
>>> - DBusPendingCall *call;
>>>
>>> if (!client->proxy_added && !client->proxy_removed) {
>>> refresh_properties(client);
>>> return;
>>> }
>>>
>>> + if (client->get_objects_call != NULL)
>>> + return;
>>> +
>>> msg = dbus_message_new_method_call(client->service_name, "/",
>>> DBUS_INTERFACE_DBUS ".ObjectManager",
>>> "GetManagedObjects");
>>> @@ -1026,16 +1034,14 @@ static void get_managed_objects(GDBusClient *client)
>>> dbus_message_append_args(msg, DBUS_TYPE_INVALID);
>>>
>>> if (dbus_connection_send_with_reply(client->dbus_conn, msg,
>>> - &call, -1) == FALSE) {
>>> + &client->get_objects_call, -1) == FALSE) {
>>> dbus_message_unref(msg);
>>> return;
>>> }
>>>
>>> - g_dbus_client_ref(client);
>>> -
>>> - dbus_pending_call_set_notify(call, get_managed_objects_reply,
>>> - client, NULL);
>>> - dbus_pending_call_unref(call);
>>> + dbus_pending_call_set_notify(client->get_objects_call,
>>> + get_managed_objects_reply,
>>> + client, NULL);
>
> Why don't you let the unref here, so you don't need to do it in
> g_dbus_client_unref() and get_managed_objects_reply(). This way the
> only ref is owned by libdbus which will free it when it's done.
But that means we have no control over any reference and from libdbus
perspective it can free whenever it likes which I think could break in
case where the callback is not called so I find it safer not to abuse
it here. Note that in get_name_owner we do the same, although some
other parts of the code it seems to not get this right and would
probably break in case the user data is freed before the pending call
completes.
--
Luiz Augusto von Dentz
On Mon, Apr 29, 2013 at 11:45 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi,
>
> On Thu, Apr 18, 2013 at 11:47 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> Calling g_dbus_client_new followed by g_dbus_client_set_proxy_handlers
>> cause two calls to GetManagedObjects in a row as GetNameOwner reply is
>> asyncronously it triggers the second call because the handlers have
>> been set by g_dbus_client_set_proxy_handlers.
>> ---
>> gdbus/client.c | 25 ++++++++++++++++++-------
>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdbus/client.c b/gdbus/client.c
>> index c2d2346..55f1d89 100644
>> --- a/gdbus/client.c
>> +++ b/gdbus/client.c
>> @@ -40,6 +40,7 @@ struct GDBusClient {
>> char *base_path;
>> GPtrArray *match_rules;
>> DBusPendingCall *pending_call;
>> + DBusPendingCall *get_objects_call;
>> GDBusWatchFunction connect_func;
>> void *connect_data;
>> GDBusWatchFunction disconn_func;
>> @@ -992,6 +993,8 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
>> DBusMessage *reply = dbus_pending_call_steal_reply(call);
>> DBusError error;
>>
>> + g_dbus_client_ref(client);
>> +
>> dbus_error_init(&error);
>>
>> if (dbus_set_error_from_message(&error, reply) == TRUE) {
>> @@ -1004,19 +1007,24 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
>> done:
>> dbus_message_unref(reply);
>>
>> + dbus_pending_call_unref(client->get_objects_call);
>> + client->get_objects_call = NULL;
>> +
>> g_dbus_client_unref(client);
>> }
>>
>> static void get_managed_objects(GDBusClient *client)
>> {
>> DBusMessage *msg;
>> - DBusPendingCall *call;
>>
>> if (!client->proxy_added && !client->proxy_removed) {
>> refresh_properties(client);
>> return;
>> }
>>
>> + if (client->get_objects_call != NULL)
>> + return;
>> +
>> msg = dbus_message_new_method_call(client->service_name, "/",
>> DBUS_INTERFACE_DBUS ".ObjectManager",
>> "GetManagedObjects");
>> @@ -1026,16 +1034,14 @@ static void get_managed_objects(GDBusClient *client)
>> dbus_message_append_args(msg, DBUS_TYPE_INVALID);
>>
>> if (dbus_connection_send_with_reply(client->dbus_conn, msg,
>> - &call, -1) == FALSE) {
>> + &client->get_objects_call, -1) == FALSE) {
>> dbus_message_unref(msg);
>> return;
>> }
>>
>> - g_dbus_client_ref(client);
>> -
>> - dbus_pending_call_set_notify(call, get_managed_objects_reply,
>> - client, NULL);
>> - dbus_pending_call_unref(call);
>> + dbus_pending_call_set_notify(client->get_objects_call,
>> + get_managed_objects_reply,
>> + client, NULL);
Why don't you let the unref here, so you don't need to do it in
g_dbus_client_unref() and get_managed_objects_reply(). This way the
only ref is owned by libdbus which will free it when it's done.
Lucas De Marchi
Hi,
On Thu, Apr 18, 2013 at 11:47 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> Calling g_dbus_client_new followed by g_dbus_client_set_proxy_handlers
> cause two calls to GetManagedObjects in a row as GetNameOwner reply is
> asyncronously it triggers the second call because the handlers have
> been set by g_dbus_client_set_proxy_handlers.
> ---
> gdbus/client.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/gdbus/client.c b/gdbus/client.c
> index c2d2346..55f1d89 100644
> --- a/gdbus/client.c
> +++ b/gdbus/client.c
> @@ -40,6 +40,7 @@ struct GDBusClient {
> char *base_path;
> GPtrArray *match_rules;
> DBusPendingCall *pending_call;
> + DBusPendingCall *get_objects_call;
> GDBusWatchFunction connect_func;
> void *connect_data;
> GDBusWatchFunction disconn_func;
> @@ -992,6 +993,8 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
> DBusMessage *reply = dbus_pending_call_steal_reply(call);
> DBusError error;
>
> + g_dbus_client_ref(client);
> +
> dbus_error_init(&error);
>
> if (dbus_set_error_from_message(&error, reply) == TRUE) {
> @@ -1004,19 +1007,24 @@ static void get_managed_objects_reply(DBusPendingCall *call, void *user_data)
> done:
> dbus_message_unref(reply);
>
> + dbus_pending_call_unref(client->get_objects_call);
> + client->get_objects_call = NULL;
> +
> g_dbus_client_unref(client);
> }
>
> static void get_managed_objects(GDBusClient *client)
> {
> DBusMessage *msg;
> - DBusPendingCall *call;
>
> if (!client->proxy_added && !client->proxy_removed) {
> refresh_properties(client);
> return;
> }
>
> + if (client->get_objects_call != NULL)
> + return;
> +
> msg = dbus_message_new_method_call(client->service_name, "/",
> DBUS_INTERFACE_DBUS ".ObjectManager",
> "GetManagedObjects");
> @@ -1026,16 +1034,14 @@ static void get_managed_objects(GDBusClient *client)
> dbus_message_append_args(msg, DBUS_TYPE_INVALID);
>
> if (dbus_connection_send_with_reply(client->dbus_conn, msg,
> - &call, -1) == FALSE) {
> + &client->get_objects_call, -1) == FALSE) {
> dbus_message_unref(msg);
> return;
> }
>
> - g_dbus_client_ref(client);
> -
> - dbus_pending_call_set_notify(call, get_managed_objects_reply,
> - client, NULL);
> - dbus_pending_call_unref(call);
> + dbus_pending_call_set_notify(client->get_objects_call,
> + get_managed_objects_reply,
> + client, NULL);
>
> dbus_message_unref(msg);
> }
> @@ -1285,6 +1291,11 @@ void g_dbus_client_unref(GDBusClient *client)
> dbus_pending_call_unref(client->pending_call);
> }
>
> + if (client->get_objects_call != NULL) {
> + dbus_pending_call_cancel(client->get_objects_call);
> + dbus_pending_call_unref(client->get_objects_call);
> + }
> +
> for (i = 0; i < client->match_rules->len; i++) {
> modify_match(client->dbus_conn, "RemoveMatch",
> g_ptr_array_index(client->match_rules, i));
> --
> 1.8.1.4
Anyone against pushing this one upstream? Otherwise I will be pushing
it latter today.
--
Luiz Augusto von Dentz