On Mon, Apr 29, 2013 at 2:34 PM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Lucas,
>
> On Mon, Apr 29, 2013 at 7:39 PM, Lucas De Marchi
> <lucas.demarchi@xxxxxxxxxxxxxx> wrote:
>> On Mon, Apr 29, 2013 at 11:45 AM, Luiz Augusto von Dentz
>> <luiz.dentz@xxxxxxxxx> wrote:
>>> Hi,
>>>
>>> On Thu, Apr 18, 2013 at 11:47 PM, Luiz Augusto von Dentz
>>> <luiz.dentz@xxxxxxxxx> wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>>>>
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
