>> device: add O.FD.NM.Device.Reapply() call


    
this always tries to reapply the entire connection and errors out if there are any fields that cannot be applied.

An alternative would be to have flags *what* to reapply and ignore the rest.

Specifying no flags, is equal to "ALL".
Other then that, currently there are
  NM_REAPPLY_FLAG_WITH_METERED,
  NM_REAPPLY_FLAG_WITH_FIREWALL_ZONE,
  NM_REAPPLY_FLAG_WITH_IP4,
  NM_REAPPLY_FLAG_WITH_IP6,
  NM_REAPPLY_FLAG_WITH_APPLICABLE = METERED | FIREWALL_ZONE | IP4 | IP6,


Specifying no flags, should be equal to NM_REAPPLY_FLAG_WITH_APPLICABLE.

Of course, NM_REAPPLY_FLAG_WITH_IP4 still means more then one single property but instead addresses and routes.




Note that currently we already have a notion of NM_REAPPLY_FLAG_WITH_METERED and NM_REAPPLY_FLAG_WITH_FIREWALL_ZONE. Your patch should be aware of that and extend on that. That is, it should somehow merge with nm_device_reapply_settings_immediately().




Regardless of whether there is only "apply-all" or with "select-what-to-apply-flags":
I don't think that this algorithm of nm_device_reapply_connection() is correct. It might work in the simple case now, but there are interdependencies between settings. I think it is wrong to iterate over individual settings and apply them individually (in fact, the code iterates over the ~diff~ and reapply the diff). How about applying it as a whole?

I'd do:

nm_device_reapply(self, connection, flags, error)
{
    GError *local = NULL;

    /* check can-apply... */
    if (NM_FLAGS_HAS (flags, NM_REAPPLY_FLAG_WITH_IP4) {
         if (!can_reapply_ip4(self, connection, local)) {
              if (local) {
                   g_propagate_error(error, local);
                   return FALSE;
              }
              flags &= ~NM_REAPPLY_FLAG_WITH_IP4;
         }
    }
    ... check other flags...

    /* [1] */

   
    if (NM_FLAGS_HAS (flags, NM_REAPPLY_FLAG_WITH_IP4) {
       reapply_ip4 (self, connection);
    }
    ... apply other flags...

    return TRUE;
}






[1] At this point, I'd check for a special flag:

  NM_REAPPLY_FLAG_APPLY_ALL

If this flag is specified, then the connection can only be applied if the resulting applied-connection will be equal to the passed connection (basically, what your patch does!).
Well, actually: have NM_REAPPLY_FLAG_APPLY_ALL be the default, and add a flag
NM_REAPPLY_FLAG_APPLY_PARTIAL with inverse meaning.





how about the D-Bus method Reapply having 3 arguments:

  @settings-path (a D-Bus path)
  @connection (a connection hash)
  @flags (what to apply, from above).


- Specifying both @connection *and* @settings-path should be rejected with 
  error.
- If @connection is given, use that connection hash to reapply.
- If @settings-path is specified, use the connection under that path.
- If both @settings-path and @connection is missing, it equals to 
  @settings-path being the currently activated connection 
  (nm_connection_get_path(nm_active_connection_get_settings_connection()))


Passing ("/", NULL, 0) would be identical to what you implemented.

> libnm: add nm_device_reapply()

        nm_vpn_service_plugin_set_ip6_config;
        nm_vpn_service_plugin_set_login_banner;
+       nm_device_reapply;
+       nm_device_reapply_async;
+       nm_device_reapply_finish;

Keep these sorted.

+ * Returns: %TRUE on success, %FALSE on error, in which case @error will be set.
+ **/
+gboolean
+nm_device_reapply (NMDevice *device,
+                      GCancellable *cancellable,
+                      GError **error)

"Since: 1.2", indentation (for nm_device_reapply_finish() too).

> cli: add nmcli d reapply

Please also add bash completion for the new command.

> device: refactor the ip cleanup

+static void
+_cleanup_ip4_pre (NMDevice *self, CleanupType cleanup_type)
+{
+       NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
+
+       priv->ip4_state =  IP_NONE;

Extra space.

> device: add possibility to reapply the ipv4 settings

+       g_clear_object (&priv->con_ip4_config);
+       priv->con_ip4_config = nm_ip4_config_new (nm_device_get_ip_ifindex (self));

nitpick: g_object_unref() is enough since the pointer is immediately set later.

(In reply to Thomas Haller from comment #1)
> >> device: add O.FD.NM.Device.Reapply() call
> 
> 
>     
> this always tries to reapply the entire connection and errors out if there
> are any fields that cannot be applied.

Yeah. The user should just do an usual activation if this doesn't succeed.

> An alternative would be to have flags *what* to reapply and ignore the rest.
> 
> Specifying no flags, is equal to "ALL".
> Other then that, currently there are
>   NM_REAPPLY_FLAG_WITH_METERED,
>   NM_REAPPLY_FLAG_WITH_FIREWALL_ZONE,
>   NM_REAPPLY_FLAG_WITH_IP4,
>   NM_REAPPLY_FLAG_WITH_IP6,
>   NM_REAPPLY_FLAG_WITH_APPLICABLE = METERED | FIREWALL_ZONE | IP4 | IP6,
> 
> 
> Specifying no flags, should be equal to NM_REAPPLY_FLAG_WITH_APPLICABLE.
> 
> Of course, NM_REAPPLY_FLAG_WITH_IP4 still means more then one single
> property but instead addresses and routes.

I don't like this for multiple reasons and find it somehow overengineered.

Some of the flags seem to duplicate the connection settings the rest seems to be chosen rather arbitrarily. I'm having difficulties inventing a use case which would involve use of e.g. the NM_REAPPLY_FLAG_WITH_IP4 flag.

As for the connection hash, I now don't find it to be a sensible thing to do anymore, since I can't think of a sensible way to merge that with the applied connection (e.g. how to remove the properties and still provide a sane api)?

I also don't believe the user should be able to specify a different setting connection, since reasociating of the settings connection seems out of scope of this call. Maybe a separate method should be provided one day.

I still think we should have a flags argument just to have a possibility to extend this in the future, but I'd prefer if we didn't define any now.

> Note that currently we already have a notion of NM_REAPPLY_FLAG_WITH_METERED
> and NM_REAPPLY_FLAG_WITH_FIREWALL_ZONE. Your patch should be aware of that
> and extend on that. That is, it should somehow merge with
> nm_device_reapply_settings_immediately().
> 
> 
> 
> 
> Regardless of whether there is only "apply-all" or with
> "select-what-to-apply-flags":
> I don't think that this algorithm of nm_device_reapply_connection() is
> correct. It might work in the simple case now, but there are
> interdependencies between settings. I think it is wrong to iterate over
> individual settings and apply them individually (in fact, the code iterates
> over the ~diff~ and reapply the diff). How about applying it as a whole?
> 
> I'd do:
> 
> nm_device_reapply(self, connection, flags, error)
> {
>     GError *local = NULL;
> 
>     /* check can-apply... */
>     if (NM_FLAGS_HAS (flags, NM_REAPPLY_FLAG_WITH_IP4) {
>          if (!can_reapply_ip4(self, connection, local)) {
>               if (local) {
>                    g_propagate_error(error, local);
>                    return FALSE;
>               }
>               flags &= ~NM_REAPPLY_FLAG_WITH_IP4;
>          }
>     }
>     ... check other flags...
> 
>     /* [1] */
> 
>    
>     if (NM_FLAGS_HAS (flags, NM_REAPPLY_FLAG_WITH_IP4) {
>        reapply_ip4 (self, connection);
>     }
>     ... apply other flags...
> 
>     return TRUE;
> }

What interdependencies are there?

The above code doesn't look too good to me -- it burdens the client with specifying the flag.
> [1] At this point, I'd check for a special flag:
> 
>   NM_REAPPLY_FLAG_APPLY_ALL
> 
> If this flag is specified, then the connection can only be applied if the
> resulting applied-connection will be equal to the passed connection
> (basically, what your patch does!).
> Well, actually: have NM_REAPPLY_FLAG_APPLY_ALL be the default, and add a flag
> NM_REAPPLY_FLAG_APPLY_PARTIAL with inverse meaning.
> 
> 
> 
> 
> 
> how about the D-Bus method Reapply having 3 arguments:
> 
>   @settings-path (a D-Bus path)
>   @connection (a connection hash)
>   @flags (what to apply, from above).
> 
> 
> - Specifying both @connection *and* @settings-path should be rejected with 
>   error.
> - If @connection is given, use that connection hash to reapply.
> - If @settings-path is specified, use the connection under that path.
> - If both @settings-path and @connection is missing, it equals to 
>   @settings-path being the currently activated connection 
>   (nm_connection_get_path(nm_active_connection_get_settings_connection()))
> 
> 
> Passing ("/", NULL, 0) would be identical to what you implemented.

At this point I believe the use case of this is that client changes the connection and needs a way to try applying the changes without reconnection if possible.

Think adding an IP address in the control center or disabling dhcp.

I don't think it should keep track of the changes by the means of specifying the flags and I don't think we should provide an api which would involve any other settings connection. Furthermore the changes should be persisted so that the next invocation of the control center can present them on the ui which conflicts with the inline hash.

(In reply to Lubomir Rintel from comment #3)
> (In reply to Thomas Haller from comment #1)
> > >> device: add O.FD.NM.Device.Reapply() call
> > 
> > 
> >     
> > this always tries to reapply the entire connection and errors out if there
> > are any fields that cannot be applied.
> 
> Yeah. The user should just do an usual activation if this doesn't succeed.
> 
> > An alternative would be to have flags *what* to reapply and ignore the rest.
> > 
> > Specifying no flags, is equal to "ALL".
> > Other then that, currently there are
> >   NM_REAPPLY_FLAG_WITH_METERED,
> >   NM_REAPPLY_FLAG_WITH_FIREWALL_ZONE,
> >   NM_REAPPLY_FLAG_WITH_IP4,
> >   NM_REAPPLY_FLAG_WITH_IP6,
> >   NM_REAPPLY_FLAG_WITH_APPLICABLE = METERED | FIREWALL_ZONE | IP4 | IP6,
> > 
> > 
> > Specifying no flags, should be equal to NM_REAPPLY_FLAG_WITH_APPLICABLE.
> > 
> > Of course, NM_REAPPLY_FLAG_WITH_IP4 still means more then one single
> > property but instead addresses and routes.
> 
> I don't like this for multiple reasons and find it somehow overengineered.
> 
> Some of the flags seem to duplicate the connection settings the rest seems
> to be chosen rather arbitrarily. I'm having difficulties inventing a use
> case which would involve use of e.g. the NM_REAPPLY_FLAG_WITH_IP4 flag.


> As for the connection hash, I now don't find it to be a sensible thing to do
> anymore, since I can't think of a sensible way to merge that with the
> applied connection (e.g. how to remove the properties and still provide a
> sane api)?

There is nothing to merge here. The user provides a connection hash. NM creates a NMConnection out of it and passes it to reapply -- instead of the current nm_device_get_settings_connection().

It's about 15 lines of code.



> I also don't believe the user should be able to specify a different setting
> connection, since reasociating of the settings connection seems out of scope
> of this call. Maybe a separate method should be provided one day.

Passing a settings-connection-path can be emulated by the connection-hash paramter (except the permission question).

Also, a separate, future command would be the "Relink" or "Reattach" which swaps the pointer to the settings connection. Thus the same functionality could be also achieved with 3 steps:

  Relink(connection2)
  Reapply()
  Relink(connection1)



> I still think we should have a flags argument just to have a possibility to
> extend this in the future, but I'd prefer if we didn't define any now.
> 
> > Note that currently we already have a notion of NM_REAPPLY_FLAG_WITH_METERED
> > and NM_REAPPLY_FLAG_WITH_FIREWALL_ZONE. Your patch should be aware of that
> > and extend on that. That is, it should somehow merge with
> > nm_device_reapply_settings_immediately().
> > 
> > 
> > 
> > 
> > Regardless of whether there is only "apply-all" or with
> > "select-what-to-apply-flags":
> > I don't think that this algorithm of nm_device_reapply_connection() is
> > correct. It might work in the simple case now, but there are
> > interdependencies between settings. I think it is wrong to iterate over
> > individual settings and apply them individually (in fact, the code iterates
> > over the ~diff~ and reapply the diff). How about applying it as a whole?
> > 
> > I'd do:
> > 
> > nm_device_reapply(self, connection, flags, error)
> > {
> >     GError *local = NULL;
> > 
> >     /* check can-apply... */
> >     if (NM_FLAGS_HAS (flags, NM_REAPPLY_FLAG_WITH_IP4) {
> >          if (!can_reapply_ip4(self, connection, local)) {
> >               if (local) {
> >                    g_propagate_error(error, local);
> >                    return FALSE;
> >               }
> >               flags &= ~NM_REAPPLY_FLAG_WITH_IP4;
> >          }
> >     }
> >     ... check other flags...
> > 
> >     /* [1] */
> > 
> >    
> >     if (NM_FLAGS_HAS (flags, NM_REAPPLY_FLAG_WITH_IP4) {
> >        reapply_ip4 (self, connection);
> >     }
> >     ... apply other flags...
> > 
> >     return TRUE;
> > }
> 
> What interdependencies are there?

A NMConnection is one atomic thing, individual parts might not make sense or reveal the whole meaning. Maybe it works for now (as you only reapply certain IPv4 and IPv6 settings), but it is wrong in principle.

> 
> The above code doesn't look too good to me -- it burdens the client with
> specifying the flag.

you already rejected those flags. Anyway, as I said, if the user specifies "no" flags, it means "all" flags. Which you could easily implement by doing
 
  if (!NM_FLAGS_ANY (flags, NM_REAPPLY_FLAG_ALL))
       flags |= NM_REAPPLY_FLAG_ALL;

The point was not about the flags but about interdependencies and passing the entire connection to Reapply, and then have a list of blocks where each part gets applied.

> > [1] At this point, I'd check for a special flag:
> > 
> >   NM_REAPPLY_FLAG_APPLY_ALL
> > 
> > If this flag is specified, then the connection can only be applied if the
> > resulting applied-connection will be equal to the passed connection
> > (basically, what your patch does!).
> > Well, actually: have NM_REAPPLY_FLAG_APPLY_ALL be the default, and add a flag
> > NM_REAPPLY_FLAG_APPLY_PARTIAL with inverse meaning.
> > 
> > 
> > 
> > 
> > 
> > how about the D-Bus method Reapply having 3 arguments:
> > 
> >   @settings-path (a D-Bus path)
> >   @connection (a connection hash)
> >   @flags (what to apply, from above).
> > 
> > 
> > - Specifying both @connection *and* @settings-path should be rejected with 
> >   error.
> > - If @connection is given, use that connection hash to reapply.
> > - If @settings-path is specified, use the connection under that path.
> > - If both @settings-path and @connection is missing, it equals to 
> >   @settings-path being the currently activated connection 
> >   (nm_connection_get_path(nm_active_connection_get_settings_connection()))
> > 
> > 
> > Passing ("/", NULL, 0) would be identical to what you implemented.
> 
> At this point I believe the use case of this is that client changes the
> connection and needs a way to try applying the changes without reconnection
> if possible.

that restricts the API to one usecase. If you later want to implement a second usecase, you need to extend the API.



> Think adding an IP address in the control center or disabling dhcp.
> 
> I don't think it should keep track of the changes by the means of specifying
> the flags and I don't think we should provide an api which would involve any
> other settings connection. Furthermore the changes should be persisted so
> that the next invocation of the control center can present them on the ui
> which conflicts with the inline hash.

yes, that would be a supported usecase. By adding a connection-hash argument, other use cases are supported as well:

  - only reapply certain parts (reimplementing the flags).
  - only reapplying stuff temporary, without persisting it to the NMSettingsConnection.

Pushed a new version to the branch.

The API now optionally takes the connection hash and flags (none defined now; for future extension).

API looks good to me.

> device: add O.FD.NM.Device.Reapply() call
+       } else if (!nm_device_reapply_connection (self,
+                                                 info->connection,
+                                                 info->flags,
+                                                 FALSE,
+                                                 &local)) {
+               /* The dry-run check failed, pityfully. */
+               g_dbus_method_invocation_return_gerror (context, local);
+               g_error_free (local);
+       } else {
+               nm_device_reapply_connection (self,
+                                             info->connection,
+                                             info->flags,
+                                             FALSE,
+                                             &local);

The second time @reconfigure should be set to TRUE, I think.

> libnm: add nm_device_reapply()

		if (NM_DEVICE_GET_CLASS (self)->reapply) {
-                       if (!NM_DEVICE_GET_CLASS (self)->reapply (self, setting, reconfigure, error))
+                       if (!NM_DEVICE_GET_CLASS (self)->reapply (self, old, setting, reconfigure, error))
				return FALSE;

	gboolean            (* reapply) (NMDevice *self,
+                                        NMConnection *old,
                                         const char *setting,
                                         gboolean reconfigure,
                                         GError **error);

Don't these belong to previous commit?

Also, some points from comment 2 still apply.

>> device: add O.FD.NM.Device.Reapply() call

- info = g_malloc0 (sizeof (*info));
+ info = g_slice_new0 (ReapplyInfo);

?





this while() loop in nm_device_reapply_connection() and it's interaction with reapply() is odd.


Why have:

  if (is_of_type_a())
      handle_type_a();
  else if (is_of_type_b())
      handle_type_b();
  else if (can_handle_by_delegating())
      delegate_to_subclass()

It would be more idiomatic to have NMDevice implement reapply, and classes that overwrite it, must properly call the parent implementation.




Anyway, as said in comment 1, I think it is wrong to divide the changes based on nm_connection_diff() and try applying them one-by-one. A connection is a connection is a connection. When reapplying a connection, it must look at the whole connection *and* the whole diff.

Currently reapply() in the derived classes gets
  reapply(self, old, setting, reconfigure, error)
What is the meaning of the @setting argument? Say, the derived class gets here NM_SETTING_WIRED_SETTING_NAME, how would you sanely implement NMDeviceVeth:reapply() which derives from NMDeviceEthernet? 

Pass on the whole connection (as you do now) and the whole @diffs argument and let the derived class figure out what to do.




Obviously, you have the problem, that somebody has to reject un-handled properties in the diff (which the code currently doesn't do correctly [1]).
Which only shows that it's pretty hard to meaningfully delegate reapply() to derived classes. So, just don't do that. Get rid of the virtual reapply() function and implement it in nm_device_reapply().

Then do:

if (has_diff (diff, NM_SETTING_WIRED_SETTING_NAME, "mtu")) {
     if (NM_IS_DEVICE_ETHERNET (self)) {
           /* handle me. I am an ethernet, I want to change mtu */
     } else
         g_set_error(erorr, "cannot handle ethernet.mtu change");
}


The perceived generality by delegating to subclasses only makes it more complicated and is broken in a more complex situation. Just don't do it and have the knowledge of what to reapply in one place (or give places to derived classes to hook the whole process).
Look at what nm_connection_normalize() does. Normalize also doesn't delegate, for the very same reasons.





[1] e.g. NMDeviceEthernet:reapply() only checks whether @settings contains "802-3-ethernet". But it doesn't look at the changed properties:

   { "802-8-ethernet" : { "mtu" } }
   { "802-8-ethernet" : { "mac-address" } }





Also, still need to consolidate nm_device_reapply_connection() with nm_device_reapply_settings_immediately() because they do essentially the same thing.

> device: add O.FD.NM.Device.Reapply() call

Can you clarify what happens to the device's applied connection, if anything?  If you pass a copy of the device's applied connection but change something, does that chnage also get applied to the applied connection, but not the settings connection?  Or how does that work?  I think the docs should clarify what actually gets changed and how persistent it is.

Also "reapply" is a bit confusing to me when you can effectively replace the entire connection, but I'm not sure what term is better.

You might need to use nm_utils_get_shared_wifi_permission() during authorization somewhere too.  It looks like hte current code will allow the applied connection to be replaced by something that the normal Activation-based paths wouldn't allow.

I also think I agree with thaller about the handling of reapply, might as well just pass the whole connection to the device subclass and it is responsible for chaining up to the parent to apply things the parent would do (like MTU for ethernet).  But of course NMDevice handles generic L3/IP stuff, and overriding reapply() in subclasses would handle L2.  But some devices may also need to override L3 handling as well.  So maybe we should split reapply() into two parts, reapply_l2() and reapply_ip()?  nm_device_reapply_connection() would then call the L2 part first and the IP part second, and most devices wouldn't need ot care about L3/IP reapplication at all.


> libnm: add nm_device_reapply()

Got some whitespace issues in nm_device_reapply_finish().

Removed the MTU handling altogether, getting of calling into the device classes.

Tried to find some common ground with immediate reapply (upon connection update). Sharing some code now and allowing reapplication of the zone and metered property even when passed inline.

Adjusted the code comments & fixed some bugs.

Pushed an updated branch for review.

>> cli: add nmcli d reapply

TODO: bash-completion, man.






+static gboolean
+nm_device_reapply_ip6_config

usually our static functions have no nm_type_ prefix. nmtui being the exception, which I personally dislike because it's nice to know that stuff from public headers has a nm_type prefix (but static functions not).





let's drop the @reconfigure argument from nm_device_reapply_connection(). Callers don't want to call it with reconfigure=FALSE, they 




Pushed fixups.

(In reply to Thomas Haller from comment #10)
> >> cli: add nmcli d reapply
> 
> TODO: bash-completion, man.

Done.

> +static gboolean
> +nm_device_reapply_ip6_config
> 
> usually our static functions have no nm_type_ prefix. nmtui being the
> exception, which I personally dislike because it's nice to know that stuff
> from public headers has a nm_type prefix (but static functions not).

Afreed, fixed.

> let's drop the @reconfigure argument from nm_device_reapply_connection().
> Callers don't want to call it with reconfigure=FALSE, they 

They do, with initial dry run on nmcli d reapply.

> Pushed fixups.

Thank you. Applied all.

Pushed new version for review.

Branch merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4059b253b764657d88f6b192ed4cc3fd700eb608