ifcfg-rh: various VLAN cleanups

Add some testcases checking for DEVICE/PHYSDEV/VLAN_ID variations,
and read/write the new VLAN_ID tag, which we can use in
combination with the 'parent' property to determine the interface
name if no interface name/DEVICE is given.

src/settings/plugins/ifcfg-rh/reader.c

@@ -3744,13 +3744,27 @@ make_vlan_setting (shvarFile *ifcfg,
 	NMSettingVlan *s_vlan = NULL;
 	char *value = NULL;
 	char *iface_name = NULL;
-	char *master = NULL;
+	char *parent = NULL;
-	char *p = NULL;
+	const char *p = NULL, *w;
-	guint32 vlan_id = 0;
+	gboolean has_numbers = FALSE;
+	gint vlan_id = -1;
 	guint32 vlan_flags = 0;
 
+	value = svGetValue (ifcfg, "VLAN_ID", FALSE);
+	if (value) {
+		errno = 0;
+		vlan_id = (gint) g_ascii_strtoll (value, NULL, 10);
+		if (vlan_id < 0 || vlan_id > 4096 || errno) {
+			g_set_error (error, IFCFG_PLUGIN_ERROR, 0, "Invalid VLAN_ID '%s'", value);
+			g_free (value);
+			return NULL;
+		}
+		g_free (value);
+	}
+
+	/* Need DEVICE if we don't have a separate VLAN_ID property */
 	iface_name = svGetValue (ifcfg, "DEVICE", FALSE);
-	if (!iface_name) {
+	if (!iface_name && vlan_id < 0) {
 		g_set_error_literal (error, IFCFG_PLUGIN_ERROR, 0,
 		                     "Missing DEVICE property; cannot determine VLAN ID.");
 		return NULL;
@@ -3758,47 +3772,65 @@ make_vlan_setting (shvarFile *ifcfg,
 
 	s_vlan = NM_SETTING_VLAN (nm_setting_vlan_new ());
 
-	g_object_set (s_vlan, NM_SETTING_VLAN_INTERFACE_NAME, iface_name, NULL);
+	if (iface_name) {
+		g_object_set (s_vlan, NM_SETTING_VLAN_INTERFACE_NAME, iface_name, NULL);
 
-	p = strchr (iface_name, '.');
+		p = strchr (iface_name, '.');
-	if (p) {
+		if (p) {
-		/* eth0.43; PHYSDEV is assumed from it */
+			/* eth0.43; PHYSDEV is assumed from it */
-		master = g_strndup (iface_name, p - iface_name);
+			parent = g_strndup (iface_name, p - iface_name);
-		p++;
+			p++;
-	} else {
+		} else {
-		/* format like vlan43; PHYSDEV or MASTER must be set */
+			/* format like vlan43; PHYSDEV or MASTER must be set */
-		if (g_str_has_prefix (iface_name, "vlan"))
+			if (g_str_has_prefix (iface_name, "vlan"))
-			p = iface_name + 4;
+				p = iface_name + 4;
+		}
 
-		master = svGetValue (ifcfg, "PHYSDEV", FALSE);
+		w = p;
-		if (master == NULL)
+		while (*w && !has_numbers)
-			master = svGetValue (ifcfg, "MASTER", FALSE);
+			has_numbers = g_ascii_isdigit (*w);
+
+		/* Grab VLAN ID from interface name; this takes precedence over the
+		 * separate VLAN_ID property for backwards compat.
+		 */
+		if (has_numbers) {
+			errno = 0;
+			vlan_id = (gint) g_ascii_strtoll (p, NULL, 10);
+			if (vlan_id < 0 || vlan_id > 4095 || errno) {
+				g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
+				             "Failed to determine VLAN ID from DEVICE '%s'",
+				             iface_name);
+				goto error;
+			}
+		}
 	}
 
-	if (master == NULL) {
+	if (vlan_id < 0) {
 		g_set_error_literal (error, IFCFG_PLUGIN_ERROR, 0,
-				             "Failed to determine VLAN master from DEVICE, PHYSDEV, or MASTER");
+		                     "Failed to determine VLAN ID from DEVICE or VLAN_ID.");
 		goto error;
 	}
+	g_object_set (s_vlan, NM_SETTING_VLAN_ID, vlan_id, NULL);
 
-	vlan_id = g_ascii_strtoull (p, NULL, 10);
+	if (!parent)
-	if (vlan_id >= 0 && vlan_id < 4096)
+		parent = svGetValue (ifcfg, "PHYSDEV", FALSE);
-		g_object_set (s_vlan, NM_SETTING_VLAN_ID, vlan_id, NULL);
+	if (parent == NULL) {
-	else {
+		g_set_error_literal (error, IFCFG_PLUGIN_ERROR, 0,
-		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
+		                     "Failed to determine VLAN parent from DEVICE or PHYSDEV");
-		             "Failed to determine VLAN ID from DEVICE '%s'",
-		             iface_name);
 		goto error;
 	}
+	g_object_set (s_vlan, NM_SETTING_VLAN_PARENT, parent, NULL);
 
 	if (svTrueValue (ifcfg, "REORDER_HDR", FALSE))
 		vlan_flags |= NM_VLAN_FLAG_REORDER_HEADERS;
 
 	value = svGetValue (ifcfg, "VLAN_FLAGS", FALSE);
-	if (g_strstr_len (value, -1, "GVRP"))
+	if (value) {
-		vlan_flags |= NM_VLAN_FLAG_GVRP;
+		if (g_strstr_len (value, -1, "GVRP"))
-	if (g_strstr_len (value, -1, "LOOSE_BINDING"))
+			vlan_flags |= NM_VLAN_FLAG_GVRP;
-		vlan_flags |= NM_VLAN_FLAG_LOOSE_BINDING;
+		if (g_strstr_len (value, -1, "LOOSE_BINDING"))
+			vlan_flags |= NM_VLAN_FLAG_LOOSE_BINDING;
+	}
 
 	g_object_set (s_vlan, NM_SETTING_VLAN_FLAGS, vlan_flags, NULL);
 	g_free (value);
@@ -3807,11 +3839,11 @@ make_vlan_setting (shvarFile *ifcfg,
 	parse_prio_map_list (s_vlan, ifcfg, "VLAN_EGRESS_PRIORITY_MAP", NM_VLAN_EGRESS_MAP);
 
 	if (out_master)
-		*out_master = master;
+		*out_master = svGetValue (ifcfg, "MASTER", FALSE);
 	return (NMSetting *) s_vlan;
 
 error:
-	g_free (master);
+	g_free (parent);
 	g_free (iface_name);
 	g_object_unref (s_vlan);
 	return NULL;
@@ -3858,12 +3890,7 @@ vlan_connection_from_ifcfg (const char *file,
 	nm_connection_add_setting (connection, vlan_setting);
 
 	/* Handle master interface or connection */
-	if (master == NULL) {
+	if (master) {
-		g_set_error_literal (error, IFCFG_PLUGIN_ERROR, 0,
-		                     "No master interface or connection UUID specified.");
-		g_object_unref (connection);
-		return NULL;
-	} else {
 		g_object_set (con_setting, NM_SETTING_CONNECTION_MASTER, master, NULL);
 		g_object_set (con_setting,
 		              NM_SETTING_CONNECTION_SLAVE_TYPE, NM_SETTING_VLAN_SETTING_NAME,

src/settings/plugins/ifcfg-rh/tests/network-scripts/Makefile.am

@@ -79,6 +79,8 @@ EXTRA_DIST = \
 	ifcfg-test-bridge-main \
 	ifcfg-test-bridge-component \
 	ifcfg-test-vlan-interface \
+	ifcfg-test-vlan-only-vlanid \
+	ifcfg-test-vlan-only-device \
 	ifcfg-test-wifi-wep-no-keys \
 	ifcfg-test-permissions \
 	ifcfg-test-wifi-wep-agent-keys \

src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-only-device

@@ -0,0 +1,4 @@
+VLAN=yes
+TYPE=Vlan
+DEVICE=eth0.9
+

src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-vlan-only-vlanid

@@ -0,0 +1,6 @@
+VLAN=yes
+TYPE=Vlan
+PHYSDEV=eth9
+VLAN_ID=43
+ONBOOT=yes
+

src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c

@@ -11756,7 +11756,6 @@ test_read_vlan_interface (void)
 	char *route6file = NULL;
 	gboolean ignore_error = FALSE;
 	GError *error = NULL;
-	NMSettingConnection *s_con;
 	NMSettingVlan *s_vlan;
 	guint32 from = 0, to = 0;
 
@@ -11778,16 +11777,11 @@ test_read_vlan_interface (void)
 	g_free (routefile);
 	g_free (route6file);
 
-	s_con = nm_connection_get_setting_connection (connection);
-	g_assert (s_con);
-
-	g_assert_cmpstr (nm_setting_connection_get_master (s_con), ==, "eth9");
-	g_assert_cmpstr (nm_setting_connection_get_slave_type (s_con), ==, NM_SETTING_VLAN_SETTING_NAME);
-
 	s_vlan = nm_connection_get_setting_vlan (connection);
 	g_assert (s_vlan);
 
 	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, "vlan43");
+	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 43);
 	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==,
 	                 NM_VLAN_FLAG_GVRP | NM_VLAN_FLAG_LOOSE_BINDING);
@@ -11821,6 +11815,90 @@ test_read_vlan_interface (void)
 	g_object_unref (connection);
 }
 
+#define TEST_IFCFG_VLAN_ONLY_VLANID TEST_IFCFG_DIR"/network-scripts/ifcfg-test-vlan-only-vlanid"
+
+static void
+test_read_vlan_only_vlan_id (void)
+{
+	NMConnection *connection;
+	char *unmanaged = NULL;
+	char *keyfile = NULL;
+	char *routefile = NULL;
+	char *route6file = NULL;
+	gboolean ignore_error = FALSE;
+	GError *error = NULL;
+	NMSettingVlan *s_vlan;
+
+	connection = connection_from_file (TEST_IFCFG_VLAN_ONLY_VLANID,
+	                                   NULL,
+	                                   TYPE_ETHERNET,
+	                                   NULL,
+	                                   &unmanaged,
+	                                   &keyfile,
+	                                   &routefile,
+	                                   &route6file,
+	                                   &error,
+	                                   &ignore_error);
+	g_assert_no_error (error);
+	g_assert (connection != NULL);
+
+	g_free (unmanaged);
+	g_free (keyfile);
+	g_free (routefile);
+	g_free (route6file);
+
+	s_vlan = nm_connection_get_setting_vlan (connection);
+	g_assert (s_vlan);
+
+	g_assert (nm_setting_vlan_get_interface_name (s_vlan) == NULL);
+	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
+	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 43);
+
+	g_object_unref (connection);
+}
+
+#define TEST_IFCFG_VLAN_ONLY_DEVICE TEST_IFCFG_DIR"/network-scripts/ifcfg-test-vlan-only-device"
+
+static void
+test_read_vlan_only_device (void)
+{
+	NMConnection *connection;
+	char *unmanaged = NULL;
+	char *keyfile = NULL;
+	char *routefile = NULL;
+	char *route6file = NULL;
+	gboolean ignore_error = FALSE;
+	GError *error = NULL;
+	NMSettingVlan *s_vlan;
+
+	connection = connection_from_file (TEST_IFCFG_VLAN_ONLY_DEVICE,
+	                                   NULL,
+	                                   TYPE_ETHERNET,
+	                                   NULL,
+	                                   &unmanaged,
+	                                   &keyfile,
+	                                   &routefile,
+	                                   &route6file,
+	                                   &error,
+	                                   &ignore_error);
+	g_assert_no_error (error);
+	g_assert (connection != NULL);
+
+	g_free (unmanaged);
+	g_free (keyfile);
+	g_free (routefile);
+	g_free (route6file);
+
+	s_vlan = nm_connection_get_setting_vlan (connection);
+	g_assert (s_vlan);
+
+	g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, "eth0.9");
+	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth0");
+	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 9);
+
+	g_object_unref (connection);
+}
+
 static void
 test_write_vlan (void)
 {
@@ -11861,6 +11939,79 @@ test_write_vlan (void)
 	g_free (route6file);
 }
 
+static void
+test_write_vlan_only_vlanid (void)
+{
+	NMConnection *connection, *reread;
+	char *unmanaged = NULL;
+	char *keyfile = NULL;
+	char *routefile = NULL;
+	char *route6file = NULL;
+	char *written = NULL;
+	gboolean ignore_error = FALSE;
+	GError *error = NULL;
+	gboolean success = FALSE;
+
+	connection = connection_from_file (TEST_IFCFG_VLAN_ONLY_VLANID,
+	                                   NULL,
+	                                   TYPE_VLAN,
+	                                   NULL,
+	                                   &unmanaged,
+	                                   &keyfile,
+	                                   &routefile,
+	                                   &route6file,
+	                                   &error,
+	                                   &ignore_error);
+	g_assert_no_error (error);
+	g_assert (connection != NULL);
+
+	g_free (unmanaged);
+	unmanaged = NULL;
+	g_free (keyfile);
+	keyfile = NULL;
+	g_free (routefile);
+	routefile = NULL;
+	g_free (route6file);
+	route6file = NULL;
+
+	success = writer_new_connection (connection,
+	                                 TEST_SCRATCH_DIR "/network-scripts/",
+	                                 &written,
+	                                 &error);
+	g_assert (success);
+
+	/* re-read the connection for comparison */
+	reread = connection_from_file (written,
+	                               NULL,
+	                               TYPE_ETHERNET,
+	                               NULL,
+	                               &unmanaged,
+	                               &keyfile,
+	                               &routefile,
+	                               &route6file,
+	                               &error,
+	                               &ignore_error);
+	unlink (written);
+	g_free (written);
+	g_free (unmanaged);
+	g_free (keyfile);
+	g_free (routefile);
+	g_free (route6file);
+
+	g_assert_no_error (error);
+	g_assert (reread != NULL);
+
+	success = nm_connection_verify (reread, &error);
+	g_assert_no_error (error);
+	g_assert (success);
+
+	success = nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT);
+	g_assert (success);
+
+	g_object_unref (connection);
+	g_object_unref (reread);
+}
+
 #define TEST_IFCFG_BOND_MAIN TEST_IFCFG_DIR"/network-scripts/ifcfg-test-bond-main"
 
 static void
@@ -12541,6 +12692,9 @@ int main (int argc, char **argv)
 	test_read_permissions ();
 	test_read_wifi_wep_agent_keys ();
 	test_read_infiniband ();
+	test_read_vlan_interface ();
+	test_read_vlan_only_vlan_id ();
+	test_read_vlan_only_device ();
 
 	test_write_wired_static ();
 	test_write_wired_static_ip6_only ();
@@ -12605,6 +12759,8 @@ int main (int argc, char **argv)
 	test_write_permissions ();
 	test_write_wifi_wep_agent_keys ();
 	test_write_infiniband ();
+	test_write_vlan ();
+	test_write_vlan_only_vlanid ();
 
 	/* iSCSI / ibft */
 	test_read_ibft_dhcp ();

src/settings/plugins/ifcfg-rh/writer.c

@@ -1152,7 +1152,7 @@ write_vlan_setting (NMConnection *connection, shvarFile *ifcfg, GError **error)
 {
 	NMSettingVlan *s_vlan;
 	NMSettingConnection *s_con;
-	const char *master = NULL;
+	char *tmp;
 	guint32 vlan_flags = 0;
 	GString *text = NULL;
 
@@ -1170,17 +1170,13 @@ write_vlan_setting (NMConnection *connection, shvarFile *ifcfg, GError **error)
 
 	svSetValue (ifcfg, "VLAN", "yes", FALSE);
 	svSetValue (ifcfg, "TYPE", TYPE_VLAN, FALSE);
-
-	master = nm_setting_connection_get_master (s_con);
-	if (!master) {
-		g_set_error_literal (error, IFCFG_PLUGIN_ERROR, 0,
-		                     "Missing VLAN master interface name or connection UUID");
-		return FALSE;
-	}
-	svSetValue (ifcfg, "PHYSDEV", master, FALSE);
-	svSetValue (ifcfg, "MASTER", master, FALSE);
-
 	svSetValue (ifcfg, "DEVICE", nm_setting_vlan_get_interface_name (s_vlan), FALSE);
+	svSetValue (ifcfg, "PHYSDEV", nm_setting_vlan_get_parent (s_vlan), FALSE);
+	svSetValue (ifcfg, "MASTER", nm_setting_connection_get_master (s_con), FALSE);
+
+	tmp = g_strdup_printf ("%d", nm_setting_vlan_get_id (s_vlan));
+	svSetValue (ifcfg, "VLAN_ID", tmp, FALSE);
+	g_free (tmp);
 
 	vlan_flags = nm_setting_vlan_get_flags (s_vlan);
 	if (vlan_flags & NM_VLAN_FLAG_REORDER_HEADERS)
@@ -1188,27 +1184,24 @@ write_vlan_setting (NMConnection *connection, shvarFile *ifcfg, GError **error)
 	else
 		svSetValue (ifcfg, "REORDER_HDR", "0", FALSE);
 
+	svSetValue (ifcfg, "VLAN_FLAGS", NULL, FALSE);
 	if (vlan_flags & NM_VLAN_FLAG_GVRP) {
 		if (vlan_flags & NM_VLAN_FLAG_LOOSE_BINDING)
 			svSetValue (ifcfg, "VLAN_FLAGS", "GVRP,LOOSE_BINDING", FALSE);
 		else
 			svSetValue (ifcfg, "VLAN_FLAGS", "GVRP", FALSE);
-	} else {
+	} else if (vlan_flags & NM_VLAN_FLAG_LOOSE_BINDING)
-		if (vlan_flags & NM_VLAN_FLAG_LOOSE_BINDING)
+		svSetValue (ifcfg, "VLAN_FLAGS", "LOOSE_BINDING", FALSE);
-			svSetValue (ifcfg, "VLAN_FLAGS", "LOOSE_BINDING", FALSE);
-	}
 
 	text = vlan_priority_maplist_to_stringlist (s_vlan, NM_VLAN_INGRESS_MAP);
-	if (text != NULL)
+	svSetValue (ifcfg, "VLAN_INGRESS_PRIORITY_MAP", text ? text->str : NULL, FALSE);
-		svSetValue (ifcfg, "VLAN_INGRESS_PRIORITY_MAP", text->str, FALSE);
+	if (text)
-	g_string_free (text, TRUE);
+		g_string_free (text, TRUE);
-	text = NULL;
 
 	text = vlan_priority_maplist_to_stringlist (s_vlan, NM_VLAN_EGRESS_MAP);
-	if (text != NULL)
+	svSetValue (ifcfg, "VLAN_EGRESS_PRIORITY_MAP", text ? text->str : NULL, FALSE);
-		svSetValue (ifcfg, "VLAN_EGRESS_PRIORITY_MAP", text->str, FALSE);
+	if (text)
-	g_string_free (text, TRUE);
+		g_string_free (text, TRUE);
-	text = NULL;
 
 	return TRUE;
 }