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

(cherry picked from commit 687b6515980c08cdbb9734bd112a594166c4d6dd)

8 files changed, 86 insertions, 15 deletions

diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c
index 353804f97c..9d8c873482 100644
--- a/libnm-core/nm-keyfile-reader.c
+++ b/libnm-core/nm-keyfile-reader.c
@@ -1253,6 +1253,15 @@ static KeyParser key_parsers[] = {
 };
 
 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,
                         const GValue *value,
@@ -1316,6 +1325,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;
 	}
 
@@ -1651,6 +1663,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);
diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c
index ff433b264a..1dac5c3277 100644
--- a/libnm-core/nm-keyfile-writer.c
+++ b/libnm-core/nm-keyfile-writer.c
@@ -30,18 +30,7 @@
 #include <string.h>
 #include <glib/gi18n-lib.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 "gsystem-local-alloc.h"
 #include "nm-glib-compat.h"
 #include "nm-keyfile-internal.h"
@@ -598,6 +587,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,
@@ -628,7 +626,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;
 		}
diff --git a/libnm-core/nm-setting-vlan.c b/libnm-core/nm-setting-vlan.c
index cbc85b3e45..0547141c59 100644
--- a/libnm-core/nm-setting-vlan.c
+++ b/libnm-core/nm-setting-vlan.c
@@ -749,7 +749,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 |
diff --git a/libnm-util/nm-setting-vlan.c b/libnm-util/nm-setting-vlan.c
index 43b6a91c13..d3bc0a3796 100644
--- a/libnm-util/nm-setting-vlan.c
+++ b/libnm-util/nm-setting-vlan.c
@@ -806,7 +806,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 |
diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
index f42dbf4dc2..39a0abaf79 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -10889,6 +10889,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);
 }
diff --git a/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am b/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am
index 4ca4c3faa5..be431067f0 100644
--- a/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am
+++ b/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
diff --git a/src/settings/plugins/keyfile/tests/keyfiles/Test_Missing_Vlan_Flags b/src/settings/plugins/keyfile/tests/keyfiles/Test_Missing_Vlan_Flags
new file mode 100644
index 0000000000..330adda0f5
--- /dev/null
+++ b/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
diff --git a/src/settings/plugins/keyfile/tests/test-keyfile.c b/src/settings/plugins/keyfile/tests/test-keyfile.c
index 4fc44c7bd2..785ad4cb68 100644
--- a/src/settings/plugins/keyfile/tests/test-keyfile.c
+++ b/src/settings/plugins/keyfile/tests/test-keyfile.c
@@ -3292,6 +3292,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);
 }
@@ -3688,6 +3717,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);