http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/default-unmanaged-bgo746566

(In reply to Lubomir Rintel from comment #0)
> Discussion in bug #731014
> 
> Two options:
> 
> 1.) Make everything default-unmanaged -- allow activation from UNMANAGED
> state
> 
> 2.) Remove default-unamnaged and change Device.Managed property to
> read-write.

the branch goes with 2.), right? That seems better to me.






>> device: allow modifying Managed property


+    case PROP_MANAGED:
...
+         } else {
+              if (priv->state != NM_DEVICE_STATE_UNMANAGED)
+                   nm_device_state_changed (self,
+                                            NM_DEVICE_STATE_UNMANAGED,
+                                            NM_DEVICE_STATE_REASON_USER_REQUESTED);
+         }


can you just transition the device from any state to unmanged?

For example, from ACTIVATED, don't you first have to cycle through NM_DEVICE_STATE_DEACTIVATING, etc?

I must admit, I still don't get this state change logic...





"TODO: is UNMANAGED_USER appropriate for generic / veth?"

I'd say so. This looks right to me.

I took lr/default-unmanaged-bgo746566 and rebased it on master.
I want to use this as a base for doing bug 746440.

The original version is still at lr/default-unmanaged-bgo746566-1.


Did a few changes too...

>> cli: add nmcli device set command

+                  "ARGUMENTS := <ifname> { SETTING, [ SETTING ... ] }\n"

as passing multiple settings does not mean that those settings are set atomically together, so I don't think there is much value in accepting multiple settings.

It complicates stuff, but is not much better (atomic) then:
  nmcli device set managed on &&
  nmcli device set autoconnect on

Rebased and heavily reworked the branch.

Up to the last patch, they seem good to me. The last need more thought (but already review too).



343d0a2 device: remove default-unmanaged
1322fcd device/trivial: rename nm_device_get_unmanaged_flag() to nm_device_get_unmanaged()
1804b08 device/trivial: rename nm_device_set_initial_unmanaged_flag() to nm_device_set_unmanaged_initial()
746c64f cli: add nmcli device set command
7450cac cli,trivial: move nmc_switch_parse_on_off
6bfaeae libnm,libnm-glib: add Device.Manager setter
0a6d940 device: allow modifying Managed property
56609c6 device: add explicit NM_UNMANAGED_LOOPBACK flag for not managing "lo"
cb102b3 core: refactor setting of D-Bus properties via NMManager
143856e core: refactor NMBusManager to hold reference to NMExportedObject directly
2333126 core: forward declare NMExportedObject in "nm-types.h"



The first 3 patches fix a bug in NMManager handling setting properties via D-Bus.

143856e core: refactor NMBusManager to hold reference to NMExportedObject directly

>@@ -547,12 +598,17 @@ nm_bus_manager_dispose (GObject *object)

>+			g_hash_table_iter_remove (&iter);
>+		}
> 
> 		g_hash_table_destroy (priv->exported);

Removing each element as you go, when you're planning to destroy the whole hash table afterward anyway, just makes things slower, right?

>-object_destroyed (NMBusManager *self, gpointer object)
>+object_destroyed (NMBusManager *self, NMExportedObject *object) {
>-	g_hash_table_remove (NM_BUS_MANAGER_GET_PRIVATE (self)->exported, object);
>+	nm_assert_exported (self, NULL, object);
>+
>+	g_hash_table_remove (NM_BUS_MANAGER_GET_PRIVATE (self)->exported,
>+	                     nm_exported_object_get_path (object));

This is wrong; when a weak-notify function is called, the object has already been destroyed. The original code was fine because we were only using the pointer value of the object, not dereferencing it, but the new code will crash.

Since you didn't notice any crash, that implies that this code is never actually getting run. Which seems to be true; nm_exported_object_dispose() always unexports the device if it had been exported, so we never hit the case where an object is freed while it's still exported. So probably this can just go away.



cb102b3 core: refactor setting of D-Bus properties via NMManager

>    - set the property on the proper D-Bus skeleton interface instead
>      setting it directly on the exported-object.

You don't need to do that; NMExportedObject sets up GBindings between the object's properties and its skeletons' properties.



6bfaeae libnm,libnm-glib: add Device.Manager setter

I wouldn't add it to libnm-glib... Also, typo "Manager"


(Didn't look at the rest of the commits.)

(In reply to Dan Winship from comment #6)
> 143856e core: refactor NMBusManager to hold reference to NMExportedObject
> directly
> 
> >@@ -547,12 +598,17 @@ nm_bus_manager_dispose (GObject *object)
> 
> >+			g_hash_table_iter_remove (&iter);
> >+		}
> > 
> > 		g_hash_table_destroy (priv->exported);
> 
> Removing each element as you go, when you're planning to destroy the whole
> hash table afterward anyway, just makes things slower, right?

dropped the g_h_t_iter_remove().


> >-object_destroyed (NMBusManager *self, gpointer object)
> >+object_destroyed (NMBusManager *self, NMExportedObject *object) {
> >-	g_hash_table_remove (NM_BUS_MANAGER_GET_PRIVATE (self)->exported, object);
> >+	nm_assert_exported (self, NULL, object);
> >+
> >+	g_hash_table_remove (NM_BUS_MANAGER_GET_PRIVATE (self)->exported,
> >+	                     nm_exported_object_get_path (object));
> 
> This is wrong; when a weak-notify function is called, the object has already
> been destroyed. The original code was fine because we were only using the
> pointer value of the object, not dereferencing it, but the new code will
> crash.

A weak-notify is called before starting to destroy the object. The object is still valid at that point and priv->path must sill be set.

Anyway, there was a bug here, and I just removed the weak-ref.


> Since you didn't notice any crash, that implies that this code is never
> actually getting run. Which seems to be true; nm_exported_object_dispose()
> always unexports the device if it had been exported, so we never hit the
> case where an object is freed while it's still exported. So probably this
> can just go away.


> cb102b3 core: refactor setting of D-Bus properties via NMManager
> 
> >    - set the property on the proper D-Bus skeleton interface instead
> >      setting it directly on the exported-object.
> 
> You don't need to do that; NMExportedObject sets up GBindings between the
> object's properties and its skeletons' properties.

I wanted to do it to ensure we set the right property (like, two D-Bus interfaces could have identically named properties, that map to different NMObject-properties.

But I just remove that.


> 6bfaeae libnm,libnm-glib: add Device.Manager setter
> 
> I wouldn't add it to libnm-glib... Also, typo "Manager"

it's a very old patch...


fixup follows (soon)

(In reply to Thomas Haller from comment #7)
> A weak-notify is called before starting to destroy the object. The object is
> still valid at that point and priv->path must sill be set.

Hm... while it happens that currently the weaknotify is called before finalize(), I'm not sure that was always the case, and you shouldn't depend on it. The docs say: "Since the object is already being finalized when the #GWeakNotify is called, there's not much you could do with the object, apart from e.g. using its address as hash-index or the like.". (Note that the prototype for a GWeakNotify refers to the object as "GObject *where_the_object_was".)

> > You don't need to do that; NMExportedObject sets up GBindings between the
> > object's properties and its skeletons' properties.
> 
> I wanted to do it to ensure we set the right property (like, two D-Bus
> interfaces could have identically named properties, that map to different
> NMObject-properties.

We don't have any support for non-standard D-Bus-property-name-to-GObject-property-name mappings, so if the properties had the same D-Bus property name, they'd have to have the same GObject property name too, and GObject wouldn't let you do that.

merged an early part of the branch that fixes setting D-Bus properties:

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

... and included two non-controversial(?) patches.


Rest is again on review...

All the patches up to the WIP "device: remove default-unmanaged" patch look OK to me.

For the WIP patch the general approach seems OK to me, but I'm having some trouble figuring out from the patch how NM_UNMANAGED_USER gets flipped when the device is manually activated?

merged all but the last patch of lr/default-unmanaged-bgo746566

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

(In reply to Dan Williams from comment #10)
> For the WIP patch the general approach seems OK to me, but I'm having some
> trouble figuring out from the patch how NM_UNMANAGED_USER gets flipped when
> the device is manually activated?

Haven't looked at all of this branch, but in my old danw/no-default-unmanaged branch the idea was that automatic-manage-on-activate would go away, because it was so complicated and buggy, and you'd have to explicitly make the device managed first before you could activate it.

(In reply to Dan Winship from comment #12)
> (In reply to Dan Williams from comment #10)
> > For the WIP patch the general approach seems OK to me, but I'm having some
> > trouble figuring out from the patch how NM_UNMANAGED_USER gets flipped when
> > the device is manually activated?

indeed, it's not there (yet).


> Haven't looked at all of this branch, but in my old
> danw/no-default-unmanaged branch the idea was that
> automatic-manage-on-activate would go away, because it was so complicated
> and buggy, and you'd have to explicitly make the device managed first before
> you could activate it.

I imagined, if the user activates a connection that is unmanaged-user, we clear the unmanaged-user flag, and start activation -- provided the device is not unmanaged for other reasons.

When the interface deactivates, it would not go back to unmanaged automatically but stay disconnected/managed.


(v2 will follow)

Please review lr/default-unmanaged-bgo746566 branch. See also: https://mail.gnome.org/archives/networkmanager-list/2015-November/msg00031.html

> device: remove default-unmanaged and refactor unamanged flags

+       if (for_user_request) {
+
+               /* @for_user_request can make the result only ~more~ managed.
+                * If the flags already indicate a managed state for a non-user-request,
+                * then it is also managed for an explict user-request. */
+               if (_get_managed (flags, mask, FALSE))
+                       return TRUE;

At first sight this doesn't seem to be required because when
@for_user_request is set the remaining code in the "if" makes the
device "more managed".

Also, pushed a fixup.

Otherwise looks pretty good.

(In reply to Beniamino Galvani from comment #15)
> > device: remove default-unmanaged and refactor unamanged flags
> 
> +       if (for_user_request) {
> +
> +               /* @for_user_request can make the result only ~more~ managed.
> +                * If the flags already indicate a managed state for a
> non-user-request,
> +                * then it is also managed for an explict user-request. */
> +               if (_get_managed (flags, mask, FALSE))
> +                       return TRUE;
> 
> At first sight this doesn't seem to be required because when
> @for_user_request is set the remaining code in the "if" makes the
> device "more managed".

Indeed, I was aware of that. Removing this code would make no difference. I did it because I wanted the invariant (that for-user-request means "more-managed") be expressed explicitly.

Also, when refactoring the code below, the invariant cannot break. So, I'd keep it, but add a code comment about that.

(In reply to Thomas Haller from comment #16)
> Also, when refactoring the code below, the invariant cannot break. So, I'd
> keep it, but add a code comment about that.

Ah, ok. I agree then.

rebased on master.

(merged an early part)

Overall looks OK.  There are two things that are now broken though, and that's explicit activation of default-unmanaged devices and management of unmanaged devices with connections.

-----------------------------
reproduce for default-unmanaged activation

1) add ENV{INTERFACE}=="foobar0", ENV{NM_UNMANAGED}="1" to /lib/udev/rules.d/85-nm-unmanaged.rules  (this simulates the old default-unmanaged)
2) udevadm control --reload
3) brctl addbr foobar0 - note that the device is (correctly) unmanaged due to !IFF_UP and externally created
4) ip link set dev foobar0 up - note that device is *still* unmanaged due to the udev rules
5) create a bridge connection for "foobar0":
DEVICE=foobar0
STP=no
TYPE=Bridge
ONBOOT=no
IPADDR=1.2.3.4
PREFIX=24
NAME=foobar0
6) nmcli con reload
7) nmcli dev show foobar0 -- note there are no available connections which I think is wrong; with default-unmanaged devices previously they are available when the device is unmanaged because you need to support 'nmcli con up' and devices cannot (and sholdn't) be allowed to activate when no connections are available on them
7) nmcli con up foobar0
8) crash

In this case, and previously with defualt-unmanaged devices, the device should be available for activation but that doesn't happen because the device has no available-connections.

-------------------------------------
Mis-management of devices with connections

This is probably caused by some missing code in my original dcbw/devices-for-all branch, or maybe a forgotten assumption in the new code.  But try this:

1) add ENV{INTERFACE}=="foobar0", ENV{NM_UNMANAGED}="1" to /lib/udev/rules.d/85-nm-unmanaged.rules
2) udevadm control --reload
3) create a bridge connection for "foobar0":
DEVICE=foobar0
STP=no
TYPE=Bridge
ONBOOT=no
IPADDR=1.2.3.4
PREFIX=24
NAME=foobar0
4) nmcli con reload
6) brctl addbr foobar0 - note that the device is (correctly) unmanaged due to !IFF_UP
7) ip link set dev foobar0 up - device now becomes managed, even though no explicit action has been taken

I think here the issue is that the IFF_UP checking bits in device_link_changed() aren't properly accounting for externally-created, because the device already exists (unrealized) due to dcbw/devices-for-all and the connection existing before the brctl addbr.

Does the re-push of the branch fix both of the above issues?

First patches look fine to me.

nm_device_set_unmanaged_flags() is a bit confusing to me; I think it would clearer to split the state change variant from the non-state-change variant.  So have a base nm_device_set_unmanaged_flags_simple() or something and then the normal nm_device_set_unmanaged_flags() that does change state.  That way we don't need the fake DEVICE_STATE_REASON value either?  Something like http://fpaste.org/310027/45264029/ perhaps.

Maybe nm_device_set_unmanaged_flags_user_udev() -> nm_device_update_unmanaged_flags_user_udev() since you can't pass flags directly into that function?

The FORGET operation isn't used; remove it?

(In reply to Dan Williams from comment #21)

> nm_device_set_unmanaged_flags() is a bit confusing to me; I think it would
> clearer to split the state change variant from the non-state-change variant.
> So have a base nm_device_set_unmanaged_flags_simple() or something and then
> the normal nm_device_set_unmanaged_flags() that does change state.  That way
> we don't need the fake DEVICE_STATE_REASON value either?  Something like
> http://fpaste.org/310027/45264029/ perhaps.

Split and renamed functions.

Now we have plain getter/setter for flags:

  nm_device_get_unmanaged_flags()
  nm_device_set_unmanaged_flags()

then setting flags, and possibly doing a state change:

  nm_device_set_unmanaged_by_*().



> Maybe nm_device_set_unmanaged_flags_user_udev() ->
> nm_device_update_unmanaged_flags_user_udev() since you can't pass flags
> directly into that function?

I renamed it to nm_device_set_unmanaged_by_user_config().
The first part of the name is to match the nm_device_set_unmanaged_by_* pattern.
The *_user_config() part is because it set NM_UNMANAGED_USER_CONFIG flag.


> The FORGET operation isn't used; remove it?

Used now.

Repushed, latest and greatest.

merged another early part of the branch that fixes bugs on master (related to realizing devices): http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=320a3dee14d3d3675379861a7ed07a98544349f5

I like this latest version.  But one issue I see is that non-platform devices are not correctly managed in realize_start_setup().

You can reproduce this with:

modprobe atmtcp
atmtcp virtual listen

and not how the atm0 device never gets managed.  I think this is because of:

	nm_device_set_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT,
	                               !plink || !plink->initialized);

for these devices, plink == NULL, so they will always have PLATFORM_INIT set.  The logic should be:

	nm_device_set_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT,
	                               plink ? !plink->initialized : FALSE);

to ensure that non-platform devices get the PLATFORM_INIT flag cleared when they are realized.

For the splitting of the functions from the earlier comment, I was thinking more that nm_device_set_unmanaged_flags() would do the switch statement and basic logging, and the state-change-triggering function would do all the rest of the stuff but call nm_device_set_unmanaged_flags() in between.  But this way is OK too I guess.

(In reply to Dan Williams from comment #25)
> I like this latest version.  But one issue I see is that non-platform
> devices are not correctly managed in realize_start_setup().
> 
> You can reproduce this with:
> 
> modprobe atmtcp
> atmtcp virtual listen
> 
> and not how the atm0 device never gets managed.  I think this is because of:
> 
> 	nm_device_set_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT,
> 	                               !plink || !plink->initialized);
> 
> for these devices, plink == NULL, so they will always have PLATFORM_INIT
> set.  The logic should be:
> 
> 	nm_device_set_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT,
> 	                               plink ? !plink->initialized : FALSE);
> 
> to ensure that non-platform devices get the PLATFORM_INIT flag cleared when
> they are realized.

fixed.



> For the splitting of the functions from the earlier comment, I was thinking
> more that nm_device_set_unmanaged_flags() would do the switch statement and
> basic logging, and the state-change-triggering function would do all the
> rest of the stuff but call nm_device_set_unmanaged_flags() in between.  But
> this way is OK too I guess.

I got that, but I wanted to do logging in one line telling which flags change, but also whether we change state (and the reason).

For that I pass @reason and @allow_state_transition down to _set_unmanaged_flags(). As there are really only two callers of _set_unmanaged_flags(), I figured this is the cleanest solution. 




Repushed, but still needs more testing.

In _nm_device_check_connection_available(), should the early-return when the device isn't realized also ensure the device is a software/virtual device?  Hardware devices obviously cannot have any available connections until they are realized...

Also now in _set_state_full() the code allows re-entering the UNAVAILABLE state for all cases, where previously it was just for missing firmware.  What is that necessary?  The only reason it's necessary for missing firmware is that devices are typically opened during UNAVAILABLE and that's where the firmware error would happen (ENOENT from IFF_UP), and to get that to happen again we need to redo the IFF_UP when firmware directory changes.  But why would we re-enter UNAVAILABLE from normal cases?

(In reply to Dan Williams from comment #27)
> In _nm_device_check_connection_available(), should the early-return when the
> device isn't realized also ensure the device is a software/virtual device? 
> Hardware devices obviously cannot have any available connections until they
> are realized...

Fixup added.


> Also now in _set_state_full() the code allows re-entering the UNAVAILABLE
> state for all cases, where previously it was just for missing firmware. 
> What is that necessary?  The only reason it's necessary for missing firmware
> is that devices are typically opened during UNAVAILABLE and that's where the
> firmware error would happen (ENOENT from IFF_UP), and to get that to happen
> again we need to redo the IFF_UP when firmware directory changes.  But why
> would we re-enter UNAVAILABLE from normal cases?

There was a reason why I did that, but I forgot. I reverted it for now, let's see how tests work.


Repushed.

finally merged to master:

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