platform: merge branch 'th/platform-infiniband-cleanup-bgo765303'

https://bugzilla.gnome.org/show_bug.cgi?id=765303

5 files changed, 118 insertions, 49 deletions

diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c
index bbe466545d..6ece1b089c 100644
--- a/src/nm-core-utils.c
+++ b/src/nm-core-utils.c
@@ -1866,6 +1866,33 @@ nm_utils_new_vlan_name (const char *parent_iface, guint32 vlan_id)
 	return ifname;
 }
 
+/* nm_utils_new_infiniband_name:
+ * @name: the output-buffer where the value will be written. Must be
+ *   not %NULL and point to a string buffer of at least IFNAMSIZ bytes.
+ * @parent_name: the parent interface name
+ * @p_key: the partition key.
+ *
+ * Returns: the infiniband name will be written to @name and @name
+ *   is returned.
+ */
+const char *
+nm_utils_new_infiniband_name (char *name, const char *parent_name, int p_key)
+{
+	g_return_val_if_fail (name, NULL);
+	g_return_val_if_fail (parent_name && parent_name[0], NULL);
+	g_return_val_if_fail (strlen (parent_name) < IFNAMSIZ, NULL);
+
+	/* technically, p_key of 0x0000 and 0x8000 is not allowed either. But we don't
+	 * want to assert against that in nm_utils_new_infiniband_name(). So be more
+	 * resilient here, and accept those. */
+	g_return_val_if_fail (p_key >= 0 && p_key <= 0xffff, NULL);
+
+	/* If parent+suffix is too long, kernel would just truncate
+	 * the name. We do the same. See ipoib_vlan_add().  */
+	g_snprintf (name, IFNAMSIZ, "%s.%04x", parent_name, p_key);
+	return name;
+}
+
 /**
  * nm_utils_read_resolv_conf_nameservers():
  * @rc_contents: contents of a resolv.conf; or %NULL to read /etc/resolv.conf
diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h
index 528288c34c..486942a22a 100644
--- a/src/nm-core-utils.h
+++ b/src/nm-core-utils.h
@@ -281,6 +281,7 @@ const char *nm_utils_get_ip_config_method (NMConnection *connection,
                                            GType         ip_setting_type);
 
 char *nm_utils_new_vlan_name (const char *parent_iface, guint32 vlan_id);
+const char *nm_utils_new_infiniband_name (char *name, const char *parent_name, int p_key);
 
 GPtrArray *nm_utils_read_resolv_conf_nameservers (const char *rc_contents);
 GPtrArray *nm_utils_read_resolv_conf_dns_options (const char *rc_contents);
diff --git a/src/platform/nm-fake-platform.c b/src/platform/nm-fake-platform.c
index 7b54507696..b0d4411507 100644
--- a/src/platform/nm-fake-platform.c
+++ b/src/platform/nm-fake-platform.c
@@ -709,12 +709,13 @@ static gboolean
 infiniband_partition_add (NMPlatform *platform, int parent, int p_key, const NMPlatformLink **out_link)
 {
 	NMFakePlatformLink *device, *parent_device;
-	gs_free char *name = NULL;
+	char name[IFNAMSIZ];
 
 	parent_device = link_get (platform, parent);
 	g_return_val_if_fail (parent_device != NULL, FALSE);
 
-	name = g_strdup_printf ("%s.%04x", parent_device->link.name, p_key);
+	nm_utils_new_infiniband_name (name, parent_device->link.name, p_key);
+
 	if (!link_add (platform, name, NM_LINK_TYPE_INFINIBAND, NULL, 0, out_link))
 		return FALSE;
 
@@ -726,7 +727,6 @@ infiniband_partition_add (NMPlatform *platform, int parent, int p_key, const NMP
 	device->lnk->lnk_infiniband.p_key = p_key;
 	device->lnk->lnk_infiniband.mode = "datagram";
 	device->link.parent = parent;
-
 	return TRUE;
 }
 
@@ -739,7 +739,7 @@ infiniband_partition_delete (NMPlatform *platform, int parent, int p_key)
 	parent_device = link_get (platform, parent);
 	g_return_val_if_fail (parent_device != NULL, FALSE);
 
-	name = g_strdup_printf ("%s.%04x", parent_device->link.name, p_key);
+	nm_utils_new_infiniband_name (name, parent_device->link.name, p_key);
 	return link_delete (platform, nm_platform_link_get_ifindex (platform, name));
 }
 
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 254f9c855d..1ce4818a13 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -176,6 +176,11 @@
  * Forward declarations and enums
  ******************************************************************/
 
+typedef enum {
+	INFINIBAND_ACTION_CREATE_CHILD,
+	INFINIBAND_ACTION_DELETE_CHILD,
+} InfinibandAction;
+
 enum {
 	DELAYED_ACTION_IDX_REFRESH_ALL_LINKS,
 	DELAYED_ACTION_IDX_REFRESH_ALL_IP4_ADDRESSES,
@@ -2473,6 +2478,7 @@ sysctl_set (NMPlatform *platform, const char *path, const char *value)
 	gsize len;
 	char *actual;
 	gs_free char *actual_free = NULL;
+	int errsv;
 
 	g_return_val_if_fail (path != NULL, FALSE);
 	g_return_val_if_fail (value != NULL, FALSE);
@@ -2483,18 +2489,22 @@ sysctl_set (NMPlatform *platform, const char *path, const char *value)
 	/* Don't write to suspicious locations */
 	g_assert (!strstr (path, "/../"));
 
-	if (!nm_platform_netns_push (platform, &netns))
+	if (!nm_platform_netns_push (platform, &netns)) {
+		errno = ENETDOWN;
 		return FALSE;
+	}
 
 	fd = open (path, O_WRONLY | O_TRUNC);
 	if (fd == -1) {
-		if (errno == ENOENT) {
+		errsv = errno;
+		if (errsv == ENOENT) {
 			_LOGD ("sysctl: failed to open '%s': (%d) %s",
-			       path, errno, strerror (errno));
+			       path, errsv, strerror (errsv));
 		} else {
 			_LOGE ("sysctl: failed to open '%s': (%d) %s",
-			       path, errno, strerror (errno));
+			       path, errsv, strerror (errsv));
 		}
+		errno = errsv;
 		return FALSE;
 	}
 
@@ -2515,26 +2525,43 @@ sysctl_set (NMPlatform *platform, const char *path, const char *value)
 	actual[len] = '\0';
 
 	/* Try to write the entire value three times if a partial write occurs */
+	errsv = 0;
 	for (tries = 0, nwrote = 0; tries < 3 && nwrote != len; tries++) {
 		nwrote = write (fd, actual, len);
 		if (nwrote == -1) {
-			if (errno == EINTR) {
+			errsv = errno;
+			if (errsv == EINTR) {
 				_LOGD ("sysctl: interrupted, will try again");
 				continue;
 			}
 			break;
 		}
 	}
-	if (nwrote == -1 && errno != EEXIST) {
+	if (nwrote == -1 && errsv != EEXIST) {
 		_LOGE ("sysctl: failed to set '%s' to '%s': (%d) %s",
-		       path, value, errno, strerror (errno));
+		       path, value, errsv, strerror (errsv));
 	} else if (nwrote < len) {
 		_LOGE ("sysctl: failed to set '%s' to '%s' after three attempts",
 		       path, value);
 	}
 
-	close (fd);
-	return (nwrote == len);
+	if (nwrote != len) {
+		if (close (fd) != 0) {
+			if (errsv != 0)
+				errno = errsv;
+		} else if (errsv != 0)
+			errno = errsv;
+		else
+			errno = EIO;
+		return FALSE;
+	}
+	if (close (fd) != 0) {
+		/* errno is already properly set. */
+		return FALSE;
+	}
+
+	/* success. errno is undefined (no need to set). */
+	return TRUE;
 }
 
 static GSList *sysctl_clear_cache_list;
@@ -5078,57 +5105,67 @@ link_release (NMPlatform *platform, int master, int slave)
 /******************************************************************/
 
 static gboolean
-_infiniband_partition_action (NMPlatform *platform, int parent, int p_key, const char *action, char **ifname)
+_infiniband_partition_action (NMPlatform *platform,
+                              InfinibandAction action,
+                              int parent,
+                              int p_key,
+                              const NMPlatformLink **out_link)
 {
 	NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform);
 	const NMPObject *obj_parent;
-	gs_free char *path = NULL;
-	gs_free char *id = NULL;
+	const NMPObject *obj;
+	char path[NM_STRLEN ("/sys/class/net/%s/%s") + IFNAMSIZ + 100];
+	char id[20];
+	char name[IFNAMSIZ];
+	gboolean success;
+
+	nm_assert (NM_IN_SET (action, INFINIBAND_ACTION_CREATE_CHILD, INFINIBAND_ACTION_DELETE_CHILD));
+	nm_assert (p_key > 0 && p_key <= 0xffff && p_key != 0x8000);
 
 	obj_parent = nmp_cache_lookup_link (priv->cache, parent);
-	if (!obj_parent || !obj_parent->link.name[0])
-		g_return_val_if_reached (FALSE);
+	if (!obj_parent || !obj_parent->link.name[0]) {
+		errno = ENOENT;
+		return FALSE;
+	}
+
+	nm_sprintf_buf (path,
+	                "/sys/class/net/%s/%s",
+	                NM_ASSERT_VALID_PATH_COMPONENT (obj_parent->link.name),
+	                (action == INFINIBAND_ACTION_CREATE_CHILD
+	                     ? "create_child"
+	                     : "delete_child"));
+	nm_sprintf_buf (id, "0x%04x", p_key);
+	success = nm_platform_sysctl_set (platform, path, id);
+	if (!success) {
+		if (   action == INFINIBAND_ACTION_DELETE_CHILD
+		    && errno == ENODEV)
+			return TRUE;
+		return FALSE;
+	}
 
-	*ifname = g_strdup_printf ("%s.%04x", obj_parent->link.name, p_key);
+	nm_utils_new_infiniband_name (name, obj_parent->link.name, p_key);
+	do_request_link (platform, 0, name);
 
-	path = g_strdup_printf ("/sys/class/net/%s/%s",
-	                        NM_ASSERT_VALID_PATH_COMPONENT (obj_parent->link.name),
-	                        action);
-	id = g_strdup_printf ("0x%04x", p_key);
+	if (action == INFINIBAND_ACTION_DELETE_CHILD)
+		return TRUE;
 
-	return nm_platform_sysctl_set (platform, path, id);
+	obj = nmp_cache_lookup_link_full (priv->cache, 0, name, FALSE,
+	                                  NM_LINK_TYPE_INFINIBAND, NULL, NULL);
+	if (out_link)
+		*out_link = obj ? &obj->link : NULL;
+	return !!obj;
 }
 
-
 static gboolean
 infiniband_partition_add (NMPlatform *platform, int parent, int p_key, const NMPlatformLink **out_link)
 {
-	const NMPObject *obj;
-	gs_free char *ifname = NULL;
-
-	if (!_infiniband_partition_action (platform, parent, p_key, "create_child", &ifname))
-		return FALSE;
-
-	do_request_link (platform, 0, ifname);
-
-	obj = nmp_cache_lookup_link_full (NM_LINUX_PLATFORM_GET_PRIVATE (platform)->cache,
-	                                  0, ifname, FALSE, NM_LINK_TYPE_INFINIBAND, NULL, NULL);
-	if (out_link)
-		*out_link = obj ? &obj->link : NULL;
-	return !!obj;
+	return _infiniband_partition_action (platform, INFINIBAND_ACTION_CREATE_CHILD, parent, p_key, out_link);
 }
 
 static gboolean
 infiniband_partition_delete (NMPlatform *platform, int parent, int p_key)
 {
-	gs_free char *ifname = NULL;
-
-	if (!_infiniband_partition_action (platform, parent, p_key, "delete_child", &ifname)) {
-		if (errno != ENODEV)
-			return FALSE;
-	}
-
-	return TRUE;
+	return _infiniband_partition_action (platform, INFINIBAND_ACTION_DELETE_CHILD, parent, p_key, NULL);
 }
 
 /******************************************************************/
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 0139867a0c..58c54cb9af 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -1895,14 +1895,18 @@ _infiniband_add_add_or_delete (NMPlatform *self,
                                gboolean add,
                                const NMPlatformLink **out_link)
 {
-	gs_free char *name = NULL;
+	char name[IFNAMSIZ];
 	const NMPlatformLink *parent_link;
 	NMPlatformError plerr;
 
 	_CHECK_SELF (self, klass, NM_PLATFORM_ERROR_BUG);
 
 	g_return_val_if_fail (parent >= 0, NM_PLATFORM_ERROR_BUG);
-	g_return_val_if_fail (p_key >= 0, NM_PLATFORM_ERROR_BUG);
+	g_return_val_if_fail (p_key >= 0 && p_key <= 0xffff, NM_PLATFORM_ERROR_BUG);
+
+	/* the special keys 0x0000 and 0x8000 are not allowed. */
+	if (NM_IN_SET (p_key, 0, 0x8000))
+		return NM_PLATFORM_ERROR_UNSPECIFIED;
 
 	parent_link = nm_platform_link_get (self, parent);
 	if (!parent_link)
@@ -1911,7 +1915,7 @@ _infiniband_add_add_or_delete (NMPlatform *self,
 	if (parent_link->type != NM_LINK_TYPE_INFINIBAND)
 		return NM_PLATFORM_ERROR_WRONG_TYPE;
 
-	name = g_strdup_printf ("%s.%04x", parent_link->name, p_key);
+	nm_utils_new_infiniband_name (name, parent_link->name, p_key);
 
 	if (add) {
 		plerr = _link_add_check_existing (self, name, NM_LINK_TYPE_INFINIBAND, out_link);