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

diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c
index daa9c2a..7cfa05b 100644 (file)
--- a/libnm-core/nm-keyfile-reader.c
+++ b/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);

diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c
index ead1305..ccf516e 100644 (file)
--- 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 382c326..a1750c5 100644 (file)
--- 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 52676d4..c3d4bb5 100644 (file)
--- 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 d7b8bd5..55a423e 100644 (file)
--- 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 4ca4c3f..be43106 100644 (file)
--- 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 (file)
index 0000000..330adda
--- /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 375f0d8..ede40ee 100644 (file)
--- 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);