Pushed bg/checkpoint-improvements-bgo770315 branch for review.

>> core: allow passing an applied connection to nm_act_request_new()
    

in set_property(), both setting PROP_INT_SETTINGS_CONNECTION and PROP_INT_APPLIED_CONNECTION will try to initialize priv->applied_connection.

That's IMO wrong. The setter should only remember the setting and it should be handled in the "constructed()" method.

constructed(...)
{
   ...
   if (!priv->applied_connection) {
       if (priv->settings_connection)
            priv->applied_connection = clone(priv->settings_connection);
   } else
       priv->applied_connection = clone(priv->applied_connection);




nm_settings_get_connection_by_uuid (nm_settings_get(), con_uuid);
whitespace                                 ^^^^^^^^^^^



+    gboolean realized;
+    gboolean unmanaged_explicit;

    bool realized:1;
?

>> checkpoint: better handle unmanaged and unrealized devices
    

+activate:
+         if (dev_checkpoint->state == NM_DEVICE_STATE_UNMANAGED) {

in that if-block, we should always "goto next_dev" and skip the following "if(dev_checkpoint->connection)" check.



>> checkpoint: consider all devices when an empty list is passed

get_all_device_paths() seems generally useful. Could we instead add a function nm_manager_get_device_paths() which does the same?





Rest lgtm

Fixed all the points above and repushed.

+    for (iter = devices; iter; iter = g_slist_next (iter)) {
+         path = nm_exported_object_get_path (NM_EXPORTED_OBJECT (iter->data));
+         g_ptr_array_add (paths, (gpointer) path);
+    }

hm, are we sure that devices only contains exported objects? That is not immediately clear to me.

Maybe

    if (path)
       g_ptr_array_add (paths, (gpointer) path);




>> core: allow passing an applied connection to nm_act_request_new()
    

+    case PROP_INT_APPLIED_CONNECTION:
+         /* construct-only */
+         acon = g_value_get_object (value);
+         if (acon)
+              priv->applied_connection = g_object_ref (acon);


In commit 2, I was about to say, that the applied-connection does not have to be cloned in set_property() and instead just taking a reference. But eventually I didn't say that because the caller must then ensure not to modify the connection after passing it to the active connection.
That is changed now which is better performance-wise. But maybe we could add a code-comment to nm_act_request_new(), _new_active_connection(), nm_manager_activate_connection() and point out, that the caller hands over the applied-connection and must not touch it afterwards (be he still must unref when done).


+    if (!priv->applied_connection && priv->settings_connection) {
+         priv->applied_connection =
+              nm_simple_connection_new_clone ((NMConnection *) priv->settings_connection);
+         nm_connection_clear_secrets (priv->applied_connection);
+    }

could we also clear the secrets, if the applied-connection is passed-in? Probably there are none, but just to be sure.

(In reply to Thomas Haller from comment #5)
> Maybe
> 
>     if (path)
>        g_ptr_array_add (paths, (gpointer) path);

> In commit 2, I was about to say, that the applied-connection does not have
> to be cloned in set_property() and instead just taking a reference. But
> eventually I didn't say that because the caller must then ensure not to
> modify the connection after passing it to the active connection.
> That is changed now which is better performance-wise. But maybe we could add
> a code-comment to nm_act_request_new(), _new_active_connection(),
> nm_manager_activate_connection() and point out, that the caller hands over
> the applied-connection and must not touch it afterwards (be he still must
> unref when done).

> could we also clear the secrets, if the applied-connection is passed-in?
> Probably there are none, but just to be sure.

All fixed, thanks.

lgtm

LGTM, found just a typo in commit msg:
"checkpoint: use UUID instead of patch to match connections"
                                   ^^^

(In reply to Francesco Giudici from comment #8)
> LGTM, found just a typo in commit msg:
> "checkpoint: use UUID instead of patch to match connections"
>                                    ^^^

Fixed and merged, thanks.

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d27ae4a8fec0c8f21a7171e01535f8beb8e5b105