diff options
author | Thomas Haller <thaller@redhat.com> | 2014-08-13 15:02:30 +0200 |
---|---|---|
committer | Thomas Haller <thaller@redhat.com> | 2014-08-22 15:24:31 +0200 |
commit | 8bb4c540901abe20d5fafc5ade90158cd5c965e1 (patch) | |
tree | a816608d0603808051d5320d8f59f98e12e17028 | |
parent | 4f8b45e802e49f5fe88ca99e2f5fe72e85287893 (diff) | |
download | NetworkManager-8bb4c540901abe20d5fafc5ade90158cd5c965e1.tar.gz |
libnm-core: fix NMSettingConnection:verify() not to modify interface-name
verify() used to modify interface-name of the base settings. This is
discouraged, because verify() should not touch the connection.
For libnm-core we can change behavior and only modify the connection
in normalize().
Also, be more strict not to verify() sucessfully on invalid
interface-name.
Signed-off-by: Thomas Haller <thaller@redhat.com>
-rw-r--r-- | libnm-core/nm-connection.c | 5 | ||||
-rw-r--r-- | libnm-core/nm-setting-connection.c | 33 | ||||
-rw-r--r-- | libnm-core/nm-setting.c | 12 | ||||
-rw-r--r-- | libnm-core/tests/test-general.c | 107 | ||||
-rw-r--r-- | libnm-util/nm-setting.c | 2 | ||||
-rw-r--r-- | src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 11 |
6 files changed, 52 insertions, 118 deletions
diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index 8ff956e298..8a7aa10cb8 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -798,10 +798,7 @@ _nm_connection_verify (NMConnection *connection, GError **error) g_hash_table_iter_init (&iter, priv->settings); while (g_hash_table_iter_next (&iter, NULL, &value)) { /* Order NMSettingConnection so that it will be verified first. - * The reason is, that NMSettingConnection:verify() modifies the connection - * by setting NMSettingConnection:interface_name. So we want to call that - * verify() first, because the order can affect the outcome. - * Another reason is, that errors in this setting might be more fundamental + * The reason is, that errors in this setting might be more fundamental * and should be checked and reported with higher priority. * Another reason is, that some settings look especially at the * NMSettingConnection, so they find it first in the all_settings list. */ diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index 28e5adf7de..b605d8289c 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -752,7 +752,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error) NMSettingConnectionPrivate *priv = NM_SETTING_CONNECTION_GET_PRIVATE (setting); gboolean is_slave; const char *slave_setting_type = NULL; - GSList *iter; NMSetting *normerr_base_type = NULL; const char *normerr_slave_setting_type = NULL; const char *normerr_missing_slave_type = NULL; @@ -792,38 +791,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error) return FALSE; } - /* FIXME: previously, verify() set the NMSettingConnection:interface_name property, - * thus modifying the setting. verify() should not do this, but keep this not to change - * behaviour. - */ - if (!priv->interface_name) { - for (iter = all_settings; iter; iter = iter->next) { - NMSetting *s_current = iter->data; - char *virtual_iface_name = NULL; - - if (NM_IS_SETTING_BOND (s_current)) - g_object_get (s_current, NM_SETTING_BOND_INTERFACE_NAME, &virtual_iface_name, NULL); - else if (NM_IS_SETTING_BRIDGE (s_current)) - g_object_get (s_current, NM_SETTING_BRIDGE_INTERFACE_NAME, &virtual_iface_name, NULL); - else if (NM_IS_SETTING_TEAM (s_current)) - g_object_get (s_current, NM_SETTING_TEAM_INTERFACE_NAME, &virtual_iface_name, NULL); - else if (NM_IS_SETTING_VLAN (s_current)) - g_object_get (s_current, NM_SETTING_VLAN_INTERFACE_NAME, &virtual_iface_name, NULL); - /* For NMSettingInfiniband, virtual_iface_name has no backing field. - * No need to set the (unset) interface_name to the default value. - **/ - - if (virtual_iface_name) { - if (nm_utils_iface_valid_name (virtual_iface_name)) { - /* found a new interface name. */ - priv->interface_name = virtual_iface_name; - break; - } - g_free (virtual_iface_name); - } - } - } - if (priv->interface_name) { if (!nm_utils_iface_valid_name (priv->interface_name)) { g_set_error (error, diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index e6305f6ecb..a29b6678ea 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1424,7 +1424,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, e_invalid_property, _("property is invalid")); g_prefix_error (error, "%s.%s: ", setting_name, setting_property); - return NM_SETTING_VERIFY_NORMALIZABLE; + return NM_SETTING_VERIFY_NORMALIZABLE_ERROR; } return NM_SETTING_VERIFY_SUCCESS; } @@ -1459,7 +1459,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY, _("property is missing")); g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME); - return NM_SETTING_VERIFY_NORMALIZABLE; + return NM_SETTING_VERIFY_NORMALIZABLE_ERROR; } if (!nm_utils_iface_valid_name (con_name)) { /* NMSettingConnection:interface_name is invalid, we cannot normalize it. */ @@ -1477,19 +1477,17 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, e_missing_property, _("property is missing")); g_prefix_error (error, "%s.%s: ", setting_name, setting_property); - return NM_SETTING_VERIFY_NORMALIZABLE; + return NM_SETTING_VERIFY_NORMALIZABLE_ERROR; } if (strcmp (con_name, interface_name) != 0) { /* con_name and interface_name are different. It can be normalized by setting interface_name * to con_name. */ g_set_error_literal (error, error_quark, - e_missing_property, + e_invalid_property, _("property is invalid")); g_prefix_error (error, "%s.%s: ", setting_name, setting_property); - /* we would like to make this a NORMALIZEABLE_ERROR, but that might - * break older connections. */ - return NM_SETTING_VERIFY_NORMALIZABLE; + return NM_SETTING_VERIFY_NORMALIZABLE_ERROR; } return NM_SETTING_VERIFY_SUCCESS; diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 8e01677294..2d26440b91 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -2562,43 +2562,27 @@ test_setting_old_uuid (void) g_assert (success == TRUE); } -/* - * nm_connection_verify() modifies the connection by setting - * the interface-name property to the virtual_iface_name of - * the type specific settings. - * - * It would be preferable of verify() not to touch the connection, - * but as it is now, stick with it and test it. - **/ static void -test_connection_verify_sets_interface_name (void) +test_connection_normalize_connection_interface_name (void) { NMConnection *con; NMSettingConnection *s_con; NMSettingBond *s_bond; - GError *error = NULL; - gboolean success; - s_con = (NMSettingConnection *) nm_setting_connection_new (); - g_object_set (G_OBJECT (s_con), - NM_SETTING_CONNECTION_ID, "test1", - NM_SETTING_CONNECTION_UUID, "22001632-bbb4-4616-b277-363dce3dfb5b", - NM_SETTING_CONNECTION_TYPE, NM_SETTING_BOND_SETTING_NAME, - NULL); - s_bond = (NMSettingBond *) nm_setting_bond_new (); + con = nmtst_create_minimal_connection ("test1", + "22001632-bbb4-4616-b277-363dce3dfb5b", + NM_SETTING_BOND_SETTING_NAME, + &s_con); + + s_bond = nm_connection_get_setting_bond (con); g_object_set (G_OBJECT (s_bond), NM_SETTING_BOND_INTERFACE_NAME, "bond-x", NULL); - con = nm_simple_connection_new (); - nm_connection_add_setting (con, NM_SETTING (s_con)); - nm_connection_add_setting (con, NM_SETTING (s_bond)); - g_assert_cmpstr (nm_connection_get_interface_name (con), ==, NULL); /* for backward compatiblity, normalizes the interface name */ - success = nm_connection_verify (con, &error); - g_assert (success && !error); + nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, "bond-x"); @@ -2611,68 +2595,61 @@ test_connection_verify_sets_interface_name (void) static void test_connection_normalize_virtual_iface_name (void) { - NMConnection *con; + gs_unref_object NMConnection *con = NULL; NMSettingConnection *s_con; NMSettingVlan *s_vlan; - NMSetting *setting; - GError *error = NULL; - gboolean success; const char *IFACE_NAME = "iface"; const char *IFACE_VIRT = "iface-X"; - gboolean modified = FALSE; - con = nm_simple_connection_new (); + con = nmtst_create_minimal_connection ("test1", + "22001632-bbb4-4616-b277-363dce3dfb5b", + NM_SETTING_VLAN_SETTING_NAME, + &s_con); - setting = nm_setting_ip4_config_new (); - g_object_set (setting, - NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO, - NULL); - nm_connection_add_setting (con, setting); + nm_connection_add_setting (con, + g_object_new (NM_TYPE_SETTING_IP4_CONFIG, + NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO, + NULL)); - setting = nm_setting_ip6_config_new (); - g_object_set (setting, - NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_AUTO, - NM_SETTING_IP6_CONFIG_MAY_FAIL, TRUE, - NULL); - nm_connection_add_setting (con, setting); + nm_connection_add_setting (con, + g_object_new (NM_TYPE_SETTING_IP6_CONFIG, + NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO, + NULL)); + + s_vlan = nm_connection_get_setting_vlan (con); - s_con = (NMSettingConnection *) nm_setting_connection_new (); - g_object_set (G_OBJECT (s_con), - NM_SETTING_CONNECTION_ID, "test1", - NM_SETTING_CONNECTION_UUID, "22001632-bbb4-4616-b277-363dce3dfb5b", - NM_SETTING_CONNECTION_TYPE, NM_SETTING_VLAN_SETTING_NAME, - NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, - NULL); - s_vlan = (NMSettingVlan *) nm_setting_vlan_new (); g_object_set (G_OBJECT (s_vlan), - NM_SETTING_VLAN_INTERFACE_NAME, IFACE_VIRT, NM_SETTING_VLAN_PARENT, "eth0", NULL); - nm_connection_add_setting (con, NM_SETTING (s_con)); - nm_connection_add_setting (con, NM_SETTING (s_vlan)); + g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, NULL); + g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, IFACE_VIRT, NULL); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_VIRT); - - /* for backward compatiblity, normalizes the interface name */ - success = nm_connection_verify (con, &error); - g_assert (success && !error); - + nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_VLAN_ERROR, NM_SETTING_VLAN_ERROR_INVALID_PROPERTY); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); - g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_VIRT); + g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME); + - success = nm_connection_normalize (con, NULL, &modified, &error); - g_assert (success && !error); - g_assert (modified); + g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, NULL); + g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, NULL, NULL); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); + g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, NULL); + nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_VLAN_ERROR, NM_SETTING_VLAN_ERROR_MISSING_PROPERTY); + g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME); - success = nm_connection_verify (con, &error); - g_assert (success && !error); - g_object_unref (con); + g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, NULL, NULL); + g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, IFACE_NAME, NULL); + + g_assert_cmpstr (nm_connection_get_interface_name (con), ==, NULL); + g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME); + nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY); + g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); + g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, IFACE_NAME); } static void @@ -3142,7 +3119,7 @@ int main (int argc, char **argv) g_test_add_func ("/core/general/test_connection_replace_settings", test_connection_replace_settings); g_test_add_func ("/core/general/test_connection_replace_settings_from_connection", test_connection_replace_settings_from_connection); g_test_add_func ("/core/general/test_connection_new_from_hash", test_connection_new_from_hash); - g_test_add_func ("/core/general/test_connection_verify_sets_interface_name", test_connection_verify_sets_interface_name); + g_test_add_func ("/core/general/test_connection_normalize_connection_interface_name", test_connection_normalize_connection_interface_name); g_test_add_func ("/core/general/test_connection_normalize_virtual_iface_name", test_connection_normalize_virtual_iface_name); g_test_add_func ("/core/general/test_connection_normalize_type", test_connection_normalize_type); g_test_add_func ("/core/general/test_connection_normalize_slave_type_1", test_connection_normalize_slave_type_1); diff --git a/libnm-util/nm-setting.c b/libnm-util/nm-setting.c index b479bf4aa0..b943369e2e 100644 --- a/libnm-util/nm-setting.c +++ b/libnm-util/nm-setting.c @@ -1340,7 +1340,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, * to con_name. */ g_set_error_literal (error, error_quark, - e_missing_property, + e_invalid_property, _("property is invalid")); g_prefix_error (error, "%s.%s: ", setting_name, setting_property); /* we would like to make this a NORMALIZEABLE_ERROR, but that might 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 d917159efe..f835064ce2 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -12346,8 +12346,7 @@ test_write_bridge_main (void) NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); - g_assert (nm_connection_verify (connection, &error)); - g_assert_no_error (error); + nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY); /* Save the ifcfg */ success = writer_new_connection (connection, @@ -13131,9 +13130,7 @@ test_write_bond_main (void) NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); - ASSERT (nm_connection_verify (connection, &error) == TRUE, - "bond-main-write", "failed to verify connection: %s", - (error && error->message) ? error->message : "(unknown)"); + nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY); /* Save the ifcfg */ success = writer_new_connection (connection, @@ -14216,9 +14213,7 @@ test_write_team_master (void) s_wired = (NMSettingWired *) nm_setting_wired_new (); nm_connection_add_setting (connection, NM_SETTING (s_wired)); - success = nm_connection_verify (connection, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY); /* Save the ifcfg */ success = writer_new_connection (connection, |