GNOME Bugzilla – Bug 790720
Fix agent re-registration
Last modified: 2017-11-23 12:28:53 UTC
The agent is being registered twice and then only unregistered once. This causes issue when e.g. switching away from the bluetooth panel and back again in gnome-control-center. The patchset fixes at least a warning (and I assume pairing, but didn't verify it was broken before).
Created attachment 364221 [details] [review] lib: Fix agent re-registration by removing duplicate registration bluetooth_agent_setup would register the object at a user choosen location as bluetooth_agent_register did the same for a fixed location (also registering the agent with bluez) the object would only be removed from one of the two locations. Effectively, the only thing that bluetooth_agent_setup did was setting custom DBus path for the object. So remove bluetooth_agent_setup and instead make the path a construct only property. This fixes agent registration when switching away and back to the bluetooth panel in gnome-control-center.
Created attachment 364222 [details] [review] lib: Remove spurious weak pointer reference to bluetooth agent The bluetooth widget holds a single reference to the agent object. This reference is unref'ed and cleard using g_object_cleared in all relevant cases. This pointer should not be a weak reference. It will not cause issues simply because agent will always be destroyed at the same time as the widget.
Review of attachment 364221 [details] [review]: Looks good otherwise. Can you please also mention the warning you got when navigating, or a backtrace of it? ::: lib/bluetooth-agent.c @@ +112,3 @@ +enum { + PROP_0, Remove those line feeds between the enum members. @@ +119,3 @@ +}; + +static GParamSpec *props[PROP_LAST] = {NULL, }; I don't think you need to initialise this. First time I see this in glib/gtk+ libraries. @@ +327,3 @@ + + switch (prop_id) { + case PROP_PATH: "case" is supposed to be aligned with "switch" above @@ +329,3 @@ + case PROP_PATH: + g_value_set_string (value, priv->path); + break; The break; is supposed to be aligned with the g_value_... call above. @@ +330,3 @@ + g_value_set_string (value, priv->path); + break; + No need for that linefeed. @@ +349,3 @@ + priv->path = g_value_dup_string (value); + break; + Ditto about the linefeed. @@ +411,2 @@ { + if (!path) path == NULL @@ +411,3 @@ { + if (!path) + path = BLUEZ_AGENT_PATH; return g_object_new... without the "path" property, no? Otherwise we have a default in 2 places.
Review of attachment 364221 [details] [review]: Thinking about it, does it work now, and did it work before to run this when bluetoothd wasn't running? Looking at the code, I think that the idea was that it could handle that case, but I'm pretty sure that broke, over the years.
Review of attachment 364222 [details] [review]: > cleard cleared Looks correct otherwise, thanks.
(In reply to Bastien Nocera from comment #4) > Review of attachment 364221 [details] [review] [review]: > Thinking about it, does it work now, and did it work before to run this when > bluetoothd wasn't running? Looking at the code, I think that the idea was > that it could handle that case, but I'm pretty sure that broke, over the > years. Hm, not sure what the expected behaviour is. But looking at bluetooth_agent_register the behaviour was and is to error out and free the agent again if the either the registration or setting the agent as the default fails. I did verify that I could pair a device after switching away from the bluetooth settings panel and back to it again.
To be honest, it feels like the whole bluetooth_agent_setup function should have been removed years ago and its usage only survived because it was the only function that would accept a different path.
Created attachment 364227 [details] [review] lib: Fix agent re-registration by removing duplicate registration bluetooth_agent_setup would register the object at a user choosen location as bluetooth_agent_register did the same for a fixed location (also registering the agent with bluez) the object would only be removed from one of the two locations. Effectively, the only thing that bluetooth_agent_setup did was setting custom DBus path for the object. So remove bluetooth_agent_setup and instead make the path a construct only property. This fixes agent registration when switching away and back to the bluetooth panel in gnome-control-center. Before the following warning would be printed and the agent registration would fail: Bluetooth-WARNING **: Failed to register object: An object is already exported for the interface org.bluez.Agent1 at /org/bluez/agent/gnome
Review of attachment 364227 [details] [review]: > choosen chosen Please add "()" after function names, makes it easier to figure out they're functions when reading.
The following fixes have been pushed: a143c87 lib: Fix agent re-registration by removing duplicate registration 33bc704 lib: Remove spurious weak pointer reference to bluetooth agent
Created attachment 364271 [details] [review] lib: Fix agent re-registration by removing duplicate registration bluetooth_agent_setup() would register the object at a user chosen location as bluetooth_agent_register() did the same for a fixed location (also registering the agent with bluez) the object would only be removed from one of the two locations. Effectively, the only thing that bluetooth_agent_setup() did was setting custom DBus path for the object. So remove bluetooth_agent_setup() and instead make the path a construct only property. This fixes agent registration when switching away and back to the bluetooth panel in gnome-control-center. Before the following warning would be printed and the agent registration would fail: Bluetooth-WARNING **: Failed to register object: An object is already exported for the interface org.bluez.Agent1 at /org/bluez/agent/gnome
Created attachment 364272 [details] [review] lib: Remove spurious weak pointer reference to bluetooth agent The bluetooth widget holds a single reference to the agent object. This reference is unref'ed and cleared using g_object_cleared() in all relevant cases. This pointer should not be a weak reference. It will not cause issues simply because agent will always be destroyed at the same time as the widget.