Bug 1401515

For a different take on this, why not just use the checkpoint feature?

Any time you want to make configuration changes that don't have an immediate effect, start a checkpoint and pass a new ONLY_DO_STUFF_WHEN_I_CALL_COMMIT flag.  Make your changes; none of them would have an effect yet because of the flag.  Then call the Commit() action which then allows all the changes to have an effect.

Then if for whatever reason that breaks networking, the rollback timeout would still be in force until the checkpoint was made permanent or something like that.

(In reply to Dan Williams from comment #2)
> For a different take on this, why not just use the checkpoint feature?

That seems an orthogonal solution to me

> Any time you want to make configuration changes that don't have an immediate
> effect, start a checkpoint and pass a new ONLY_DO_STUFF_WHEN_I_CALL_COMMIT
> flag.  Make your changes; none of them would have an effect yet because of
> the flag.  Then call the Commit() action which then allows all the changes
> to have an effect.

Hm, that would extend the current checkpoint/rollback semantic. Currently, checkpoint just records the current state, and rollback restores that state. Between checkpoint/rollback, all actions are performed as usual and are globally visible
(I like the simplicity of that).

ONLY_DO_STUFF_WHEN_I_CALL_COMMIT sounds like, allowing the user to apply multiple changes that only take effect when commit is called -- very much like a database transation. That sounds super complicated to me.

If multiple users start such transations, would they see each others changes? (Isolation-level?)

 
> Then if for whatever reason that breaks networking, the rollback timeout
> would still be in force until the checkpoint was made permanent or something
> like that.


I see one part of NetworkManager to have a global connection settings storage and API to manage these profiles. That is, all the settings exposed as /org/freedesktop/NetworkManager/Settings/*.
This RFE is to add more API to allow managing this global-storage without *any* changes to the actual networking.

th/autoconnect-rh1401515 LGTM

th/user-block-autoconnect-rh1401515:

> policy: merge reset_autoconnect_*() functions

+       if (   only_for_failed_secrets
+           && nm_settings_connection_autoconnect_blocked_reason_get (connection) == NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_NO_SECRETS)
+               continue;

Shouldn't it be

 if (   only_for_failed_secrets
     && !nm_settings_connection_autoconnect_blocked_reason_get (NO_SECRETS))

?

Ok, it's fixed by a later patch.

Also, the line is very long (137 columns). Can you split it, using
temporary variables if needed?


> policy: minor code cleanup in activate_slave_connections()

This introduces very long lines (> 130), which require scrolling for
me if I don't use the editor fully maximized.


The rest looks good!

> all: add new D-Bus API org.freedesktop.NetworkManager.Settings.Connection.Update2()

+       if (   ((guint32) ((NMSettingsUpdate2Flags) flags)) != flags

I don't understand this, what does it do?


     <!--
+        Update2:
         ...
+        Update the connection with new settings and properties (replacing all
+        previous settings and properties) and save the connection to disk. Secrets
                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
As I understand it, it saves only when the SAVE flag is passed or the
connection is already persistent, right? Can you explain it?


> settings: add NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT flag

+ *   has autoconnect enabled and is modified, it becomes ellegible to autoconnect

ellegible --> eligible


@@ -1713,64 +1713,68 @@ update_auth_cb (NMSettingsConnection *self,
...
+       nm_settings_connection_autoconnect_blocked_reason_set (self,
+                                                              NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_USER_REQUEST,
+                                                              NM_FLAGS_HAS (info->flags, NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT));

Isn't this a change in behavior? I mean, previously an Update()	would
not unblock a connection blocked by user request, but now it does.

(In reply to Beniamino Galvani from comment #6)
> > all: add new D-Bus API org.freedesktop.NetworkManager.Settings.Connection.Update2()
> 
> +       if (   ((guint32) ((NMSettingsUpdate2Flags) flags)) != flags
> 
> I don't understand this, what does it do?

It's there to catch a situation where sizeof(flags) < sizeof(guint32). I solved it differently.


>      <!--
> +        Update2:
>          ...
> +        Update the connection with new settings and properties (replacing
> all
> +        previous settings and properties) and save the connection to disk.
> Secrets
>                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> As I understand it, it saves only when the SAVE flag is passed or the
> connection is already persistent, right? Can you explain it?

better?

> > settings: add NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT flag
> 
> + *   has autoconnect enabled and is modified, it becomes ellegible to
> autoconnect
> 
> ellegible --> eligible

fixed

> @@ -1713,64 +1713,68 @@ update_auth_cb (NMSettingsConnection *self,
> ...
> +       nm_settings_connection_autoconnect_blocked_reason_set (self,
> +                                                             
> NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_USER_REQUEST,
> +                                                              NM_FLAGS_HAS
> (info->flags, NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT));
> 
> Isn't this a change in behavior? I mean, previously an Update()	would
> not unblock a connection blocked by user request, but now it does.

changed.


Thanks. Repushed.

> libnm: add nm_remote_connection_update2()

+/**
+ * nm_remote_connection_update2:
+ * @self: the #NMRemoteConnection

Should be @connection.


> settings: support setting a connection as volatile via Update2()

+       NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED         = (1LL <<  2),
+       NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY             = (1LL <<  3),

I'm not sure about the usefulness of these two flags, but I'm fine
with them if you think they are needed.

The rest LGTM.

merged upstream as:

master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=632e693251715526817b6fb27872df66b23595a4

nm-1-10: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ad7f1d18a0eafb1352d305035cd138f43a47b955


This extends SettingsConnection's Update() function with a new variant of update that allows to modify the settings with less interference with the current active connections:

  - Update2() can atomically (and temporarily) block autoconnect
  - Update2() can persist the connection to disk, or make it in-memory only (with
      3 variants, what should happen with a potential persisted profile on disk).
  - Update2() can also mark the connection as volatile. That is, it will be   
      automatically deleted once it's no longer used.


What is still missing is to extend AddConnection() API to allow blocking autoconnect (temporarily) and extend AddAndActivated() to create volatile connections. I am still marking this request as fixed. Let's open follow-up RFEs, if need be.

automated, tested, working well via gi

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:0778