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

8 files changed, 86 insertions, 16 deletions

diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c
index daa9c2adbf..7cfa05bdfd 100644
--- a/libnm-core/nm-keyfile-reader.c
+++ b/libnm-core/nm-keyfile-reader.c
@@ -1249,6 +1249,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,
@@ -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);
diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c
index ead1305534..ccf516ea12 100644
--- a/libnm-core/nm-keyfile-writer.c
+++ b/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;
 		}
diff --git a/libnm-core/nm-setting-vlan.c b/libnm-core/nm-setting-vlan.c
index 382c32606e..a1750c52cc 100644
--- a/libnm-core/nm-setting-vlan.c
+++ b/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 |
diff --git a/libnm-util/nm-setting-vlan.c b/libnm-util/nm-setting-vlan.c
index 52676d47f0..c3d4bb58f1 100644
--- a/libnm-util/nm-setting-vlan.c
+++ b/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 |
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 d7b8bd57aa..55a423e13d 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/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);
 }
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 375f0d8383..ede40ee1cc 100644
--- a/src/settings/plugins/keyfile/tests/test-keyfile.c
+++ b/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);