Created attachment 235831 [details] [review]
core: move most spec_match_list handling into NMDevice

Since NMDevice has a generic get_hw_address() method now, it can do
nm_device_spec_match_list() itself (for everything except ethernet,
which needs to match against s390 subchannels too).

Created attachment 235832 [details] [review]
core: allow marking a device unmanaged by its interface name

Virtual devices often don't have stable hardware addresses. So allow
for marking a device unmanaged via its interface name instead.

*** Bug 692880 has been marked as a duplicate of this bug. ***

https://bugzilla.redhat.com/show_bug.cgi?id=902907 points out additional issues involving matching bridge interfaces

Yeah, we can create any number of unmanaged specs we want here, but again this will go away when we cooperatively handle the interfaces.  SO maybe some hacks here that we remove later, if that's possible.

Created attachment 238399 [details] [review]
core: fix NMDeviceEthernet matching

nm_device_connection_match_config() on an ethernet connection could
succeed for a device that was in the connection's
mac-address-blacklist, but then NM would immediately fail to activate
the connection because nm_device_connection_compatible() would check
the blacklist and return FALSE. Fix the former to match the latter.

Created attachment 238401 [details] [review]
core: simplify nm_device_spec_match_list()

Since NMDevice has a generic get_hw_address() method now, it can do
nm_device_spec_match_list() itself (for everything except ethernet,
which needs to match against s390 subchannels too).

Created attachment 238402 [details] [review]
core: simplify/rename nm_device_connection_match_config()

nm_device_connection_match_config() sounded more generic than it
really was; rename it to nm_device_find_assumable_connection(), which
is what it really does.

There was also a lot of redundancy/cut+paste in the subclass
implementations of connection_match_config(); Improve things by moving
the looping-over-connections code into NMDevice itself, and also doing
the general-device-compatibility and IP-config checking there, leaving
the device subclasses to just verify L2 properties. Which most of them
aren't doing...

Created attachment 238403 [details] [review]
core: simplify nm_device_get_best_auto_connection()

As with the other connection-matching methods, move the loop and the
device-independent bits into NMDevice. By reusing
nm_device_check_connection_compatible(), this means that most device
types now no longer need any type-specific code for this.

Created attachment 238404 [details] [review]
libnm-util: add NMSettingConnection:interface-name

Created attachment 238405 [details] [review]
settings: read/write NMSettingConnection:interface-name

Created attachment 238406 [details] [review]
core: make nm_device_check_connection_compatible() check interface-name

If an NMConnection specifies an interface-name, it is only compatible
with a device with the same ip-iface.

Created attachment 238407 [details] [review]
core: allow marking a device unmanaged by its interface name

Virtual devices often don't have stable hardware addresses. So allow
for marking a device unmanaged via its interface name instead.

(In reply to comment #11)
> Created an attachment (id=238405) [details] [review]
> settings: read/write NMSettingConnection:interface-name

the non-ifcfg-rh non-keyfile parts of this are untested and probably completely wrong...

also pushed to danw/iface-match

Created attachment 238580 [details] [review]
libnm-glib: match interface name in nm_device_connection_compatible()

Do NMSettingConnection:interface-name matching on the client side as
well, so that, eg, nm-applet does not list connections under the wrong
device.

(Also, move some return-if-fail checks from the subclass method
implementations into the wrapper function.)

Review of attachment 238399 [details] [review]:

Looks good.

Review of attachment 238401 [details] [review]:

Looks good.

Review of attachment 238402 [details] [review]:

One thing to note is that find_assumable_connection() now uses check_connection_compatible().  There's a behavioral difference between the two; find_assumable() matches a connection specifically to a device's existing configuration, while check_connection_compatible() is a much looser check to determine if the device *could*, if reconfigured, use this connection.  I don't think that matters right now but coudn't you note that in some docs for find_assumable_connection?

Review of attachment 238403 [details] [review]:

Looks good.

Review of attachment 238404 [details] [review]:

Looks good, though one docs change suggestion.

::: libnm-util/nm-setting-connection.c
@@ +1041,3 @@
+	 **/
+	g_object_class_install_property
+		(object_class, PROP_INTERFACE_NAME,

For the last part, let's go with something more like:

For connection types where interface names cannot easily be made persistent (eg, mobile broadband or USB ethernet), this property should not be used.  Setting this property restricts the interfaces a connection can be used with, and if interface names change or are reordered the connection may be applied to the wrong interface.

Review of attachment 238405 [details] [review]:

Looks good.

Review of attachment 238406 [details] [review]:

::: src/nm-device.c
@@ +1474,3 @@
+
+	config_iface = nm_setting_connection_get_interface_name (s_con);
+	device_iface = nm_device_get_ip_iface (device);

We should be checking nm_device_get_iface() here, not ip_iface.  Using ip_iface() here would mean that when eth0 has an active PPPoE connection, suddenly any connection with the interface name "eth0" no longer applies to the NMDevice because the IP iface is ppp0, not eth0.

Also, the IP iface is dynamically allocated, so it could be ppp1, ppp2, nas5, etc.

@@ +1475,3 @@
+	config_iface = nm_setting_connection_get_interface_name (s_con);
+	device_iface = nm_device_get_ip_iface (device);
+	if (config_iface && device_iface && strcmp (config_iface, device_iface) != 0)

nm_device_get_iface() should always return something, otherwise the sky is falling.  We don't need the check for "device_iface" here.  Feel free to g_assert (device_iface) if you want.

(In reply to comment #19)
> Review of attachment 238402 [details] [review]:
> 
> One thing to note is that find_assumable_connection() now uses
> check_connection_compatible().

It doesn't *only* use check_connection_compatible() though; that's just one of several checks it makes.

Passing check_connection_compatible() is a necessary (but not sufficient) condition for being assumable, because if NMManager decides to assume a connection, it will call internal_activate_device() on it, which then immediately calls nm_device_check_connection_compatible().

Review of attachment 238407 [details] [review]:

::: src/nm-device.c
@@ +5504,3 @@
+	if (matched)
+		return TRUE;
+	else if (nm_match_spec_interface_name (specs, nm_device_get_ip_iface (device)))

We want nm_device_get_iface() here, not ip_iface.

@@ +5507,3 @@
+		return TRUE;
+	else
+		return FALSE;

Seems a bit clearer to do something like:

if (!matched)
    matched = nm_match_spec_interface_name (specs, nm_device_get_ip_iface (device));

return matched;

::: src/settings/plugins/keyfile/plugin.c
@@ +457,3 @@
 		for (i = 0; udis[i] != NULL; i++) {
 			/* Verify unmanaged specification and add it to the list */
+			if (!strncmp (udis[i], "mac:", 4) && ether_aton (udis[i] + 4)) {

g_str_has_prefix()?

@@ +467,3 @@
 				}
 				specs = g_slist_append (specs, udis[i]);
+			} else if (!strncmp (udis[i], "interface-name:", 10) && nm_utils_iface_valid_name (udis[i] + 10)) {

g_str_has_prefix()?

Review of attachment 238580 [details] [review]:

It kinda burns me that we have this API, since we now have AvailableConnections and I can't think of a good use for checking some random connection against the device that's not already in NetworkManager's AvailableConnections list for the device, which means a lot of duplicated code between libnm-glib and NM itself.  Oh well...

::: libnm-glib/nm-device.c
@@ +1619,3 @@
+
+	config_iface = nm_setting_connection_get_interface_name (s_con);
+	device_iface = nm_device_get_ip_iface (device);

nm_device_get_iface()

(In reply to comment #25)
> @@ +5507,3 @@
> +        return TRUE;
> +    else
> +        return FALSE;
> 
> Seems a bit clearer to do something like:
> 
> if (!matched)
>     matched = nm_match_spec_interface_name (specs, nm_device_get_ip_iface
> (device));
> 
> return matched;

And of course here I mean to use nm_device_get_iface() too...

(In reply to comment #25)
> +            if (!strncmp (udis[i], "mac:", 4) && ether_aton (udis[i] + 4)) {
> 
> g_str_has_prefix()?

thought about that, but I left it as it was because we'd still be hardcoding the length later on in the line anyway

fixed and pushed

*** Bug 696988 has been marked as a duplicate of this bug. ***

*** Bug 683102 has been marked as a duplicate of this bug. ***