Commit 8bb4c540 authored by Thomas Haller's avatar Thomas Haller

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's avatarThomas Haller <thaller@redhat.com>
parent 4f8b45e8
...@@ -798,10 +798,7 @@ _nm_connection_verify (NMConnection *connection, GError **error) ...@@ -798,10 +798,7 @@ _nm_connection_verify (NMConnection *connection, GError **error)
g_hash_table_iter_init (&iter, priv->settings); g_hash_table_iter_init (&iter, priv->settings);
while (g_hash_table_iter_next (&iter, NULL, &value)) { while (g_hash_table_iter_next (&iter, NULL, &value)) {
/* Order NMSettingConnection so that it will be verified first. /* Order NMSettingConnection so that it will be verified first.
* The reason is, that NMSettingConnection:verify() modifies the connection * The reason is, that errors in this setting might be more fundamental
* 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
* and should be checked and reported with higher priority. * and should be checked and reported with higher priority.
* Another reason is, that some settings look especially at the * Another reason is, that some settings look especially at the
* NMSettingConnection, so they find it first in the all_settings list. */ * NMSettingConnection, so they find it first in the all_settings list. */
......
...@@ -752,7 +752,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error) ...@@ -752,7 +752,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
NMSettingConnectionPrivate *priv = NM_SETTING_CONNECTION_GET_PRIVATE (setting); NMSettingConnectionPrivate *priv = NM_SETTING_CONNECTION_GET_PRIVATE (setting);
gboolean is_slave; gboolean is_slave;
const char *slave_setting_type = NULL; const char *slave_setting_type = NULL;
GSList *iter;
NMSetting *normerr_base_type = NULL; NMSetting *normerr_base_type = NULL;
const char *normerr_slave_setting_type = NULL; const char *normerr_slave_setting_type = NULL;
const char *normerr_missing_slave_type = NULL; const char *normerr_missing_slave_type = NULL;
...@@ -792,38 +791,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error) ...@@ -792,38 +791,6 @@ verify (NMSetting *setting, GSList *all_settings, GError **error)
return FALSE; 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 (priv->interface_name) {
if (!nm_utils_iface_valid_name (priv->interface_name)) { if (!nm_utils_iface_valid_name (priv->interface_name)) {
g_set_error (error, g_set_error (error,
......
...@@ -1424,7 +1424,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, ...@@ -1424,7 +1424,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
e_invalid_property, e_invalid_property,
_("property is invalid")); _("property is invalid"));
g_prefix_error (error, "%s.%s: ", setting_name, setting_property); 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; return NM_SETTING_VERIFY_SUCCESS;
} }
...@@ -1459,7 +1459,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, ...@@ -1459,7 +1459,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY,
_("property is missing")); _("property is missing"));
g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME); 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)) { if (!nm_utils_iface_valid_name (con_name)) {
/* NMSettingConnection:interface_name is invalid, we cannot normalize it. */ /* NMSettingConnection:interface_name is invalid, we cannot normalize it. */
...@@ -1477,19 +1477,17 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, ...@@ -1477,19 +1477,17 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
e_missing_property, e_missing_property,
_("property is missing")); _("property is missing"));
g_prefix_error (error, "%s.%s: ", setting_name, setting_property); 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) { if (strcmp (con_name, interface_name) != 0) {
/* con_name and interface_name are different. It can be normalized by setting interface_name /* con_name and interface_name are different. It can be normalized by setting interface_name
* to con_name. */ * to con_name. */
g_set_error_literal (error, g_set_error_literal (error,
error_quark, error_quark,
e_missing_property, e_invalid_property,
_("property is invalid")); _("property is invalid"));
g_prefix_error (error, "%s.%s: ", setting_name, setting_property); g_prefix_error (error, "%s.%s: ", setting_name, setting_property);
/* we would like to make this a NORMALIZEABLE_ERROR, but that might return NM_SETTING_VERIFY_NORMALIZABLE_ERROR;
* break older connections. */
return NM_SETTING_VERIFY_NORMALIZABLE;
} }
return NM_SETTING_VERIFY_SUCCESS; return NM_SETTING_VERIFY_SUCCESS;
......
...@@ -2562,43 +2562,27 @@ test_setting_old_uuid (void) ...@@ -2562,43 +2562,27 @@ test_setting_old_uuid (void)
g_assert (success == TRUE); 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 static void
test_connection_verify_sets_interface_name (void) test_connection_normalize_connection_interface_name (void)
{ {
NMConnection *con; NMConnection *con;
NMSettingConnection *s_con; NMSettingConnection *s_con;
NMSettingBond *s_bond; NMSettingBond *s_bond;
GError *error = NULL;
gboolean success;
s_con = (NMSettingConnection *) nm_setting_connection_new (); con = nmtst_create_minimal_connection ("test1",
g_object_set (G_OBJECT (s_con), "22001632-bbb4-4616-b277-363dce3dfb5b",
NM_SETTING_CONNECTION_ID, "test1", NM_SETTING_BOND_SETTING_NAME,
NM_SETTING_CONNECTION_UUID, "22001632-bbb4-4616-b277-363dce3dfb5b", &s_con);
NM_SETTING_CONNECTION_TYPE, NM_SETTING_BOND_SETTING_NAME,
NULL); s_bond = nm_connection_get_setting_bond (con);
s_bond = (NMSettingBond *) nm_setting_bond_new ();
g_object_set (G_OBJECT (s_bond), g_object_set (G_OBJECT (s_bond),
NM_SETTING_BOND_INTERFACE_NAME, "bond-x", NM_SETTING_BOND_INTERFACE_NAME, "bond-x",
NULL); 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); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, NULL);
/* for backward compatiblity, normalizes the interface name */ /* for backward compatiblity, normalizes the interface name */
success = nm_connection_verify (con, &error); nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
g_assert (success && !error);
g_assert_cmpstr (nm_connection_get_interface_name (con), ==, "bond-x"); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, "bond-x");
...@@ -2611,68 +2595,61 @@ test_connection_verify_sets_interface_name (void) ...@@ -2611,68 +2595,61 @@ test_connection_verify_sets_interface_name (void)
static void static void
test_connection_normalize_virtual_iface_name (void) test_connection_normalize_virtual_iface_name (void)
{ {
NMConnection *con; gs_unref_object NMConnection *con = NULL;
NMSettingConnection *s_con; NMSettingConnection *s_con;
NMSettingVlan *s_vlan; NMSettingVlan *s_vlan;
NMSetting *setting;
GError *error = NULL;
gboolean success;
const char *IFACE_NAME = "iface"; const char *IFACE_NAME = "iface";
const char *IFACE_VIRT = "iface-X"; 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 (); nm_connection_add_setting (con,
g_object_set (setting, g_object_new (NM_TYPE_SETTING_IP4_CONFIG,
NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO, NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO,
NULL); NULL));
nm_connection_add_setting (con, setting);
setting = nm_setting_ip6_config_new (); nm_connection_add_setting (con,
g_object_set (setting, g_object_new (NM_TYPE_SETTING_IP6_CONFIG,
NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_AUTO, NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO,
NM_SETTING_IP6_CONFIG_MAY_FAIL, TRUE, NULL));
NULL);
nm_connection_add_setting (con, setting); 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), g_object_set (G_OBJECT (s_vlan),
NM_SETTING_VLAN_INTERFACE_NAME, IFACE_VIRT,
NM_SETTING_VLAN_PARENT, "eth0", NM_SETTING_VLAN_PARENT, "eth0",
NULL); NULL);
nm_connection_add_setting (con, NM_SETTING (s_con)); g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, NULL);
nm_connection_add_setting (con, NM_SETTING (s_vlan)); 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_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_VIRT);
nmtst_assert_connection_verifies_after_normalization (con, NM_SETTING_VLAN_ERROR, NM_SETTING_VLAN_ERROR_INVALID_PROPERTY);
/* for backward compatiblity, normalizes the interface name */
success = nm_connection_verify (con, &error);
g_assert (success && !error);
g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); 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_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, IFACE_NAME, NULL);
g_assert (success && !error); g_object_set (G_OBJECT (s_vlan), NM_SETTING_VLAN_INTERFACE_NAME, NULL, NULL);
g_assert (modified);
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_connection_get_interface_name (con), ==, IFACE_NAME);
g_assert_cmpstr (nm_setting_vlan_get_interface_name (s_vlan), ==, 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 static void
...@@ -3142,7 +3119,7 @@ int main (int argc, char **argv) ...@@ -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", 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_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_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_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_type", test_connection_normalize_type);
g_test_add_func ("/core/general/test_connection_normalize_slave_type_1", test_connection_normalize_slave_type_1); g_test_add_func ("/core/general/test_connection_normalize_slave_type_1", test_connection_normalize_slave_type_1);
......
...@@ -1340,7 +1340,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name, ...@@ -1340,7 +1340,7 @@ _nm_setting_verify_deprecated_virtual_iface_name (const char *interface_name,
* to con_name. */ * to con_name. */
g_set_error_literal (error, g_set_error_literal (error,
error_quark, error_quark,
e_missing_property, e_invalid_property,
_("property is invalid")); _("property is invalid"));
g_prefix_error (error, "%s.%s: ", setting_name, setting_property); g_prefix_error (error, "%s.%s: ", setting_name, setting_property);
/* we would like to make this a NORMALIZEABLE_ERROR, but that might /* we would like to make this a NORMALIZEABLE_ERROR, but that might
......
...@@ -12346,8 +12346,7 @@ test_write_bridge_main (void) ...@@ -12346,8 +12346,7 @@ test_write_bridge_main (void)
NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE,
NULL); NULL);
g_assert (nm_connection_verify (connection, &error)); nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
g_assert_no_error (error);
/* Save the ifcfg */ /* Save the ifcfg */
success = writer_new_connection (connection, success = writer_new_connection (connection,
...@@ -13131,9 +13130,7 @@ test_write_bond_main (void) ...@@ -13131,9 +13130,7 @@ test_write_bond_main (void)
NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NM_SETTING_IP6_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE,
NULL); NULL);
ASSERT (nm_connection_verify (connection, &error) == TRUE, nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
"bond-main-write", "failed to verify connection: %s",
(error && error->message) ? error->message : "(unknown)");
/* Save the ifcfg */ /* Save the ifcfg */
success = writer_new_connection (connection, success = writer_new_connection (connection,
...@@ -14216,9 +14213,7 @@ test_write_team_master (void) ...@@ -14216,9 +14213,7 @@ test_write_team_master (void)
s_wired = (NMSettingWired *) nm_setting_wired_new (); s_wired = (NMSettingWired *) nm_setting_wired_new ();
nm_connection_add_setting (connection, NM_SETTING (s_wired)); nm_connection_add_setting (connection, NM_SETTING (s_wired));
success = nm_connection_verify (connection, &error); nmtst_assert_connection_verifies_after_normalization (connection, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_MISSING_PROPERTY);
g_assert_no_error (error);
g_assert (success);
/* Save the ifcfg */ /* Save the ifcfg */
success = writer_new_connection (connection, success = writer_new_connection (connection,
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment