After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 741572 - [PATCH] Don't autolaunch logind; wait for logind to be ready before taking inhibitor
[PATCH] Don't autolaunch logind; wait for logind to be ready before taking in...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: nm-1.0
 
 
Reported: 2014-12-15 20:16 UTC by Dan Williams
Modified: 2015-01-12 15:51 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
0001-core-don-t-auto-launch-logind-bgo-741572.patch (2.82 KB, patch)
2014-12-15 20:19 UTC, Dan Williams 		none Details | Review
0001-core-don-t-auto-launch-logind-bgo-741572.patch (2.91 KB, patch)
2014-12-15 22:43 UTC, Dan Williams 		none Details | Review

Description Dan Williams 2014-12-15 20:16:04 UTC 
NM doesn't pass G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START to the proxy for logind, resulting in NM auto-launching logind when it starts.  Since logind might need the network for login information, this causes logind to be started before the network is up and apparently it doesn't like that.

Yeah, logind should just wait for the resources it actually requires to show up or something, but NM also doesn't need to auto-launch it either.

Nor does NM care about logind properties, so we can also pass G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES.  We can also make the proxy creation non-blocking, and since we need to wait for logind to show up anyway, react to name-owner changes by taking and dropping the inhibitor.
Comment 1 Dan Williams 2014-12-15 20:19:14 UTC 
Created attachment 292770 [details] [review]
0001-core-don-t-auto-launch-logind-bgo-741572.patch
Comment 2 Dan Williams 2014-12-15 20:19:37 UTC 
Does this seem OK as 1.0 material to everyone?
Comment 3 Dan Winship 2014-12-15 21:17:36 UTC 
looks good, and seems fine for 1.0 as long as it gets basic testing
Comment 4 Thomas Haller 2014-12-15 21:35:14 UTC 
usually we don't indent like this (do we?):

»···g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM,
»···                          G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START |
»···                              G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES,
»···                          NULL,


+    if (g_dbus_proxy_get_name_owner (self->sd_proxy))
+         take_inhibitor (self);

this leaks the result of get_name_owner() -- two places.


Rest LGTM. Me too: ACK to 1.0 after basic testing.
Comment 5 Dan Williams 2014-12-15 22:43:32 UTC 
(In reply to comment #4)
> usually we don't indent like this (do we?):
> 
> »···g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM,
> »···                          G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START |
> »···                              G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES,
> »···                          NULL,

You're right, we don't.  I'll change it.

> +    if (g_dbus_proxy_get_name_owner (self->sd_proxy))
> +         take_inhibitor (self);
> 
> this leaks the result of get_name_owner() -- two places.

Fixed.
Comment 6 Dan Williams 2014-12-15 22:43:47 UTC 
Created attachment 292779 [details] [review]
0001-core-don-t-auto-launch-logind-bgo-741572.patch
Comment 7 Dan Williams 2014-12-15 22:48:18 UTC 
Tested with restarting logind, and starting NM before logind is started, that logind is not auto-launched, and that NM reconnects to logind when it starts up.
Comment 8 Thomas Haller 2014-12-16 11:39:59 UTC 
LGTM
Comment 9 Dan Williams 2014-12-16 15:26:52 UTC 
master: 5a051ad71671b5808f7b57d5d2cdaaa7b4a66d67
1.0: c140f64b3982a5659ca4554f2b95d69a5733d2e7