Created attachment 279001 [details] [review]
[PATCH] delete nm-generated connection

Attached patch applies on f5681d15ba6b21aa6c13bc9a0e6a0af234a23e2f


The first is my approach to this [1] and the seconds is dcbw's approach [2].


Notably differences:

[1] adds a new function nm_settings_connection_delete_nm_generated(). I think this is cleaner, and it also sets the '&connection' variable to NULL (thus avoiding dangling pointers).

Also, [1] deletes a connection whenever the active (nm-generated) connection becomes deactivated. [2] only when the device becomes removed. I think the approach [1] makes more sense, because I often start NM, which generates a new connection, and I immediately start another connection. In this case the generated connection keeps hanging around but I don't have any use for it.



Please review.

(In reply to comment #1)
> [1] adds a new function nm_settings_connection_delete_nm_generated(). I think
> this is cleaner

Heh. I have exactly the opposite feeling. I feel like it's an unnecessary (and somewhat unusual) API, and the "if (nm_generated) delete()" way is better, because it's completely clear what it's doing (whereas with the special method, who knows what other steps it might be taking).

> Also, [1] deletes a connection whenever the active (nm-generated) connection
> becomes deactivated. [2] only when the device becomes removed.

[1] definitely seems better to me. The only (small) advantage [2] has is that if a device goes down and then comes back up with the same connection details, then with [2] it (hopefully) keeps the same D-Bus path and UUID, whereas with [1] the original connection goes away and a new one gets generated.

If we care about that (and I'm really not sure we do), it might be possible to make it so the connection just gets unexported rather than deleted in this case, and is re-exported if it gets used again later. Although, it would still get a different UUID if you restarted NM, so...


[Also, meta-comments: (1) it would have been easier to review as two separate attachments, (2) I don't find the extremely large number of diff context lines to be helpful; it means much more scrolling back and forth through the patch, and the extra context generally isn't needed anyway.]

(In reply to comment #2)
> (In reply to comment #1)
> > [1] adds a new function nm_settings_connection_delete_nm_generated(). I think
> > this is cleaner
> 
> Heh. I have exactly the opposite feeling. I feel like it's an unnecessary (and
> somewhat unusual) API, and the "if (nm_generated) delete()" way is better,
> because it's completely clear what it's doing (whereas with the special method,
> who knows what other steps it might be taking).

Ok. Then let's take approach [2] (in this regard).


> > Also, [1] deletes a connection whenever the active (nm-generated) connection
> > becomes deactivated. [2] only when the device becomes removed.
> 
> [1] definitely seems better to me. The only (small) advantage [2] has is that
> if a device goes down and then comes back up with the same connection details,
> then with [2] it (hopefully) keeps the same D-Bus path and UUID, whereas with
> [1] the original connection goes away and a new one gets generated.
> 
> If we care about that (and I'm really not sure we do), it might be possible to
> make it so the connection just gets unexported rather than deleted in this
> case, and is re-exported if it gets used again later. Although, it would still
> get a different UUID if you restarted NM, so...

I also prefer approach [1] here, but I would not care about the mentioned issue.

It adds quite some complexity and it leaves an "un-exported" connection hanging. How long do we keep it? Indefinitely? Better, until a future assumption on the same device does not match the un-exported connection. Sounds already complicated.



> [Also, meta-comments: (1) it would have been easier to review as two separate
> attachments, (2) I don't find the extremely large number of diff context lines
> to be helpful; it means much more scrolling back and forth through the patch,
> and the extra context generally isn't needed anyway.]

ACK. Message received!! :)

So I agree with the general approach of [1], also agree with danw's comments regarding "if (nm_generated) delete"...

Also, in the spirit of correct refcounting, lets move:

	G_OBJECT_CLASS (nm_act_request_parent_class)->dispose (object);

	if (connection)
		nm_settings_connection_delete_nm_generated ((NMSettingsConnection **) &connection);

the delete above the parent->dispose() call, because it's the parent that owns a reference to the 'connection', and thus we may not own a reference to 'connection' after the parent dispose.

Created attachment 279335 [details] [review]
new version of patch

(03:08:53 PM) thaller: dcbw, now it looks like: http://ur1.ca/hm0um ... but this has the subtle effect, that a connection might be deleted, while deactivating it... could lead to subtle bugs... e.g. http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/devices/nm-device.c#n6752
15:10
(03:10:53 PM) thaller: dcbw, in this case, it's no problem, because we take an additional ref to the act_req while doing the state transition... but it's subtle... isn't it? Should we delete the connection in a g_idle_add()?

Yes, this is subtle.  So how about removing them in nm-manager.c::active_connection_remove()?  That has a few advantages:

1) the Manager is what creates the connection anyway, so you could see this as more "balanced"

2) if the connection was activated, then active_connection_remove() gets called from an idle handler by the manager already anyway

In all cases (and we want this to always be true) the return value of _new_active_connection() is passed to active_connection_add().  Once we do that, the AC must also be removed with active_connection_remove().  So I think using active_connection_remove() should work here.  Something liek this maybe?

		g_signal_handlers_disconnect_by_func (active, active_connection_state_changed, self);
		g_signal_handlers_disconnect_by_func (active, active_connection_default_changed, self);

		connection = nm_active_connection_get_connection (active);
		if (connection)
			g_object_ref (connection);

		g_object_unref (active);

		if (connection) {
			/* If the connection was generated by NM originally, remove it */
			if (nm_settings_connection_get_nm_generated (NM_SETTINGS_CONNECTION (connection)))
				nm_settings_connection_delete (NM_SETTINGS_CONNECTION (connection), NULL, NULL);
			g_object_unref (connection);
		}
	}


=====

 gboolean
 nm_settings_connection_get_nm_generated (NMSettingsConnection *connection)
 {
+	g_return_val_if_fail (connection && NM_IS_SETTINGS_CONNECTION (connection), FALSE);
+
 	return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->nm_generated;
 }

I don't think we need the 'connection &&' here since NM_IS_XXX already checks for NULL.

Created attachment 279367 [details] [review]
[patch] according comment #6

(In reply to comment #6)

>  gboolean
>  nm_settings_connection_get_nm_generated (NMSettingsConnection *connection)
>  {
> +    g_return_val_if_fail (connection && NM_IS_SETTINGS_CONNECTION
> (connection), FALSE);
> +
>      return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->nm_generated;
>  }
> 
> I don't think we need the 'connection &&' here since NM_IS_XXX already checks
> for NULL.

Correct, I changed it. I confused this with NM_XXX() which accepts NULL (but asserts against unexpected types).



> (03:08:53 PM) thaller: dcbw, now it looks like: http://ur1.ca/hm0um ... but
> this has the subtle effect, that a connection might be deleted, while
> deactivating it... could lead to subtle bugs... e.g.
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/devices/nm-device.c#n6752
> 15:10
> (03:10:53 PM) thaller: dcbw, in this case, it's no problem, because we take an
> additional ref to the act_req while doing the state transition... but it's
> subtle... isn't it? Should we delete the connection in a g_idle_add()?
> 
> Yes, this is subtle.  So how about removing them in
> nm-manager.c::active_connection_remove()?  That has a few advantages:
> 
> 1) the Manager is what creates the connection anyway, so you could see this as
> more "balanced"
> 
> 2) if the connection was activated, then active_connection_remove() gets called
> from an idle handler by the manager already anyway
> 
> In all cases (and we want this to always be true) the return value of
> _new_active_connection() is passed to active_connection_add().  Once we do
> that, the AC must also be removed with active_connection_remove().  So I think
> using active_connection_remove() should work here.  Something liek this maybe?
> 

I did your suggestion in comment 7, attachment 279367 [details] [review].

But I think this is even worse.

The "subtlety" I dislike is:

  connection = nm_device_get_connection (device);

  /* deactivate device (and free act_request, and possibly connection) */

  /* dangling pointer connection */


Deleting the connection in NMActRequest:dispose() keeps the connection alive at least until the act-request gets disposed. Now active_connection_remove() might delete the connection even earlier, before the lifetime of act-request ends.

Created attachment 279379 [details] [review]
again delete connection in NMActivationRequest:dispose()

I was thinking, but there is no simple solution to this.

I feel, that the connection should not be deleted before the NMActiveConnection.
It might be nice, to only delete if, if really nobody has any interest in the connection, but I think that is hard to verify (with out adding lots of code/complexity, so that in the end it does not simplify things).

So, when having a NMConnection, be aware that it might get deleted at (unexpected) moments. Just do expect those moments :)

I prefer this variant to attachment 279367 [details] [review]

(In reply to comment #9)
> So, when having a NMConnection, be aware that it might get deleted at
> (unexpected) moments. Just do expect those moments :)

It seems like if you're going to have to expect that, then there's no reason to dislike the original patch (deleting from NMManager:active_connection_remove rather than from NMActRequest:dispose)?

But I'm not opposed to the NMActRequest version either. (Though maybe put it in NMActiveConnection instead? The idea of cleaning up temporary connections is not specific to device-based connections [even if it currently only applies to that case].)

One thought on both versions: Both calls to assume_connection() are now followed by "if assumption-failed then delete-generated-connection", so maybe that code should just go into assume_connection() itself.

Hm... this should render the temporary-PAN-connection-deleting code in nm-bluez-device.c unnecessary, shouldn't it?

(In reply to comment #10)
> (In reply to comment #9)
> > So, when having a NMConnection, be aware that it might get deleted at
> > (unexpected) moments. Just do expect those moments :)
> 
> It seems like if you're going to have to expect that, then there's no reason to
> dislike the original patch (deleting from NMManager:active_connection_remove
> rather than from NMActRequest:dispose)?
> 
> But I'm not opposed to the NMActRequest version either. (Though maybe put it in
> NMActiveConnection instead? The idea of cleaning up temporary connections is
> not specific to device-based connections [even if it currently only applies to
> that case].)

My previous preference was that I did not want to remove the connection as long as a referencing NMActRequest exists.
Now I tend to "deleting from NMManager:active_connection_remove", because at that point, the NMActRequest itself is already removed an no longer active. Hence, it seems to be the right moment also the delete the connection.


> One thought on both versions: Both calls to assume_connection() are now
> followed by "if assumption-failed then delete-generated-connection", so maybe
> that code should just go into assume_connection() itself.

I like that. But the caller to assume_connection() does not own a reference to the connection. Hence it could end up with a dangling pointer. How about passing &connection, and clearing the pointer when the connection gets deleted?



(In reply to comment #11)
> Hm... this should render the temporary-PAN-connection-deleting code in
> nm-bluez-device.c unnecessary, shouldn't it?

Not really, the PAN connection exists as long as the device exists, not as long as the connection is active. Nonetheless, while looking at that, I think we should improve on that.

Please see branch th/bgo731937_delete_nm_generated_connection (whose first commit is based on attachment 279367 [details] [review].

>+	nm_log_dbg (LOGD_DEVICE, "(%s): connection assumption failed. Delete generated connection",

"Deleting"

> libnm-util/core: implement 'unsaved' and 'nm-generated' as #NMConnectionOptionFlags

>    E.g. nmcli already supports creating unsaved connections, but
>    `nmcli connection modify` will always persistate the connection.
>    With the now exposed 'unsaved' property, it could default to
>    change the connection in memory only.

NMRemoteConnection already has an "unsaved" property which nmcli could use.

>-	PROP_UNSAVED,

oh, yeah, you can't do that. It's part of the org.freedesktop.NetworkManager.Settings.Connection API.

And "nm-generated" is even more specific to server-side-connections than "unsaved" is. (It wouldn't make any sense to set that flag on a newly-created client-side NMConnection and then try to AddConnection it...) So I think it would make sense as an NMSettingsConnection / NMRemoteConnection property, but not as a generic NMConnection option.

(And so we don't need "libnm-util: add NMConnection option flags".)



> libnm-util/core: add new NM_CONNECTION_OPTION_FLAGS_NM_ASSUMED flag
>
> We want to descriminate between nm-generated connections and connections

"discriminate"

Repushed.

For now I kept the NMConnectionFlags (I renamed them from previously NMConnectionOptionFlags).

The probably more correct solution is to add NMRemoteConnectionFlags, NMSettingsConnectionFlags, etc. whenever we need them (note that we might soon get an NMAppliedConnection as another subclass of NMConnection).

But I do see some overlap. For example, currently the "unsaved" flag is both relevant to NMRemoteConnection and NMSettingsConnection. We could later also expose "nm-generated" and "nm-assumed" flags to client. And we might have additional flags in the future.

Given that, I think it is actually clearer to put the flags in the NMConnection base class.


What do you think?

The more I think about it, the more I think they should actually be NMSettingsConnection flags.  None of the flags are actually specific to NMConnection, and some are actively irrelevant (generated, assumed) since they only apply to SettingsConnection.  Moreso, 'assumed' is a runtime property, not a configuration property, so it should really be a property of NMActiveConnection (though 'generated' is still a property of NMSettingsConnection).  Also, I don't think it's a ton more code to mirror those on the Remote side either.  I'd rather keep the base NMConnection class more "pure" and not add runtime-type stuff that NMSettingsConnection/NMActiveConnection are meant for, at least not now.

That said, all the core logic is good.  A few suggestions I have for that would be:

> core: clean up nm-generated connection if assumption fails or active connection disconnects

I'm still not very happy about the **inout_connection parameter to assume_connection().  I don't think it's very clear, and I'd really like the connection to be destroyed from the function that actually owns the reference (eg, that calls get_existing_connection()).  I know why you did that, and I have a solution that I think should make us both happy:

- we can just call recheck_assume_connection() from add_device() and get down to one caller of assume_connection().  That's excellent by itself, but it also means that we can dispose/delete the generated connection directly from recheck_assume_connection() too, and remove the **inout_connection argument.

- that also means we don't have to check for the ASSUMED flag at all, since the only time a connection would ever be assumed, it's also GENERATED.  And since recheck_assume_connection() won't ever get to this point for connections that are GENERATED but not assumed (eg, bluetooth) that's also fine.

An untested RFC patch is:

http://bigw.org/~dan/assume-cleanup.patch

> libnm-util/core: implement 'unsaved' and 'nm-generated' as #NMConnectionFlags
> libnm-util/core: add new %NM_CONNECTION_FLAGS_NM_ASSUMED flag

NMActiveConnection already has an assumed property so nm_device_uses_assumed_connection() should simply be:

return priv->act_request ? nm_active_connection_get_assumed (priv->act_request) : FALSE;

and then we don't need the ASSUMED flag at all.  If we want to give clients access to that, we should do it through NMActiveConnection instead.  I don't think it's a good fit on NMConnection itself (or NMSettingsConnection either).

I rework it, see th/bgo731937_delete_nm_generated_connection
(previous version is th/bgo731937_delete_nm_generated_connection-1 -- including the assume-cleanup.patch)


few notes, where I am unsure whether it is correct:

- add_device(), it calls now "system_create_virtual_devices()" after "assume_connection()"
- shouldn't we disconnect recheck_assume_connection() once we successfully activate/assume a connection -- so that we only assume at startup.

(In reply to comment #16)
> I rework it, see th/bgo731937_delete_nm_generated_connection
> (previous version is th/bgo731937_delete_nm_generated_connection-1 -- including
> the assume-cleanup.patch)

I like th/bgo731937_delete_nm_generated_connection; I skimmed the changes and I should do some more testing with it, but I have no architectural issues with this branch.

> few notes, where I am unsure whether it is correct:
> 
> - add_device(), it calls now "system_create_virtual_devices()" after
> "assume_connection()"

I looked into that with the original patch and I think this is OK but I'm not 100% sure.  We should investigate more.  The only case I can think of here is a virtual interface which specifies this original device as the parent.  But the child cant' do anything immediately because if we just created it, it clearly has no existing connection to assume, and thus when the child's add_device() gets called it won't assume a connection at all and shouldn't affect the original device.  Maybe you can think of a case where it would matter? 

> - shouldn't we disconnect recheck_assume_connection() once we successfully
> activate/assume a connection -- so that we only assume at startup.

Actually I think this is no longer true; I seem to recall this code was added by danw as part of the fix to ensure that NM recognizes master/slave changes made by outside tools.  I could be wrong; maybe check with him?

(In reply to comment #17)
> > - shouldn't we disconnect recheck_assume_connection() once we successfully
> > activate/assume a connection -- so that we only assume at startup.
> 
> Actually I think this is no longer true; I seem to recall this code was added
> by danw as part of the fix to ensure that NM recognizes master/slave changes
> made by outside tools.  I could be wrong; maybe check with him?

I think it was added as part of that, but it's more general than just master/slave. Basically, there was a race condition when someone else created a new device; if NM noticed the device before the creator added IP addresses to it, then it couldn't generate+assume a connection, but if it noticed it after the IPs were added then it could. So recheck-assume-connection exists to fix that; if the device is un-assumable the first time NM sees it, it can try again later any time something changes that might make the device assumable.

Rebased branch on master, fixed a minor bug and added commit:

  "core: recheck_assume_connection() do nothing if the device is not disconnected"

needs more testing though.

The new commit looks fine to me.

Rebased on master.


On testing, the following happens:

- start openvpn (externally). NM generates a new connection tun0.
- `nmcli connection down tun0` causes the deletion of the generated connection. Immediately, NM assumes another connection (again 'tun0').
- `nmcli connection down tun0` now deletes the generated connection AND deconfigures and removes the tun interface.

Todo: needs investigation

(In reply to comment #21)
> Rebased on master.
> 
> 
> On testing, the following happens:
> 
> - start openvpn (externally). NM generates a new connection tun0.
> - `nmcli connection down tun0` causes the deletion of the generated connection.
> Immediately, NM assumes another connection (again 'tun0').
> - `nmcli connection down tun0` now deletes the generated connection AND
> deconfigures and removes the tun interface.


That happened because I tested with an external openvpn connection that configures a peer address:

26: tun7: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN group default qlen 100
    link/none 
    inet 10.172.113.6 peer 10.172.113.5/32 scope global tun7
       valid_lft forever preferred_lft forever

nm_platform_ip4_address_delete() was unable to delete this connection, hence during disconnect the address was not flushed, causing a re-assumption.

I fixed this by adding commit
  "platform: add @peer_address argument to nm_platform_ip4_address_delete()"



Repushed branch, the following commits are new:

94eaeca platform: add @peer_address argument to nm_platform_ip4_address_delete()
6fcac3a platform: log ifindex and ifname when deleting addresses or routes
70073f6 platform/trivial: move code in nm-platform.c
0c20dda core: log when emitting RECHECK_ASSUME signal

> platform: add @peer_address argument to nm_platform_ip4_address_delete()

But the peer address is not required for IPv6?



> settings: implement 'unsaved' and 'nm-generated' as #NMSettingsConnectionFlags

I'd keep nm_settings_connection_get_unsaved(), nm_settings_connection_get_nm_generated(), and nm_settings_connection_set_nm_generated() around, but just change them to work in terms of flags internally. Because this doesn't really seem to be an improvement:

>-		if (   nm_settings_connection_get_unsaved (NM_SETTINGS_CONNECTION (priv->pan_connection))
>+		if (   (nm_settings_connection_get_flags (NM_SETTINGS_CONNECTION (priv->pan_connection)) & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED)



> core: check for assumed connection by looking at NMActiveConnection:assumed property

This is incorrect. Each of the places that calls nm_device_uses_generated_connection() cares specifically about the case where we the device has a temporary connection that was created to match its current state, and should NOT be run in the case where we determined that the device had previously had a known NMSettingsConnection activated on it.

Eg, nm_device_master_release_slaves() wants to release the slaves if this is an "NM-owned" device, but not if it's a device that we're merely observing (eg, virbr0). But changing it from uses_generated_connection() to uses_assumed_connection() means that if you bring up a bridge in NM, restart NM, and then take the bridge down, it would now skip releasing the slaves, since the device has an assumed connection.



> core: clean up nm-generated, assumed connection when assumption fails or connection disconnects

>+		nm_log_dbg (LOGD_DEVICE, "(%s): connection assumption failed. Deleting generated connection",
>+		            nm_device_get_iface (device));

>+		if (generated)
>+			nm_settings_connection_delete (NM_SETTINGS_CONNECTION (connection), NULL, NULL);

shouldn't log that you're deleting a generated connection if you aren't

>+	if (try_assume) {
>+		try_assume = recheck_assume_connection (device, self);

The return value of recheck_assume_connection() is whether we *did* assume a connection, not whether we should try to. So a separate "assumed_connection" variable would make things a lot clearer.

Why did you move the assumption from the end to the beginning?



> core: don't merge an assumed connection back into the device IP config

I think you only need this because of the earlier uses_generated to uses_assumed change.

> settings: add NMSettingsConnection:flags

+	new_flags = NM_SETTINGS_CONNECTION_GET_PRIVATE (self)->flags;
+	if (set)
+		new_flags = new_flags | flags;
+	else
+		new_flags = new_flags & ~flags;

Maybe use |= flags and &= ~flags?


> settings: implement 'unsaved' and 'nm-generated' as #NMSettingsConnectionFlags

if (!(flags & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED) != !now_unsaved) {

I think I know why it's written this way, but its starting to get confusing, as in I have to think about it for 5 seconds instead of immediately knowing what it does.  Then below there's:

if (!(old_flags & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED) != !(flags & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED))

what if instead we have:

FBOOL(flags, f) ((flags & NM_SETTINGS_CONNECTION_FLAGS_##f) ? TRUE : FALSE)

and then it becomes

if (FBOOL(flags, UNSAVED) != now_unsaved)
if (FBOOL(old_flags, UNSAVED) != FBOOL(flags, UNSAVED))
g_value_set_boolean (value, FBOOL(nm_settings_connection_get_flags (self), UNSAVED));

or something like that?

> core: don't merge an assumed connection back into the device IP config

Yeah, what's the issue here?  Even merging it back in shouldn't change the actual configuration.  What's the difference between the existing config and the final composite config that's screwing things up?

(In reply to comment #23)
> > platform: add @peer_address argument to nm_platform_ip4_address_delete()
> 
> But the peer address is not required for IPv6?

seems libnl does not support that. I guess, it just doesn't make sense(?).

http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nm-linux-platform.c?id=26eafd3420be7fa189ee0964b120a609e1d0affb#n3417
https://github.com/thom311/libnl/blob/16e3d61689161c77feffe605259f9c0282234be9/lib/route/addr.c#L960



> > settings: implement 'unsaved' and 'nm-generated' as #NMSettingsConnectionFlags
> 
> I'd keep nm_settings_connection_get_unsaved(),
> nm_settings_connection_get_nm_generated(), and
> nm_settings_connection_set_nm_generated() around, but just change them to work
> in terms of flags internally. Because this doesn't really seem to be an
> improvement:
> 
> >-		if (   nm_settings_connection_get_unsaved (NM_SETTINGS_CONNECTION (priv->pan_connection))
> >+		if (   (nm_settings_connection_get_flags (NM_SETTINGS_CONNECTION (priv->pan_connection)) & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED)

added fixup.


> > core: check for assumed connection by looking at NMActiveConnection:assumed property
> 
> This is incorrect. Each of the places that calls
> nm_device_uses_generated_connection() cares specifically about the case where
> we the device has a temporary connection that was created to match its current
> state, and should NOT be run in the case where we determined that the device
> had previously had a known NMSettingsConnection activated on it.
> 
> Eg, nm_device_master_release_slaves() wants to release the slaves if this is an
> "NM-owned" device, but not if it's a device that we're merely observing (eg,
> virbr0). But changing it from uses_generated_connection() to
> uses_assumed_connection() means that if you bring up a bridge in NM, restart
> NM, and then take the bridge down, it would now skip releasing the slaves,
> since the device has an assumed connection.

hm. I think that the term "nm-generated" has a wider use. For example it is useful for the bluez PAN connection, which is nm-generated and unsaved, but behaves the same as regular connections.

That's why I changed the meaning of "nm-generated", because I think it makes sense and is useful (e.g bluez).

In an earlier version I added a connection-flag "nm-assumed". How about re-adding the nm-assumed flag (and make it behave as nm-generated previously).


> > core: clean up nm-generated, assumed connection when assumption fails or connection disconnects
> 
> >+		nm_log_dbg (LOGD_DEVICE, "(%s): connection assumption failed. Deleting generated connection",
> >+		            nm_device_get_iface (device));
> 
> >+		if (generated)
> >+			nm_settings_connection_delete (NM_SETTINGS_CONNECTION (connection), NULL, NULL);
> 
> shouldn't log that you're deleting a generated connection if you aren't

done.


> >+	if (try_assume) {
> >+		try_assume = recheck_assume_connection (device, self);
> 
> The return value of recheck_assume_connection() is whether we *did* assume a
> connection, not whether we should try to. So a separate "assumed_connection"
> variable would make things a lot clearer.
> 
> Why did you move the assumption from the end to the beginning?

I dunno. dcbw did that in his patch, I asked him the same, see comment 17.
But it looks correct to me.



> > core: don't merge an assumed connection back into the device IP config
> 
> I think you only need this because of the earlier uses_generated to
> uses_assumed change.

dropped.





(In reply to comment #24)
> > settings: add NMSettingsConnection:flags
> 
> +    new_flags = NM_SETTINGS_CONNECTION_GET_PRIVATE (self)->flags;
> +    if (set)
> +        new_flags = new_flags | flags;
> +    else
> +        new_flags = new_flags & ~flags;
> 
> Maybe use |= flags and &= ~flags?

done


> > settings: implement 'unsaved' and 'nm-generated' as #NMSettingsConnectionFlags
> 
> if (!(flags & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED) != !now_unsaved) {
> 
> I think I know why it's written this way, but its starting to get confusing, as
> in I have to think about it for 5 seconds instead of immediately knowing what
> it does.  Then below there's:
> 
> if (!(old_flags & NM_SETTINGS_CONNECTION_FLAGS_UNSAVED) != !(flags &
> NM_SETTINGS_CONNECTION_FLAGS_UNSAVED))
> 
> what if instead we have:
> 
> FBOOL(flags, f) ((flags & NM_SETTINGS_CONNECTION_FLAGS_##f) ? TRUE : FALSE)
> 
> and then it becomes
> 
> if (FBOOL(flags, UNSAVED) != now_unsaved)
> if (FBOOL(old_flags, UNSAVED) != FBOOL(flags, UNSAVED))
> g_value_set_boolean (value, FBOOL(nm_settings_connection_get_flags (self),
> UNSAVED));
> 
> or something like that?

Added commit "core: add macros NM_FLAGS_HAS(), NM_FLAGS_ANY() and NM_FLAGS_ALL()"

which makes now:

[1] if (NM_FLAGS_HAS (flags, NM_SETTINGS_CONNECTION_FLAGS_UNSAVED) != !!now_unsaved) {
[2] if (NM_FLAGS_HAS (old_flags, NM_SETTINGS_CONNECTION_FLAGS_UNSAVED) != NM_FLAGS_HAS (flags, NM_SETTINGS_CONNECTION_FLAGS_UNSAVED))


The !!now_unsaved is still needed in [1].
For [2], the "!" was unneeded anyway in the original version.


> > core: don't merge an assumed connection back into the device IP config
> 
> Yeah, what's the issue here?  Even merging it back in shouldn't change the
> actual configuration.  What's the difference between the existing config and
> the final composite config that's screwing things up?

dropped.



Repushed.

(In reply to comment #25)
> (In reply to comment #23)
> > > core: check for assumed connection by looking at NMActiveConnection:assumed property
> > 
> > This is incorrect. Each of the places that calls
> > nm_device_uses_generated_connection() cares specifically about the case where
> > we the device has a temporary connection that was created to match its current
> > state, and should NOT be run in the case where we determined that the device
> > had previously had a known NMSettingsConnection activated on it.
> > 
> > Eg, nm_device_master_release_slaves() wants to release the slaves if this is an
> > "NM-owned" device, but not if it's a device that we're merely observing (eg,
> > virbr0). But changing it from uses_generated_connection() to
> > uses_assumed_connection() means that if you bring up a bridge in NM, restart
> > NM, and then take the bridge down, it would now skip releasing the slaves,
> > since the device has an assumed connection.

didn't we say, that if the user actively brings down a connection -- even an assumed one, that we de-configure it?

"don't touch" is only relevant on shutdown. Do we not achieve that with the @quitting argument of NMManager:remove_device()?


> hm. I think that the term "nm-generated" has a wider use. For example it is
> useful for the bluez PAN connection, which is nm-generated and unsaved, but
> behaves the same as regular connections.
> 
> That's why I changed the meaning of "nm-generated", because I think it makes
> sense and is useful (e.g bluez).
> 
> In an earlier version I added a connection-flag "nm-assumed". How about
> re-adding the nm-assumed flag (and make it behave as nm-generated previously).

(In reply to comment #25)
> hm. I think that the term "nm-generated" has a wider use. For example it is
> useful for the bluez PAN connection, which is nm-generated and unsaved, but
> behaves the same as regular connections.
> 
> That's why I changed the meaning of "nm-generated", because I think it makes
> sense and is useful (e.g bluez).

You can change the meaning of nm-generated if you want, but the places that currently call nm_device_uses_generated_connection() need a function that has exactly the semantics that nm_device_uses_generated_connection() has now, so you still need some flag or whatever corresponding to that.

(In reply to comment #26)
> didn't we say, that if the user actively brings down a connection -- even an
> assumed one, that we de-configure it?

Regardless of whether the code does the right thing with generated connections now, changing it from uses_generated() to uses_assumed() makes it do the wrong thing with assumed-but-not-generated connections.

And all of the other places that uses_generated_connection() is used also mean "generated-and-assumed", not just "assumed". (Eg, nm_device_set_ip[46]_config() should not be modifying the NMSettings of user-created connections.)

(In reply to comment #27)

Added commit:
  "core: add "nm-assumed" flag to NMSettingsConnection"
and repushed. How about now?

Well... now "generated" has a consistent meaning throughout NM, but "assumed" doesn't. (In some places "assumed" refers to all connections that were created by someone else, but in other places "assumed" refers only to connections that were created by someone else and then NOT matched to an existing NMSettingsConnection by NM.)

I think we need both nm-assumed and nm-generated, but independent of each other. So you can have:

  Neither NM_ASSUMED nor NM_GENERATED: the normal case 

  Just NM_GENERATED: eg, the auto-generated Bluetooth connections

  Just NM_ASSUMED: eg, the initrd brings up eth0, and when NM starts up,
    it recognizes that eth0 has the configuration defined by ifcfg-eth0.
    (This is the original definition of "assumed", from before we added
    generated connections.)

  Both NM_ASSUMED and NM_GENERATED: eg, virbr0, which is created and
    configured externally to NM, and does not correspond to any ifcfg
    file, but NM creates a temporary NMSettingsConnection to represent
    its state.

(And it's the last case that "nm_device_uses_generated_connection()" cares about. Maybe that state should be called something like "externally created" or "externally managed". Or "read only"?)

(In reply to comment #29)
> Well... now "generated" has a consistent meaning throughout NM, but "assumed"
> doesn't. (In some places "assumed" refers to all connections that were created
> by someone else, but in other places "assumed" refers only to connections that
> were created by someone else and then NOT matched to an existing
> NMSettingsConnection by NM.)
> 
> I think we need both nm-assumed and nm-generated, but independent of each
> other. So you can have:
> 
>   Neither NM_ASSUMED nor NM_GENERATED: the normal case 
> 
>   Just NM_GENERATED: eg, the auto-generated Bluetooth connections
> 
>   Just NM_ASSUMED: eg, the initrd brings up eth0, and when NM starts up,
>     it recognizes that eth0 has the configuration defined by ifcfg-eth0.
>     (This is the original definition of "assumed", from before we added
>     generated connections.)
> 
>   Both NM_ASSUMED and NM_GENERATED: eg, virbr0, which is created and
>     configured externally to NM, and does not correspond to any ifcfg
>     file, but NM creates a temporary NMSettingsConnection to represent
>     its state.
> 
> (And it's the last case that "nm_device_uses_generated_connection()" cares
> about. Maybe that state should be called something like "externally created" or
> "externally managed". Or "read only"?)

This sounds reasonable to me.

WRT "core: add macros NM_FLAGS_HAS(), NM_FLAGS_ANY() and NM_FLAGS_ALL()", that also looks fine to me.

(In reply to comment #29)
> Well... now "generated" has a consistent meaning throughout NM, but "assumed"
> doesn't. (In some places "assumed" refers to all connections that were created
> by someone else, but in other places "assumed" refers only to connections that
> were created by someone else and then NOT matched to an existing
> NMSettingsConnection by NM.)
> 
> I think we need both nm-assumed and nm-generated, but independent of each
> other. So you can have:
> 
>   Neither NM_ASSUMED nor NM_GENERATED: the normal case 
> 
>   Just NM_GENERATED: eg, the auto-generated Bluetooth connections

(which also implies "not yet modified by the user" and "in-memory-only")


>   Just NM_ASSUMED: eg, the initrd brings up eth0, and when NM starts up,
>     it recognizes that eth0 has the configuration defined by ifcfg-eth0.
>     (This is the original definition of "assumed", from before we added
>     generated connections.)
> 
>   Both NM_ASSUMED and NM_GENERATED: eg, virbr0, which is created and
>     configured externally to NM, and does not correspond to any ifcfg
>     file, but NM creates a temporary NMSettingsConnection to represent
>     its state.
> 
> (And it's the last case that "nm_device_uses_generated_connection()" cares
> about. Maybe that state should be called something like "externally created" or
> "externally managed". Or "read only"?)

I don't think we agree.

Note that on current master, nm_settings_connection_set_nm_generated() is only set for connections that were also just generated. See get_existing_connection().

Then you have nm_active_connection_set_assumed() which is always set when assuming a connection (regardless whether it is nm_settings_connection_set_nm_generated()).

Hence, on master, you achieve the distinction you ask for by either looking at NMSettingsConnection, or at NMActiveConnection (or both).


I would say: NM_SETTINGS_CONNECTION_FLAGS_NM_GENERATED has the meaning as you describe. _NM_ASSUMED *implies* _NM_GENERATED, but it is now used as "nm-generated" is currently used on master. It is a connection especially generated to be assumed. Actually, the branch renames "nm-generated" to nm-assumed", and adds nm-generated with a broader meaning.

To check for your above "NM_ASSUMED" case, we still check nm_active_connection_get_assumed().


Having said that, I think the current state of the branch is fine as is. Including the name NM_SETTINGS_CONNECTION_FLAGS_NM_ASSUMED.

I just fixed a commit message, and added a minor fixup! commit. Repushed

(In reply to comment #31)
> Actually, the branch renames "nm-generated" to
> nm-assumed", and adds nm-generated with a broader meaning.
> 
> To check for your above "NM_ASSUMED" case, we still check
> nm_active_connection_get_assumed().

Right. This is my complaint. nm_settings_connection_get_nm_assumed() and nm_active_connection_get_assumed() use the same word to mean different things. That's confusing, and there's no reason to do it.

(In reply to comment #32)
> (In reply to comment #31)
> > Actually, the branch renames "nm-generated" to
> > nm-assumed", and adds nm-generated with a broader meaning.
> > 
> > To check for your above "NM_ASSUMED" case, we still check
> > nm_active_connection_get_assumed().
> 
> Right. This is my complaint. nm_settings_connection_get_nm_assumed() and
> nm_active_connection_get_assumed() use the same word to mean different things.
> That's confusing, and there's no reason to do it.

We have an NMActiveConnection that assumed a connection -- hence: nm_active_connection_get_assumed(). If the corresponding NMSettingsConnection was generated specifically for the purpose of "assumption", it is nm_settings_connection_get_nm_assumed().

Yes, nm-assumed on an NMSettingsConnection has a different meaning then on an NMActiveConnection, but that would be fine with me.

And then we have nm_device_uses_assumed_connection() which boils down to "nm_settings_connection_get_nm_assumed() && nm_active_connection_get_assumed()".
-- where we would expect, that nm_settings_connection_get_nm_assumed() is only TRUE if nm_active_connection_get_assumed().


You made some suggestions, but can you be more specific? I personally favor as-is over <<"externally created" or "externally managed". Or "read only"?>>

(In reply to comment #33)
> Yes, nm-assumed on an NMSettingsConnection has a different meaning then on an
> NMActiveConnection, but that would be fine with me.

Why are you OK with having "assumed" mean different things in NMSettingsConnection and NMActiveConnection, but not OK with having "generated" mean different things in NMSettingsConnection and nm-device-bluetooth?

> You made some suggestions, but can you be more specific?

I don't want nm_active_connection_get_assumed() and nm_settings_connection_get_nm_assumed() to mean different things. So one of these things needs to happen:

  1. nm_active_connection_get_assumed() gets renamed.

  2. nm_settings_connection_get_nm_assumed() gets renamed

  3. nm_settings_connection_get_nm_assumed()'s meaning changes to match
     nm_active_connection_get_assumed()

(In reply to comment #34)
> (In reply to comment #33)
> > Yes, nm-assumed on an NMSettingsConnection has a different meaning then on an
> > NMActiveConnection, but that would be fine with me.
> 
> Why are you OK with having "assumed" mean different things in
> NMSettingsConnection and NMActiveConnection, but not OK with having "generated"
> mean different things in NMSettingsConnection and nm-device-bluetooth?

NMDeviceBluetooth (or NMBluezDevice) both have no own property with the concept of "nm-generated".
nm-bluez-device.c wants to use a notion/name/concept "nm-generated" for the NMSettingsConnection it generates. -- which conflicts with current master, where nm_settings_connection_get_nm_generated() already means: "nm-generated especially for connection assumption".

OTOH, having the same name/concept ("nm-assumed") on two different entities (NMSettingsConnection vs. NMActiveConnection), makes it clear that they are not the same. But their meaning is related.


nm_active_conneciton_get_assumed() means, that the connection was assumed (configured externally, or whatever you want to call it). Regardless of whether we we assumed an existing (on-disk) connection or generated one (an "nm-assumed" one).

nm_settings_connection_get_nm_assumed() means, "the connection was generated specifically to be activated with an assumed ActiveConnection."



> > You made some suggestions, but can you be more specific?
> 
> I don't want nm_active_connection_get_assumed() and
> nm_settings_connection_get_nm_assumed() to mean different things. So one of
> these things needs to happen:

"different things" because they are a property of two different types. But strongly related.





I don't mind renaming, if you have a good suggestion.


>   1. nm_active_connection_get_assumed() gets renamed.

to what? Before you suggested "externally created" or "externally managed" or "read only"? I don't really like these...


>   2. nm_settings_connection_get_nm_assumed() gets renamed

to what? How about "nm-generated-assumed"?


>   3. nm_settings_connection_get_nm_assumed()'s meaning changes to match
>      nm_active_connection_get_assumed()

How could they ever match? nm_active_connection_get_assumed() could return nm_settings_connection_get_nm_assumed() of its NMSettingsConnection, but then its merely a convenience function for less typing. We *need* the meaning of nm_active_connection_get_assumed() as it is (whatever we name it).

That is like saying NM_DEVICE_STATE_ACTIVATED must mean the same thing as NM_ACTIVE_CONNECTION_STATE_ACTIVATED. These are related, but different things. And that is just fine.

Renamed to "nm-generated-assumed".

Repushed.

> core: add macros NM_FLAGS_HAS(), NM_FLAGS_ANY() and NM_FLAGS_ALL()
> core: log when emitting RECHECK_ASSUME signal
> platform/trivial: move code in nm-platform.c

These are not controversial to me, and I think they could be merged immediately.

> platform: log ifindex and ifname when deleting addresses or routes

I don't think this commit does what the shortlog says...  It looks like it will only print the ifname when present, and fall back to ifindex.  If that is intended, I would say "log ifindex or ifname".

Other than that, this commit seems OK to me to merge now.

> platform: add @peer_address argument to nm_platform_ip4_address_delete()

Seems like it can be merged now.

The rest looks OK to me too, as long as we all end up agreeing on terminology.

(In reply to comment #37)
> > platform: log ifindex and ifname when deleting addresses or routes
> 
> I don't think this commit does what the shortlog says...  It looks like it will
> only print the ifname when present, and fall back to ifindex.  If that is
> intended, I would say "log ifindex or ifname".

Not really,

nm_platform_ip4_address_delete() et al. have now:
  debug ("address: deleting IPv4 address %s/%d, ifindex %d%s", 

resulting in:
  address: ..., ifindex 5 dev eth0
or:
  address: ..., ifindex 5 dev 5

(ok, not marvellous, but good enough. It's only debug logging and why would the name not be available??)

Branch merged as:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=dfdb4b8e6f547b8d8119ce75b7477d94b35eeb34