GNOME Bugzilla – Bug 790714
Fix regression and issues from patchset in bug #782530
Last modified: 2017-11-23 12:21:10 UTC
The patchset in bug #782530 fixed a race condition. Unfortunately some regressions were introduce.
Created attachment 364203 [details] [review] lib: Fix parameters of interface_removed call Patch 17983ace (lib: Use GDBusObjectManager) ported the interface addition and removal to GDBusObjectManager. However the parameter list of interface_remove was not updated correctly.
Created attachment 364204 [details] [review] lib: Listen to object addition and removal on the bus Patch 17983ace (lib: Use GDBusObjectManager) only added listeners for interface-added and interface-removed. However, as GDBusObjectManager simplifies the API to emit object-added/-removed those have to be listened for changes too. Without this the list of bluetooth devices and adapters is never updated after the initial object listing.
Created attachment 364205 [details] [review] lib: Keep references to adapters and unused devices Patch 17983ace (lib: Use GDBusObjectManager) changes the logic of device_add and adapter_add so that the device/adapter object is passsed in. Because of that the object must not be unref'ed anymore, as it is owned by the surrounding code (i.e. GDBusObjectManager).
Review of attachment 364203 [details] [review]: Ha, signal callbacks and C :/ Can you please change the subject to something more like: "lib: Fix crash when disconnecting certain devices" Which I think is the only case where bluetoothd removes interfaces from devices.
Review of attachment 364204 [details] [review]: > Listen to object addition and removal on the bus > Without this the list of bluetooth devices and adapters is never updated > after the initial object listing. Can you please put this explanation in the commit subject? I prefer the subject to show the impact of the patch, and the body to contain how it was fixed. Makes writing NEWS files, and backporting patches easier. ::: lib/bluetooth-client.c @@ +733,3 @@ + BluetoothClient *client) +{ + GList *interfaces, *li; Please use "*l" for the iterator instead of "li". @@ +737,3 @@ + interfaces = g_dbus_object_get_interfaces (object); + li = interfaces; + while (li) { I prefer: for (l = interfaces; l != NULL; l = l->next) rather than a while loop. If you find a way to reduce the code duplication between those 2 functions, that would also be nice (I couldn't find something that would end up being shorter and more readable on top of my head).
Review of attachment 364205 [details] [review]: > lib: Keep references to adapters and unused devices "lib: Fix possible crash when devices or adapters are added" ? Looks good otherwise.
Created attachment 364220 [details] [review] lib: Fix updating device and adapter list after initialization Patch 17983ace (lib: Use GDBusObjectManager) only added listeners for interface-added and interface-removed. However, as GDBusObjectManager simplifies the API to emit object-added/-removed those have to be listened for changes too. Without this the list of bluetooth devices and adapters is never updated after the initial object listing.
Review of attachment 364220 [details] [review]: ::: lib/bluetooth-client.c @@ +732,3 @@ + + for (l = interfaces; l != NULL; l = l->next) + interface_added (manager, object, G_DBUG_INTERFACE (l->data), client); Can you please compile-test the patches before uploading them? :) I doubt G_DBUG_INTERFACE is going to work.
Created attachment 364226 [details] [review] lib: Fix updating device and adapter list after initialization Patch 17983ace (lib: Use GDBusObjectManager) only added listeners for interface-added and interface-removed. However, as GDBusObjectManager simplifies the API to emit object-added/-removed those have to be listened for changes too. Without this the list of bluetooth devices and adapters is never updated after the initial object listing.
(In reply to Bastien Nocera from comment #8) > Can you please compile-test the patches before uploading them? :) > I doubt G_DBUG_INTERFACE is going to work. Haha, and I was sure I had tested that change together with the agent stuff :)
Review of attachment 364226 [details] [review]: Looks good.
The following fix has been pushed: d09e649 lib: Fix updating device and adapter list after initialization
Created attachment 364268 [details] [review] lib: Fix updating device and adapter list after initialization Patch 17983ace (lib: Use GDBusObjectManager) only added listeners for interface-added and interface-removed. However, as GDBusObjectManager simplifies the API to emit object-added/-removed those have to be listened for changes too. Without this the list of bluetooth devices and adapters is never updated after the initial object listing.