Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1366318045-22261-1-git-send-email-luiz.dentz@gmail.com> From: Lucas De Marchi Date: Mon, 29 Apr 2013 15:00:38 -0300 Message-ID: Subject: Re: [PATCH BlueZ] gdbus: Fix calling GetManagedObjects twice in a row To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Mon, Apr 29, 2013 at 2:34 PM, Luiz Augusto von Dentz wrote: > Hi Lucas, > > On Mon, Apr 29, 2013 at 7:39 PM, Lucas De Marchi > wrote: >> On Mon, Apr 29, 2013 at 11:45 AM, Luiz Augusto von Dentz >> wrote: >>> Hi, >>> >>> On Thu, Apr 18, 2013 at 11:47 PM, Luiz Augusto von Dentz >>> wrote: >>>> From: Luiz Augusto von Dentz >>>> >>>> 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