libnm/vlan: default to vlan.flags=REORDER_HDR for new connections (rh #1250225)

The kernel defaults REORDER_HDR to 1 when creating a new VLAN, but
NetworkManager's VLAN flags property defaulted to 0. Thus REORDER_HDR was not
set for NM-created VLANs with default values.

We want to match the kernel default, so we change the default value for the
vlan.flags property. However, we do not want to change the flags for existing
connections if the property is missing in connection files. Thus we have to
update plugins for that. We also make sure that vlan.flags is always written
by 'keyfile' when the value is default. That way new connections have flags
property explicitly written and it will be loaded as expected.

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

libnm-core/nm-keyfile-reader.c

@@ -1248,6 +1248,15 @@ static KeyParser key_parsers[] = {
 	{ NULL, NULL, FALSE }
 };
 
+static void
+set_default_for_missing_key (NMSetting *setting, const char *property)
+{
+	/* Set a value different from the default value of the property's spec */
+
+	if (NM_IS_SETTING_VLAN (setting) && !strcmp (property, NM_SETTING_VLAN_FLAGS))
+		g_object_set (setting, property, 0, NULL);
+}
+
 static void
 read_one_setting_value (NMSetting *setting,
                         const char *key,
@@ -1312,6 +1321,9 @@ read_one_setting_value (NMSetting *setting,
 			                  err->message))
 				goto out_error;
 		}
+
+		/* Allow default values different than in property spec */
+		set_default_for_missing_key (setting, key);
 		return;
 	}
 
@@ -1647,6 +1659,18 @@ nm_keyfile_read (GKeyFile *keyfile,
 		}
 	}
 
+	/* Make sure that if [vlan] group was missing we set vlan.flags to 0
+	 * for backwards compatibility */
+	if (nm_connection_is_type (connection, NM_SETTING_VLAN_SETTING_NAME)) {
+		if (!nm_connection_get_setting_vlan (connection)) {
+			NMSettingVlan *s_vlan;
+
+			s_vlan = NM_SETTING_VLAN (nm_setting_vlan_new ());
+			g_object_set (s_vlan, NM_SETTING_VLAN_FLAGS, 0, NULL);
+			nm_connection_add_setting (connection, NM_SETTING (s_vlan));
+		}
+	}
+
 	return connection;
 out_error:
 	g_propagate_error (error, info.error);

libnm-core/nm-keyfile-writer.c

@@ -29,19 +29,7 @@
 #include <arpa/inet.h>
 #include <string.h>
 
-#include "nm-default.h"
-#include "nm-setting.h"
-#include "nm-setting-connection.h"
-#include "nm-setting-ip4-config.h"
-#include "nm-setting-ip6-config.h"
-#include "nm-setting-vpn.h"
-#include "nm-setting-wired.h"
-#include "nm-setting-wireless.h"
-#include "nm-setting-ip4-config.h"
-#include "nm-setting-bluetooth.h"
-#include "nm-setting-8021x.h"
-#include "nm-utils.h"
-
+#include "nm-core-internal.h"
 #include "nm-keyfile-internal.h"
 #include "nm-keyfile-utils.h"
 
@@ -596,6 +584,15 @@ static KeyWriter key_writers[] = {
 	{ NULL, NULL, NULL }
 };
 
+static gboolean
+can_omit_default_value (NMSetting *setting, const char *property)
+{
+	if (NM_IS_SETTING_VLAN (setting) && !strcmp (property, NM_SETTING_VLAN_FLAGS))
+		return FALSE;
+
+	return TRUE;
+}
+
 static void
 write_setting_value (NMSetting *setting,
                      const char *key,
@@ -626,7 +623,8 @@ write_setting_value (NMSetting *setting,
 	/* If the value is the default value, remove the item from the keyfile */
 	pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (setting), key);
 	if (pspec) {
-		if (g_param_value_defaults (pspec, (GValue *) value)) {
+		if (   can_omit_default_value (setting, key)
+		    && g_param_value_defaults (pspec, (GValue *) value)) {
 			g_key_file_remove_key (info->keyfile, setting_name, key, NULL);
 			return;
 		}

libnm-core/nm-setting-vlan.c

@@ -748,7 +748,7 @@ nm_setting_vlan_class_init (NMSettingVlanClass *setting_class)
 		(object_class, PROP_FLAGS,
 		 g_param_spec_flags (NM_SETTING_VLAN_FLAGS, "", "",
 		                     NM_TYPE_VLAN_FLAGS,
-		                     0,
+		                     NM_VLAN_FLAG_REORDER_HEADERS,
 		                     G_PARAM_READWRITE |
 		                     G_PARAM_CONSTRUCT |
 		                     NM_SETTING_PARAM_INFERRABLE |

libnm-util/nm-setting-vlan.c

@@ -805,7 +805,7 @@ nm_setting_vlan_class_init (NMSettingVlanClass *setting_class)
 	g_object_class_install_property
 		(object_class, PROP_FLAGS,
 		 g_param_spec_uint (NM_SETTING_VLAN_FLAGS, "", "",
-		                    0, G_MAXUINT32, 0,
+		                    0, G_MAXUINT32, NM_VLAN_FLAG_REORDER_HEADERS,
 		                    G_PARAM_READWRITE |
 		                    G_PARAM_CONSTRUCT |
 		                    NM_SETTING_PARAM_INFERRABLE |

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

@@ -11044,6 +11044,8 @@ test_read_vlan_only_vlan_id (void)
 
 	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "eth9");
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 43);
+	/* Ensure that flags are 0 if both REORDER_HDR and VLAN_FLAGS are missing */
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 0);
 
 	g_object_unref (connection);
 }

src/settings/plugins/keyfile/tests/keyfiles/Makefile.am

@@ -28,6 +28,7 @@ KEYFILES = \
 	Test_minimal_slave_3 \
 	Test_minimal_slave_4 \
 	Test_Missing_Vlan_Setting \
+	Test_Missing_Vlan_Flags \
 	Test_Missing_ID_UUID \
 	Test_Enum_Property \
 	Test_Flags_Property

src/settings/plugins/keyfile/tests/keyfiles/Test_Missing_Vlan_Flags

@@ -0,0 +1,15 @@
+# VLAN setting with missing 'flags' key
+# vlan.flags will be set to 0 (even if the default 'flags' property value is 1)
+
+[connection]
+id=Test Missing Vlan Flags
+uuid=803ebe47-8c31-401d-b47b-03fc0d34eb11
+type=vlan
+autoconnect=true
+
+[802-3-ethernet]
+mac-address=00:11:22:33:44:55
+
+[vlan]
+id=444
+parent=em1

src/settings/plugins/keyfile/tests/test-keyfile.c

@@ -3293,6 +3293,35 @@ test_read_missing_vlan_setting (void)
 	s_vlan = nm_connection_get_setting_vlan (connection);
 	g_assert (s_vlan);
 	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 0);
+	/* Ensure the VLAN flags are not set (0) */
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 0);
+
+	g_object_unref (connection);
+}
+
+static void
+test_read_missing_vlan_flags (void)
+{
+	NMConnection *connection;
+	NMSettingVlan *s_vlan;
+	GError *error = NULL;
+	gboolean success;
+
+	connection = nm_keyfile_plugin_connection_from_file (TEST_KEYFILES_DIR"/Test_Missing_Vlan_Flags", &error);
+	g_assert_no_error (error);
+	g_assert (connection);
+	success = nm_connection_verify (connection, &error);
+	g_assert_no_error (error);
+	g_assert (success);
+
+	/* Ensure the VLAN setting exists */
+	s_vlan = nm_connection_get_setting_vlan (connection);
+	g_assert (s_vlan);
+
+	g_assert_cmpint (nm_setting_vlan_get_id (s_vlan), ==, 444);
+	g_assert_cmpstr (nm_setting_vlan_get_parent (s_vlan), ==, "em1");
+	/* Ensure the VLAN flags are not set (0) */
+	g_assert_cmpint (nm_setting_vlan_get_flags (s_vlan), ==, 0);
 
 	g_object_unref (connection);
 }
@@ -3689,6 +3718,7 @@ int main (int argc, char **argv)
 	g_test_add_func ("/keyfile/test_write_new_wireless_group_names ", test_write_new_wireless_group_names);
 
 	g_test_add_func ("/keyfile/test_read_missing_vlan_setting ", test_read_missing_vlan_setting);
+	g_test_add_func ("/keyfile/test_read_missing_vlan_flags ", test_read_missing_vlan_flags);
 	g_test_add_func ("/keyfile/test_read_missing_id_uuid ", test_read_missing_id_uuid);
 
 	g_test_add_func ("/keyfile/test_read_minimal", test_read_minimal);