device: merge branch 'th/device-set-mac-addr-no-down'

https://bugzilla.redhat.com/show_bug.cgi?id=1639274

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/54

3 files changed, 81 insertions, 34 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index ff082b3d84..d879b43c9e 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -15385,7 +15385,8 @@ _hw_addr_set (NMDevice *self,
 	NMPlatformError plerr;
 	guint8 addr_bytes[NM_UTILS_HWADDR_LEN_MAX];
 	gsize addr_len;
-	gboolean was_up;
+	gboolean was_taken_down;
+	gboolean retry_down;
 
 	nm_assert (NM_IS_DEVICE (self));
 	nm_assert (addr);
@@ -15408,21 +15409,30 @@ _hw_addr_set (NMDevice *self,
 
 	_LOGT (LOGD_DEVICE, "set-hw-addr: setting MAC address to '%s' (%s, %s)...", addr, operation, detail);
 
-	was_up = nm_device_is_up (self);
-	if (was_up) {
-		/* Can't change MAC address while device is up */
-		nm_device_take_down (self, FALSE);
-	}
+	was_taken_down = FALSE;
 
+again:
 	plerr = nm_platform_link_set_address (nm_device_get_platform (self), nm_device_get_ip_ifindex (self), addr_bytes, addr_len);
 	success = (plerr == NM_PLATFORM_ERROR_SUCCESS);
-	if (success) {
+	if (!success) {
+		retry_down =    !was_taken_down
+		             && plerr != NM_PLATFORM_ERROR_NOT_FOUND
+		             && nm_platform_link_is_up (nm_device_get_platform (self),
+		                                        nm_device_get_ip_ifindex (self));
+		_NMLOG (     retry_down
+		          || plerr == NM_PLATFORM_ERROR_NOT_FOUND
+		        ? LOGL_DEBUG
+		        : LOGL_WARN,
+		        LOGD_DEVICE,
+		        "set-hw-addr: failed to %s MAC address to %s (%s) (%s)%s",
+		        operation, addr, detail,
+		        nm_platform_error_to_string_a (plerr),
+		        retry_down ? " (retry with taking down)" : "");
+	} else {
 		/* MAC address successfully changed; update the current MAC to match */
 		nm_device_update_hw_address (self);
-		if (_hw_addr_matches (self, addr_bytes, addr_len)) {
-			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
-			       operation, addr, detail);
-		} else {
+
+		if (!_hw_addr_matches (self, addr_bytes, addr_len)) {
 			gint64 poll_end, now;
 
 			_LOGD (LOGD_DEVICE,
@@ -15463,24 +15473,40 @@ handle_fail:
 				success = FALSE;
 				break;
 			}
+		}
 
-			if (success) {
-				_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
-				       operation, addr, detail);
-			} else {
-				_LOGW (LOGD_DEVICE,
-				       "set-hw-addr: new MAC address %s not successfully %s (%s)",
-				       addr, operation, detail);
-			}
+		if (success) {
+			retry_down = FALSE;
+			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
+			       operation, addr, detail);
+		} else {
+			retry_down =    !was_taken_down
+			             && nm_platform_link_is_up (nm_device_get_platform (self),
+			                                        nm_device_get_ip_ifindex (self));
+
+			_NMLOG (  retry_down
+			        ? LOGL_DEBUG
+			        : LOGL_WARN,
+			        LOGD_DEVICE,
+			        "set-hw-addr: new MAC address %s not successfully %s (%s)%s",
+			        addr,
+			        operation,
+			        detail,
+			        retry_down ? " (retry with taking down)" : "");
 		}
-	} else {
-		_NMLOG (plerr == NM_PLATFORM_ERROR_NOT_FOUND ? LOGL_DEBUG : LOGL_WARN,
-		        LOGD_DEVICE, "set-hw-addr: failed to %s MAC address to %s (%s) (%s)",
-		        operation, addr, detail,
-		        nm_platform_error_to_string_a (plerr));
 	}
 
-	if (was_up) {
+	if (retry_down) {
+		/* changing the MAC address failed, but also the device was up (and we did not yet try to take
+		 * it down). Optimally, we change the MAC address without taking the device down, but some
+		 * devices don't like that. So, retry with taking the device down. */
+		retry_down = FALSE;
+		was_taken_down = TRUE;
+		nm_device_take_down (self, FALSE);
+		goto again;
+	}
+
+	if (was_taken_down) {
 		if (!nm_device_bring_up (self, TRUE, NULL))
 			return FALSE;
 	}
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 494e2ddaf6..f0a80cb8b5 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -1217,13 +1217,29 @@ nm_platform_link_refresh (NMPlatform *self, int ifindex)
 	return TRUE;
 }
 
-static guint
-_link_get_flags (NMPlatform *self, int ifindex)
+int
+nm_platform_link_get_ifi_flags (NMPlatform *self,
+                                int ifindex,
+                                guint requested_flags)
 {
 	const NMPlatformLink *pllink;
 
-	pllink = nm_platform_link_get (self, ifindex);
-	return pllink ? pllink->n_ifi_flags : IFF_NOARP;
+	_CHECK_SELF (self, klass, -EINVAL);
+
+	if (ifindex <= 0)
+		return -EINVAL;
+
+	/* include invisible links (only in netlink, not udev). */
+	pllink = NMP_OBJECT_CAST_LINK (nm_platform_link_get_obj (self, ifindex, FALSE));
+	if (!pllink)
+		return -ENODEV;
+
+	/* Errors are signaled as negative values. That means, you cannot request
+	 * the most significant bit (2^31) with this API. Assert against that. */
+	nm_assert ((int) requested_flags >= 0);
+	nm_assert (requested_flags < (guint) G_MAXINT);
+
+	return (int) (pllink->n_ifi_flags & requested_flags);
 }
 
 /**
@@ -1236,9 +1252,7 @@ _link_get_flags (NMPlatform *self, int ifindex)
 gboolean
 nm_platform_link_is_up (NMPlatform *self, int ifindex)
 {
-	_CHECK_SELF (self, klass, FALSE);
-
-	return NM_FLAGS_HAS (_link_get_flags (self, ifindex), IFF_UP);
+	return nm_platform_link_get_ifi_flags (self, ifindex, IFF_UP) == IFF_UP;
 }
 
 /**
@@ -1269,9 +1283,15 @@ nm_platform_link_is_connected (NMPlatform *self, int ifindex)
 gboolean
 nm_platform_link_uses_arp (NMPlatform *self, int ifindex)
 {
-	_CHECK_SELF (self, klass, FALSE);
+	int f;
 
-	return !NM_FLAGS_HAS (_link_get_flags (self, ifindex), IFF_NOARP);
+	f = nm_platform_link_get_ifi_flags (self, ifindex, IFF_NOARP);
+
+	if (f < 0)
+		return FALSE;
+	if (f == IFF_NOARP)
+		return FALSE;
+	return TRUE;
 }
 
 /**
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 2a18d8adf2..dff5102d85 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -1166,6 +1166,7 @@ int nm_platform_link_get_ifindex (NMPlatform *self, const char *name);
 const char *nm_platform_link_get_name (NMPlatform *self, int ifindex);
 NMLinkType nm_platform_link_get_type (NMPlatform *self, int ifindex);
 gboolean nm_platform_link_is_software (NMPlatform *self, int ifindex);
+int nm_platform_link_get_ifi_flags (NMPlatform *self, int ifindex, guint requested_flags);
 gboolean nm_platform_link_is_up (NMPlatform *self, int ifindex);
 gboolean nm_platform_link_is_connected (NMPlatform *self, int ifindex);
 gboolean nm_platform_link_uses_arp (NMPlatform *self, int ifindex);