GNOME Bugzilla – Bug 724041
[review] lr/applied-connection-bgo724041: refactor NetworkManager core, to create use a clone of the NMSettingConnection when activating a Device
Last modified: 2015-09-18 15:41:33 UTC
The idea is, that when a user modifies a connection that is currently active, this does not affect the currently activated device. For the changes to take effect, the user is expected to up the connection anew. In a second step (beyond this bug), it should be possible, that a strictly defined set of modifications are applied on the device immediately, without going through a full down-up trip. This bug however assumes for now, that any changes to an active connection have no consequences on the device. Exceptions to this rule will be defined later, as a separate RFE. The problem is now, that inside NM there is only one instance of NMSettingConnection, and while the NMDevice is active, it frequently consults the setting instance to get connection parameters. This can lead to subtle bugs, when the user modifies an active connection. The required solution to this is to create a clone of the setting when activation starts. This clone will be called the "applied connection" and is strictly tied to the NMActiveConnection of the device.
Created attachment 274896 [details] discussion from Brno We talked about this in Brno in February. Here's the result of the discussion. Hopefully someone can explain what it means...
Btw... just had a crash due to this... Have connection of type "ethernet" and activate it. Edit the keyfile to be a bond slave. Reload the connection. Now nm_device_get_connection() -- which was activated as ethernet connection -- returns a different type. In my case it crashed because it assumed there would be an IPv4 setting present.
*** Bug 743176 has been marked as a duplicate of this bug. ***
Created attachment 297020 [details] picture of brainstorming how to design this (brno, feb 2015, devconf)
In progress in lr/applied-connection-bgo724041 branch.
Ready for review a8a9074 device: add possibility to reapply ipv6 settings 6740d9d device: add possibility to reapply ipv4 settings a89a542 ethernet: add possibility to reapply the wired IPv4 MTU 4183279 cli: add nmcli d reapply 1da6ced libnm: add nm_device_reapply() eb887c3 device: add O.FD.NM.Device.Reapply() call 2c1a878 decice: add O.FD.NM.Device.GenerateSettings() call 7eecdda core: separate active and applied connection
>> core: separate active and applied connection maybe also rename nm_device_get_connection() and nm_active_connection_get_connection() to something like _get_settings_connection(). That name makes some sense, because we actually get an NMSettingsConnection instance. Also calling _get_applied_connection() is much more frequent then _get_connection(). So, I'd prefer either a.) _get_applied_connection() & _get_settings_connection() or b.) _get_connection() & _get_settings_connection() then c.) _get_applied_connection() & _get_connection() Also, there is for the user no way to access the applied connection. How about adding a read-only property to the NMActiveConnection D-Bus object to retrieve a connection hash? Or maybe make it a method() so that it looks more like an ~operation~ to retrieve the applied connection. Obviously, all secrets should be stripped (because secrets really are associated with the NMSettingsConnection) and the same access permissions as for reading _get_settings_connection() apply. >> decice: add O.FD.NM.Device.GenerateSettings() call In the doc: "Get the settings maps describing the currently effective network configuration for the device." maybe make it clearer, that this generates a completely new connection based on what is currently configured on the device. + if (NM_DEVICE_GET_PRIVATE (self)->act_request == NULL) { + error = g_error_new_literal (NM_DEVICE_ERROR, + NM_DEVICE_ERROR_NOT_ACTIVE, + "This device is not active"); + dbus_g_method_return_error (context, error); + g_error_free (error); + return; + } + /* Authorized */ + if (priv->state != NM_DEVICE_STATE_ACTIVATED) { + local = g_error_new_literal (NM_DEVICE_ERROR, + NM_DEVICE_ERROR_NOT_ACTIVE, + "Device is not activated"); + dbus_g_method_return_error (context, local); + g_error_free (local); no need to check the device state. You could potentially generate a connection on an unmanaged device. Just call generate_connection() and return whatever it gives. >> device: add O.FD.NM.Device.Reapply() call Hm... maybe, accetp an optional connection (path) that means: reassociate the NMActConnection with a different NMSettingsConnection. So, reapply should do: 1.) if a @connection is given, re-associate the _get_settings_connection() to point to the other connection. 2.) as you did, reapply the current _get_settings_connection(). + nm_connection_diff (connection, applied, NM_SETTING_COMPARE_FLAG_IGNORE_TIMESTAMP, &diffs); + + /* Everything set. */ + if (!diffs) + return TRUE; I think reapply should restore for example static IP addresses that were removed externally (via iproute2). In this case, the applied connection would have no @diffs, but there is still something to apply. Hence, do you actually care about the diff? I think the two steps above should be split further: 1) if a @connection is given, re-associate 2) merge _get_settings_connection() into _get_applied_connection(). You is in-memory only, you look at the diff and see what to do (and whether it is possible). 3) configure the system (platform) to reapply whatever we have in applied_connection(). all these steps are optional. I think the API for Reapply should make it possible to tackle them individually. Only step 3 can really fail in a bad way to leave you with a half-configured device. Both 1) and 2) you can check before hand and fail early if the connections are incompatible. Reapply(connection, do_apply, do_reconfigure); 0) check whether 1) and 2) will succeed of fail with error. 1) if @connection != NULL 2) if do_apply 3) if do_reconfigure + if (NM_DEVICE_GET_CLASS (self)->reapply) { + if (!NM_DEVICE_GET_CLASS (self)->reapp just add add dummy implementation of reapply() in the base class.
Only following up on the clone part now. Branch updated: 24bde83 activation-request: cease using the secrets if the original connection changed 90fd16a core: separate active and applied connection
(In reply to Lubomir Rintel from comment #8) > Only following up on the clone part now. Branch updated: > > 24bde83 activation-request: cease using the secrets if the original > connection changed > 90fd16a core: separate active and applied connection pushed 4 commits on your branch. including a: if (!check_connection_unmodified (self, &error)) { + /* FIXME: the callback must be invoked asynchnously, on an idle handler. + * We must also return a valid call_id. */ callback (self, 0, connection, error, callback_data); return 0;
Updated the branch
>> activation-request: cease using the secrets if the original connection changed I reworked nm_act_request_get_secrets() further and pushed a fixup.
(In reply to Thomas Haller from comment #11) > >> activation-request: cease using the secrets if the original connection changed > > > I reworked nm_act_request_get_secrets() further and pushed a fixup. Looks fine to me. Rebased & repushed
Rebased on master, and heavily reworked (again). The previous version is on branch lr/applied-connection-bgo724041-1 for reference.
> squash! core: separate active and applied connection It's getting confusing with 3 different connections for the ActiveConnection object. They should all get clearly explained in NMActiveConnectionPrivate. But is initial_connection only required for the add_and_activate case? I'm not sure that really needs/justifies a 3rd connection in ActiveConnection for only that use-case. If the only user is _add_and_activate_auth_done() then lets find another way to pass the NMConnection object into that function instead of leaking the implementation into NMActiveConnection. The 'userdata1' & userdata2 stuff was kind of a hack; maybe it's time to actually add closures to these functions. > core: separate active and applied connection I think the dispatcher script actions should use the applied connection? We want to report the settings that NM is actually using, not anything that change, I think. Not 100% sure, but dispatcher stuff seems like it should be based on the connection the device was activated with. Also I'm a bit concerned about canceling secrets requests when the connection has changed. I think this could mean that if I connect to wifi, and then say change the DNS servers (something entirely unrelated to wifi), and then the connection drops and NM asks for the password to reconnect, that my connection would simply fail for a seemingly random reason. eg, it failed because I changed something unrelated to wifi (rather odd) and second it failed because of something I potentially did 6 hours ago. Is there any way to tighten up the checks there, and only cancel secrets requests if they touch something relevant to the connection?
(In reply to Dan Williams from comment #14) > > squash! core: separate active and applied connection > > It's getting confusing with 3 different connections for the ActiveConnection > object. They should all get clearly explained in NMActiveConnectionPrivate. > > But is initial_connection only required for the add_and_activate case? I'm > not sure that really needs/justifies a 3rd connection in ActiveConnection > for only that use-case. If the only user is _add_and_activate_auth_done() > then lets find another way to pass the NMConnection object into that > function instead of leaking the implementation into NMActiveConnection. The > 'userdata1' & userdata2 stuff was kind of a hack; maybe it's time to > actually add closures to these functions. I dropped the initial_connection. See fixup. The issue is however not about using a closure (or lack of it). I hacked around that, by attaching the NMConnection via g_object_set_data() to the NMActiveConnection. The majore uglyness is that a NMActiveConnection can be created without settings_connection/applied_connection -- because we want to use nm_active_connection_authorize() also for add-and-activate. Changing that would be a larger refactoring (not for today). While I tried to do that, there is also a new fallout-commit "auth-utils: some refactoring in nm-auth-utils.c". > > core: separate active and applied connection > > I think the dispatcher script actions should use the applied connection? We > want to report the settings that NM is actually using, not anything that > change, I think. Not 100% sure, but dispatcher stuff seems like it should > be based on the connection the device was activated with. I agree. Fixup added. > Also I'm a bit concerned about canceling secrets requests when the > connection has changed. I think this could mean that if I connect to wifi, > and then say change the DNS servers (something entirely unrelated to wifi), > and then the connection drops and NM asks for the password to reconnect, > that my connection would simply fail for a seemingly random reason. eg, it > failed because I changed something unrelated to wifi (rather odd) and second > it failed because of something I potentially did 6 hours ago. Is there any > way to tighten up the checks there, and only cancel secrets requests if they > touch something relevant to the connection? Note the current situation is, that if the user modifies a connection that is currently active, he can crash NM. Let's do what you request here later. This is also related to the live-reconfig stuff, where some changes can be applied (e.g. add IP address) and others not (e.g. change connection.type). Similarly, some changes inhibit further secret-requests, others not. Rebased the branch on master (the previous version is still at lr/applied-connection-bgo724041-2). Please review.
(In reply to Thomas Haller from comment #15) > (In reply to Dan Williams from comment #14) > > > squash! core: separate active and applied connection > > > > It's getting confusing with 3 different connections for the ActiveConnection > > object. They should all get clearly explained in NMActiveConnectionPrivate. > > > > But is initial_connection only required for the add_and_activate case? I'm > > not sure that really needs/justifies a 3rd connection in ActiveConnection > > for only that use-case. If the only user is _add_and_activate_auth_done() > > then lets find another way to pass the NMConnection object into that > > function instead of leaking the implementation into NMActiveConnection. The > > 'userdata1' & userdata2 stuff was kind of a hack; maybe it's time to > > actually add closures to these functions. > > I dropped the initial_connection. See fixup. > > The issue is however not about using a closure (or lack of it). > I hacked around that, by attaching the NMConnection via g_object_set_data() > to the NMActiveConnection. Yeah, that's probably simpler than using a closure. > The majore uglyness is that a NMActiveConnection can be created without > settings_connection/applied_connection -- because we want to use > nm_active_connection_authorize() also for add-and-activate. > > Changing that would be a larger refactoring (not for today). While I tried > to do that, there is also a new fallout-commit "auth-utils: some refactoring > in nm-auth-utils.c". Not sure what we can do about that, except revert to the "PendingActivation" structure that we used to have, that was also very ugly. With AddAndActivate we fundamentally have to authorize before we can create the NMSettingsConnection, but we still need some object to track all the activations. Hopefully you'll have some better ideas than I on how to make that better. The only problem I see with the auth changes is that you've added another level to the 'data' hash table, but not updated nm_auth_chain_get_result() to handle that. I think you need: NMAuthCallResult nm_auth_chain_get_result (NMAuthChain *self, const char *permission) { ChainData *data; g_return_val_if_fail (self != NULL, NM_AUTH_CALL_RESULT_UNKNOWN); g_return_val_if_fail (permission != NULL, NM_AUTH_CALL_RESULT_UNKNOWN); data = g_hash_table_lookup (self->data, permission); return data ? GPOINTER_TO_UINT (data->data) : NM_AUTH_CALL_RESULT_UNKNOWN; } > > > core: separate active and applied connection > > > > I think the dispatcher script actions should use the applied connection? We > > want to report the settings that NM is actually using, not anything that > > change, I think. Not 100% sure, but dispatcher stuff seems like it should > > be based on the connection the device was activated with. > > I agree. Fixup added. > > > > Also I'm a bit concerned about canceling secrets requests when the > > connection has changed. I think this could mean that if I connect to wifi, > > and then say change the DNS servers (something entirely unrelated to wifi), > > and then the connection drops and NM asks for the password to reconnect, > > that my connection would simply fail for a seemingly random reason. eg, it > > failed because I changed something unrelated to wifi (rather odd) and second > > it failed because of something I potentially did 6 hours ago. Is there any > > way to tighten up the checks there, and only cancel secrets requests if they > > touch something relevant to the connection? > > Note the current situation is, that if the user modifies a connection that > is currently active, he can crash NM. > Let's do what you request here later. This is also related to the > live-reconfig stuff, where some changes can be applied (e.g. add IP address) > and others not (e.g. change connection.type). Similarly, some changes > inhibit further secret-requests, others not. Ok, I buy the explanation, but the implementation needs fixing because I can't autoconnect wifi right now: NetworkManager[12688]: <info> Auto-activating connection 'home wifi'. NetworkManager[12688]: <info> (wlp4s0): Activation: starting connection 'home wifi' (364ce74f-9370-439b-bc8c-202e4b7b40a7) NetworkManager[12688]: <info> (wlp4s0): device state change: disconnected -> prepare (reason 'none') [30 40 0] NetworkManager[12688]: <info> NetworkManager state is now CONNECTING NetworkManager[12688]: <info> (wlp4s0): device state change: prepare -> config (reason 'none') [40 50 0] NetworkManager[12688]: <info> (wlp4s0): Activation: (wifi) access point 'home wifi' has security, but secrets are required. NetworkManager[12688]: <info> (wlp4s0): device state change: config -> need-auth (reason 'none') [50 60 0] NetworkManager[12688]: <debug> [1441144971.551624] [settings/nm-settings-connection.c:879] agent_secrets_done_cb(): settings-connection[0x1d05f80,364ce74f-9370-439b-bc8c-202e4b7b40a7]: (802-11-wireless-security:1) secrets request completed NetworkManager[12688]: <debug> [1441144971.552565] [settings/nm-settings-connection.c:922] agent_secrets_done_cb(): settings-connection[0x1d05f80,364ce74f-9370-439b-bc8c-202e4b7b40a7]: (802-11-wireless-security:1) new agent secrets processed NetworkManager[12688]: <warn> (wlp4s0): The connection was modified since requesting secrets NetworkManager[12688]: <info> (wlp4s0): device state change: need-auth -> failed (reason 'no-secrets') [60 120 7] NetworkManager[12688]: <debug> [1441144971.553223] [nm-policy.c:1133] device_state_changed(): Connection 'home wifi' now blocked from autoconnect due to no secrets NetworkManager[12688]: <warn> (wlp4s0): Activation: failed for connection 'home wifi' Something in nm-active-connection.c::get_secrets_cb() isn't right; shouldn't it be: if (!error && nm_active_connection_is_modified (NM_ACTIVE_CONNECTION (info->self))) { g_set_error_literal (&local, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, "The connection was modified since requesting secrets"); error = local; } but even after that, the secrets don't get used. That's because NMActRequest gets secrets on the *settings* connection, and so the secrets are updated in the *settings* connection. But then NMDeviceWifi checks for secrets on the *applied* connection, where they don't exist. So I think we need another audit of the places that request secrets, what connection they request the secrets on, and what connection they actually pull secrets from for their operation. Basically anything that calls nm_act_request_get_secrets() needs a double-check here.
did another refactoring round. Out of that there emerged bug 754508 (th/secret-requests-bgo754508). Rebased lr/applied-connection-bgo724041 on top of that, the original version is still at lr/applied-connection-bgo724041-3. Needs review and testing.
> vpn-connection: refactor cancellation of secrets + if (priv->secrets_id) { + nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (priv->connection), + priv->secrets_id); + if (priv->secrets_id) { + priv->secrets_id = NULL; + g_return_if_reached (); + } I think I see what's going on here, but its weird and you don't immediately know that priv->secrets_id can change between these two statements. I don't know what we can do about it (since g_return_if_fail() can be disabled with G_DISABLE_CHECKS) but it's odd. > secrets: make agent-manager independent from NMSettingsConnection It seems really odd to split out the dbus path from the NMConnection, since we should never be requesting secrets from a connection without a path, or from a connection that doesn't match @path. And looking at the code, it seems we never actually call agent manager functions with different arguments for path and connection: call_id_a = nm_agent_manager_get_secrets (priv->agent_mgr, nm_connection_get_path (NM_CONNECTION (self)), NM_CONNECTION (self), the other places that use nm_agent_manager_*() all clone the connection, fix something up, and then send it to the agent manager. Clone copies the path too though, so the path is there already. for_agent = nm_simple_connection_new_clone (NM_CONNECTION (connection)); nm_connection_clear_secrets_with_flags (for_agent, secrets_filter_cb, GUINT_TO_POINTER (NM_SETTING_SECRET_FLAG_AGENT_OWNED)); nm_agent_manager_save_secrets (priv->agent_mgr, nm_connection_get_path (NM_CONNECTION (connection)), for_agent, subject); --- for_agents = nm_simple_connection_new_clone (NM_CONNECTION (self)); nm_connection_clear_secrets (for_agents); nm_agent_manager_delete_secrets (priv->agent_mgr, nm_connection_get_path (NM_CONNECTION (self)), for_agents); --- for_agent = nm_simple_connection_new_clone (NM_CONNECTION (self)); nm_connection_clear_secrets_with_flags (for_agent, secrets_filter_cb, GUINT_TO_POINTER (NM_SETTING_SECRET_FLAG_AGENT_OWNED)); nm_agent_manager_save_secrets (info->agent_mgr, nm_connection_get_path (NM_CONNECTION (self)), for_agent, --- /* Tell agents to remove secrets for this connection */ nm_agent_manager_delete_secrets (priv->agent_mgr, nm_connection_get_path (NM_CONNECTION (self)), NM_CONNECTION (self)); So I guess I don't get the purpose of this patch; it seems like a NOP after all the fixups? I think the benefits of enforcing the API contract that the connection and path match are more important... > core: separate active and applied connection > fixup! core: separate active and applied connection if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { if (priv->is_disposing) { /* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously * when the user calls nm_act_request_cancel_secrets(). */ g_set_error_literal (&local, G_IO_ERROR, G_IO_ERROR_NOT_CONNECTED, "Disposing NMActRequest instance"); } else { g_set_error_literal (&local, G_IO_ERROR, G_IO_ERROR_CANCELLED, "Request cancelled"); } This is also odd; there's got to be a better way. Nothing actually checks G_IO_ERROR_NOT_CONNECTED, so is the intent here to simply use a different log message when disposing? I don't think that's worth putting complexity into get_secrets_cancel()/get_secrets_cb() which is far away from the disposing code... Even in the dispose case the secrets request really is canceled; and the callback shouldn't be doing anything besides freeing memory when it gets G_IO_ERROR_CANCELED because that's an internal error, not a user-action error. I like the propagation of the always-callback up past NMSettingsConnection though; that cleans up a lot of icky stuff.
(In reply to Dan Williams from comment #18) > > vpn-connection: refactor cancellation of secrets > > + if (priv->secrets_id) { > + nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION > (priv->connection), > + priv->secrets_id); > + if (priv->secrets_id) { > + priv->secrets_id = NULL; > + g_return_if_reached (); > + } > > I think I see what's going on here, but its weird and you don't immediately > know that priv->secrets_id can change between these two statements. I don't > know what we can do about it (since g_return_if_fail() can be disabled with > G_DISABLE_CHECKS) but it's odd. I changed that part in a fixup. > > secrets: make agent-manager independent from NMSettingsConnection > > It seems really odd to split out the dbus path from the NMConnection, since > we should never be requesting secrets from a connection without a path, or > from a connection that doesn't match @path. > > And looking at the code, it seems we never actually call agent manager > functions with different arguments for path and connection: > ... > So I guess I don't get the purpose of this patch; it seems like a NOP after > all the fixups? I think the benefits of enforcing the API contract that the > connection and path match are more important... Originally, I intended to pass the applied-connection instead of the settings-connection (and in the future, the settings-connection's path might no longer match the path of the applied-connection). I was aware that this currently doesn't actually matter and the change is a NOP. I kept it as a separate patch because I already did it and because IMO the property nm_connection_[gs]et_path() is a design error. A (plain) NMConnection shouldn't have a path and it was unexpected to me -- only NMSettingsConnection and NMRemoteConnection have D-Bus paths. > > core: separate active and applied connection > > fixup! core: separate active and applied connection > > if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { > if (priv->is_disposing) { > /* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously > * when the user calls nm_act_request_cancel_secrets(). */ > g_set_error_literal (&local, G_IO_ERROR, G_IO_ERROR_NOT_CONNECTED, > "Disposing NMActRequest instance"); > } else { > g_set_error_literal (&local, G_IO_ERROR, G_IO_ERROR_CANCELLED, > "Request cancelled"); > } > > This is also odd; there's got to be a better way. Nothing actually checks > G_IO_ERROR_NOT_CONNECTED, so is the intent here to simply use a different > log message when disposing? I don't think that's worth putting complexity > into get_secrets_cancel()/get_secrets_cb() which is far away from the > disposing code... Even in the dispose case the secrets request really is > canceled; and the callback shouldn't be doing anything besides freeing > memory when it gets G_IO_ERROR_CANCELED because that's an internal error, > not a user-action error. I think the reason G_IO_ERROR_CANCELLED should only be reported if the user cancelled a request via nm_act_request_cancel_secrets(). Disposing the NMActRequest with pending operations is something different and should be treated differently. While you criticize here NMActRequest, cancelling operations in NMSecretAgent, NMAgentManager, and NMSettingsConnection now all work exactly the same w.r.t. disposing-with-pending-operations (grep for G_IO_ERROR_FAILED). That is, disposing-with-pending-operations is different from a user-cancellation, it's some kind of internal error. In the user-cancellation case, the user calls nm_act_request_cancel_secrets() and expects a synchronous call with G_IO_ERROR_CANCELLED. In the disposing-with-pending-operations case, the user must make up his mind how to handle the case(*), but it's different from a user-cancellation. (*) The easiest way is to avoid the situation alltogether, by ensuring that the lifetime of the NMActRequest exceeds any pending operation. I didn't change this, but added a fixup to use consistently G_IO_ERROR_FAILED instead. I could use a another error code for this particular reason...(??) > I like the propagation of the always-callback up past NMSettingsConnection > though; that cleans up a lot of icky stuff. Repushed.
(In reply to Thomas Haller from comment #19) > (In reply to Dan Williams from comment #18) } > > > > This is also odd; there's got to be a better way. Nothing actually checks > > G_IO_ERROR_NOT_CONNECTED, so is the intent here to simply use a different > > log message when disposing? I don't think that's worth putting complexity > > into get_secrets_cancel()/get_secrets_cb() which is far away from the > > disposing code... Even in the dispose case the secrets request really is > > canceled; and the callback shouldn't be doing anything besides freeing > > memory when it gets G_IO_ERROR_CANCELED because that's an internal error, > > not a user-action error. > > > I think the reason G_IO_ERROR_CANCELLED should only be reported if the user > cancelled a request via nm_act_request_cancel_secrets(). Disposing the > NMActRequest with pending operations is something different and should be > treated differently. > > While you criticize here NMActRequest, cancelling operations in > NMSecretAgent, NMAgentManager, and NMSettingsConnection now all work exactly > the same w.r.t. disposing-with-pending-operations (grep for > G_IO_ERROR_FAILED). > That is, disposing-with-pending-operations is different from a > user-cancellation, it's some kind of internal error. > > In the user-cancellation case, the user calls > nm_act_request_cancel_secrets() and expects a synchronous call with > G_IO_ERROR_CANCELLED. > In the disposing-with-pending-operations case, the user must make up his > mind how to handle the case(*), but it's different from a user-cancellation. > > (*) The easiest way is to avoid the situation alltogether, by ensuring that > the lifetime of the NMActRequest exceeds any pending operation. > > > I didn't change this, but added a fixup to use consistently > G_IO_ERROR_FAILED instead. I could use a another error code for this > particular reason...(??) Ok, refactored something to move the handling of cancellation out for get_secrets_cb() to where we call nm_settings_connection_cancel_secrets(). See 2 new fixup commits. I like it better now. How is it?
Yeah, that's better, thanks. Fixups LGTM; I verified that it no longer has the problems the original did when connecting to WiFi. So if it passes some Beaker runs, merge it :)
rebased to master and squashed all fixup commits.
Hm, we the help argument like: nmcli device set { ARGUMENTS | help } That means, if you name a device "help", you cannot use the command. I think we need a nmcli device set { [ifname] <ifname> ARGUMENTS | help } (same for other commands that involve device names).
(In reply to Thomas Haller from comment #23) commented on wrong bug. Please ignore :)
in the meantime, th/secret-requests-bgo754508 got merged to master. Rebased lr/applied-connection-bgo724041 on master again. Noticed other issue with handling updates to the firewall-zone and metered changes. Added two fixups for that...
merged now to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=24dc78957f082b12637b22bd1e5f803c7657af44