mirror of
https://gitlab.freedesktop.org/NetworkManager/NetworkManager
synced 2024-10-07 00:31:11 +00:00
libnm-core: fix up connection deserialize/copy/replace semantics
libnm-util's connection deserializing/copying/replacing functions have odd semantics where sometimes they both modify a connection AND return an error. libnm-core tried to improve things by guaranteeing that the connection would not be modified if the new settings were invalid, but this ended up breaking a bunch of places that needed to be able to work with invalid connections. So re-fix the functions by reverting back to the old semantics, but having return values that clearly distinguish whether the connection was modified or not. For comparison: - nm_connection_new_from_hash() / nm_simple_connection_new_from_dbus(): - libnm-util: returns a valid connection or NULL. - OLD libnm-core: returned a valid connection or NULL. - NEW libnm-core: returns a valid connection or NULL. - nm_connection_duplicate() / nm_simple_connection_new_clone(): - libnm-util: always succeeds, whether or not the connection is valid. - OLD libnm-core: returned a valid connection or NULL - NEW libnm-core: always succeeds, whether or not the connection is valid. - nm_connection_replace_settings_from_connection(): - libnm-util: always replaces the settings, but returns FALSE if the connection is now invalid. - OLD libnm-core: either replaced the settings and returned TRUE (if the settings were valid), or else left the connection unchanged and returned FALSE (if not). - NEW libnm-core: always replaces the settings, and has no return value. (The modified connection is valid if and only if the replaced-from connection was valid; just like with the libnm-util version.) - nm_connection_replace_settings(): - libnm-util: returns TRUE if the new settings are valid, or FALSE if either (a) the new settings could not be deserialized and the connection is unchanged, or (b) the new settings were deserialized, and the connection was updated, but is now not valid. - OLD libnm-core: either replaced the settings and returned TRUE (if the settings were valid), or else left the connection unchanged and returned FALSE (if not). - NEW libnm-core: returns TRUE if the connection was updated (whether or not it is valid), or FALSE if the new settings could not be deserialized and the connection is unchanged.
This commit is contained in:
parent
c47165081a
commit
ad8b13091b
|
@ -7377,8 +7377,7 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
|
|||
} else {
|
||||
/* Save/update already saved (existing) connection */
|
||||
nm_connection_replace_settings_from_connection (NM_CONNECTION (rem_con),
|
||||
connection,
|
||||
NULL);
|
||||
connection);
|
||||
update_connection (persistent, rem_con, update_connection_editor_cb, NULL);
|
||||
}
|
||||
|
||||
|
@ -7417,8 +7416,7 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
|
|||
|
||||
/* Update settings in the local connection */
|
||||
nm_connection_replace_settings_from_connection (connection,
|
||||
NM_CONNECTION (con_tmp),
|
||||
NULL);
|
||||
NM_CONNECTION (con_tmp));
|
||||
|
||||
/* Also update setting for menu context and TAB-completion */
|
||||
menu_ctx.curr_setting = s_name ? nm_connection_get_setting_by_name (connection, s_name) : NULL;
|
||||
|
|
|
@ -128,13 +128,8 @@ save_connection_and_exit (NmtNewtButton *button,
|
|||
NmtSyncOp op;
|
||||
GError *error = NULL;
|
||||
|
||||
if (!nm_connection_replace_settings_from_connection (priv->orig_connection,
|
||||
priv->edit_connection,
|
||||
&error)) {
|
||||
nmt_newt_message_dialog (_("Error saving connection: %s"), error->message);
|
||||
g_error_free (error);
|
||||
return;
|
||||
}
|
||||
nm_connection_replace_settings_from_connection (priv->orig_connection,
|
||||
priv->edit_connection);
|
||||
|
||||
nmt_sync_op_init (&op);
|
||||
if (NM_IS_REMOTE_CONNECTION (priv->orig_connection)) {
|
||||
|
|
|
@ -274,32 +274,68 @@ validate_permissions_type (GHashTable *hash, GError **error)
|
|||
* @new_settings: (element-type utf8 GLib.HashTable): a #GHashTable of settings
|
||||
* @error: location to store error, or %NULL
|
||||
*
|
||||
* Returns: %TRUE if the settings were valid and added to the connection, %FALSE
|
||||
* if they were not (in which case @connection will be unchanged).
|
||||
* Replaces @connection's settings with @new_settings (which must be
|
||||
* syntactically valid, and describe a known type of connection, but does not
|
||||
* need to result in a connection that passes nm_connection_verify()).
|
||||
*
|
||||
* Returns: %TRUE if connection was updated, %FALSE if @new_settings could not
|
||||
* be deserialized (in which case @connection will be unchanged).
|
||||
**/
|
||||
gboolean
|
||||
nm_connection_replace_settings (NMConnection *connection,
|
||||
GHashTable *new_settings,
|
||||
GError **error)
|
||||
{
|
||||
NMConnection *new;
|
||||
gboolean valid;
|
||||
NMConnectionPrivate *priv;
|
||||
GHashTableIter iter;
|
||||
const char *setting_name;
|
||||
GHashTable *setting_hash;
|
||||
GSList *settings = NULL, *s;
|
||||
gboolean changed;
|
||||
|
||||
g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE);
|
||||
g_return_val_if_fail (new_settings != NULL, FALSE);
|
||||
g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
|
||||
|
||||
priv = NM_CONNECTION_GET_PRIVATE (connection);
|
||||
|
||||
if (!validate_permissions_type (new_settings, error))
|
||||
return FALSE;
|
||||
|
||||
new = nm_simple_connection_new_from_dbus (new_settings, error);
|
||||
if (!new)
|
||||
return FALSE;
|
||||
g_hash_table_iter_init (&iter, new_settings);
|
||||
while (g_hash_table_iter_next (&iter, (gpointer) &setting_name, (gpointer) &setting_hash)) {
|
||||
NMSetting *setting;
|
||||
GType type;
|
||||
|
||||
valid = nm_connection_replace_settings_from_connection (connection, new, error);
|
||||
g_object_unref (new);
|
||||
type = nm_setting_lookup_type (setting_name);
|
||||
if (type == G_TYPE_INVALID) {
|
||||
g_set_error (error,
|
||||
NM_CONNECTION_ERROR,
|
||||
NM_CONNECTION_ERROR_INVALID_SETTING,
|
||||
"unknown setting name '%s'", setting_name);
|
||||
g_slist_free_full (settings, g_object_unref);
|
||||
return FALSE;
|
||||
}
|
||||
|
||||
g_return_val_if_fail (valid == TRUE, FALSE);
|
||||
setting = _nm_setting_new_from_dbus (type, setting_hash, new_settings, error);
|
||||
if (!setting) {
|
||||
g_slist_free_full (settings, g_object_unref);
|
||||
return FALSE;
|
||||
}
|
||||
settings = g_slist_prepend (settings, setting);
|
||||
}
|
||||
|
||||
if (g_hash_table_size (priv->settings) > 0) {
|
||||
g_hash_table_foreach_remove (priv->settings, _setting_release, connection);
|
||||
changed = TRUE;
|
||||
} else
|
||||
changed = (settings != NULL);
|
||||
|
||||
for (s = settings; s; s = s->next)
|
||||
nm_connection_add_setting (connection, s->data);
|
||||
|
||||
if (changed)
|
||||
g_signal_emit (connection, signals[CHANGED], 0);
|
||||
return TRUE;
|
||||
}
|
||||
|
||||
|
@ -307,35 +343,27 @@ nm_connection_replace_settings (NMConnection *connection,
|
|||
* nm_connection_replace_settings_from_connection:
|
||||
* @connection: a #NMConnection
|
||||
* @new_connection: a #NMConnection to replace the settings of @connection with
|
||||
* @error: location to store error, or %NULL
|
||||
*
|
||||
* Deep-copies the settings of @new_conenction and replaces the settings of @connection
|
||||
* Deep-copies the settings of @new_connection and replaces the settings of @connection
|
||||
* with the copied settings.
|
||||
*
|
||||
* Returns: %TRUE if the settings were valid and added to the connection, %FALSE
|
||||
* if they were not (in which case @connection will be unchanged).
|
||||
**/
|
||||
gboolean
|
||||
void
|
||||
nm_connection_replace_settings_from_connection (NMConnection *connection,
|
||||
NMConnection *new_connection,
|
||||
GError **error)
|
||||
NMConnection *new_connection)
|
||||
{
|
||||
NMConnectionPrivate *priv, *new_priv;
|
||||
GHashTableIter iter;
|
||||
NMSetting *setting;
|
||||
gboolean changed;
|
||||
|
||||
g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE);
|
||||
g_return_val_if_fail (NM_IS_CONNECTION (new_connection), FALSE);
|
||||
g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
|
||||
|
||||
if (!nm_connection_verify (new_connection, error))
|
||||
return FALSE;
|
||||
g_return_if_fail (NM_IS_CONNECTION (connection));
|
||||
g_return_if_fail (NM_IS_CONNECTION (new_connection));
|
||||
|
||||
/* When 'connection' and 'new_connection' are the same object simply return
|
||||
* in order not to destroy 'connection' */
|
||||
* in order not to destroy 'connection'.
|
||||
*/
|
||||
if (connection == new_connection)
|
||||
return TRUE;
|
||||
return;
|
||||
|
||||
/* No need to validate permissions like nm_connection_replace_settings()
|
||||
* since we're dealing with an NMConnection which has already done that.
|
||||
|
@ -354,12 +382,8 @@ nm_connection_replace_settings_from_connection (NMConnection *connection,
|
|||
changed = TRUE;
|
||||
}
|
||||
|
||||
/* Since new_connection verified before, this shouldn't ever fail */
|
||||
g_return_val_if_fail (nm_connection_verify (connection, error), FALSE);
|
||||
|
||||
if (changed)
|
||||
g_signal_emit (connection, signals[CHANGED], 0);
|
||||
return TRUE;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -162,9 +162,8 @@ gboolean nm_connection_replace_settings (NMConnection *connection,
|
|||
GHashTable *new_settings,
|
||||
GError **error);
|
||||
|
||||
gboolean nm_connection_replace_settings_from_connection (NMConnection *connection,
|
||||
NMConnection *new_connection,
|
||||
GError **error);
|
||||
void nm_connection_replace_settings_from_connection (NMConnection *connection,
|
||||
NMConnection *new_connection);
|
||||
|
||||
void nm_connection_clear_settings (NMConnection *connection);
|
||||
|
||||
|
|
|
@ -66,40 +66,14 @@ NMConnection *
|
|||
nm_simple_connection_new_from_dbus (GHashTable *hash, GError **error)
|
||||
{
|
||||
NMConnection *connection;
|
||||
GHashTableIter iter;
|
||||
const char *setting_name;
|
||||
GHashTable *setting_hash;
|
||||
|
||||
g_return_val_if_fail (hash != NULL, NULL);
|
||||
|
||||
connection = nm_simple_connection_new ();
|
||||
|
||||
g_hash_table_iter_init (&iter, hash);
|
||||
while (g_hash_table_iter_next (&iter, (gpointer) &setting_name, (gpointer) &setting_hash)) {
|
||||
NMSetting *setting;
|
||||
GType type;
|
||||
|
||||
type = nm_setting_lookup_type (setting_name);
|
||||
if (type == G_TYPE_INVALID) {
|
||||
g_set_error (error,
|
||||
NM_CONNECTION_ERROR,
|
||||
NM_CONNECTION_ERROR_INVALID_SETTING,
|
||||
"unknown setting name '%s'", setting_name);
|
||||
goto failed;
|
||||
}
|
||||
|
||||
setting = _nm_setting_new_from_dbus (type, setting_hash, hash, error);
|
||||
if (!setting)
|
||||
goto failed;
|
||||
nm_connection_add_setting (connection, setting);
|
||||
}
|
||||
|
||||
if (nm_connection_verify (connection, error))
|
||||
return connection;
|
||||
|
||||
failed:
|
||||
g_object_unref (connection);
|
||||
return NULL;
|
||||
if ( !nm_connection_replace_settings (connection, hash, error)
|
||||
|| !nm_connection_verify (connection, error))
|
||||
g_clear_object (&connection);
|
||||
return connection;
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -120,7 +94,7 @@ nm_simple_connection_new_clone (NMConnection *connection)
|
|||
|
||||
clone = nm_simple_connection_new ();
|
||||
nm_connection_set_path (clone, nm_connection_get_path (connection));
|
||||
nm_connection_replace_settings_from_connection (clone, connection, NULL);
|
||||
nm_connection_replace_settings_from_connection (clone, connection);
|
||||
|
||||
return clone;
|
||||
}
|
||||
|
|
|
@ -957,8 +957,6 @@ static void
|
|||
test_connection_replace_settings_from_connection ()
|
||||
{
|
||||
NMConnection *connection, *replacement;
|
||||
GError *error = NULL;
|
||||
gboolean success;
|
||||
NMSettingConnection *s_con;
|
||||
NMSetting *setting;
|
||||
GBytes *ssid;
|
||||
|
@ -996,9 +994,7 @@ test_connection_replace_settings_from_connection ()
|
|||
nm_connection_add_setting (replacement, setting);
|
||||
|
||||
/* Replace settings and test */
|
||||
success = nm_connection_replace_settings_from_connection (connection, replacement, &error);
|
||||
g_assert_no_error (error);
|
||||
g_assert (success);
|
||||
nm_connection_replace_settings_from_connection (connection, replacement);
|
||||
|
||||
s_con = nm_connection_get_setting_connection (connection);
|
||||
g_assert (s_con);
|
||||
|
@ -1017,16 +1013,11 @@ test_connection_replace_settings_from_connection ()
|
|||
static void
|
||||
test_connection_replace_settings_bad (void)
|
||||
{
|
||||
NMConnection *connection, *clone, *new_connection;
|
||||
GHashTable *new_settings;
|
||||
NMConnection *connection, *new_connection;
|
||||
GHashTable *new_settings, *setting_hash;
|
||||
GError *error = NULL;
|
||||
gboolean success;
|
||||
NMSettingConnection *s_con;
|
||||
NMSettingWired *s_wired;
|
||||
|
||||
connection = new_test_connection ();
|
||||
clone = nm_simple_connection_new_clone (connection);
|
||||
g_assert (nm_connection_compare (connection, clone, NM_SETTING_COMPARE_FLAG_EXACT));
|
||||
|
||||
new_connection = new_test_connection ();
|
||||
g_assert (nm_connection_verify (new_connection, NULL));
|
||||
|
@ -1036,33 +1027,40 @@ test_connection_replace_settings_bad (void)
|
|||
NM_SETTING_CONNECTION_ID, "bad-connection",
|
||||
NULL);
|
||||
g_assert (!nm_connection_verify (new_connection, NULL));
|
||||
s_wired = nm_connection_get_setting_wired (new_connection);
|
||||
g_object_set (s_wired,
|
||||
NM_SETTING_WIRED_MTU, 12,
|
||||
NULL);
|
||||
|
||||
/* nm_connection_replace_settings_from_connection() should fail */
|
||||
success = nm_connection_replace_settings_from_connection (connection, new_connection, &error);
|
||||
g_assert (error != NULL);
|
||||
g_assert (!success);
|
||||
g_clear_error (&error);
|
||||
/* nm_connection_replace_settings_from_connection() should succeed */
|
||||
connection = new_test_connection ();
|
||||
nm_connection_replace_settings_from_connection (connection, new_connection);
|
||||
g_assert_cmpstr (nm_connection_get_id (connection), ==, "bad-connection");
|
||||
g_assert (!nm_connection_verify (connection, NULL));
|
||||
g_object_unref (connection);
|
||||
|
||||
g_assert (nm_connection_compare (connection, clone, NM_SETTING_COMPARE_FLAG_EXACT));
|
||||
|
||||
/* nm_connection_replace_settings() should fail */
|
||||
/* nm_connection_replace_settings() should succeed */
|
||||
new_settings = nm_connection_to_dbus (new_connection, NM_CONNECTION_SERIALIZE_ALL);
|
||||
g_assert (new_settings != NULL);
|
||||
|
||||
connection = new_test_connection ();
|
||||
success = nm_connection_replace_settings (connection, new_settings, &error);
|
||||
g_assert (error != NULL);
|
||||
g_assert (!success);
|
||||
g_clear_error (&error);
|
||||
g_assert_no_error (error);
|
||||
g_assert (success);
|
||||
|
||||
g_assert (nm_connection_compare (connection, clone, NM_SETTING_COMPARE_FLAG_EXACT));
|
||||
g_assert_cmpstr (nm_connection_get_id (connection), ==, "bad-connection");
|
||||
g_assert (!nm_connection_verify (connection, NULL));
|
||||
g_object_unref (connection);
|
||||
|
||||
/* But given an invalid hash, it should fail */
|
||||
setting_hash = g_hash_table_new (g_str_hash, g_str_equal);
|
||||
g_hash_table_insert (new_settings, g_strdup ("ip-over-avian-carrier"), setting_hash);
|
||||
|
||||
connection = new_test_connection ();
|
||||
success = nm_connection_replace_settings (connection, new_settings, &error);
|
||||
g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_SETTING);
|
||||
g_assert (!success);
|
||||
|
||||
g_assert (nm_connection_verify (connection, NULL));
|
||||
g_object_unref (connection);
|
||||
|
||||
g_hash_table_unref (new_settings);
|
||||
g_object_unref (connection);
|
||||
g_object_unref (clone);
|
||||
g_object_unref (new_connection);
|
||||
}
|
||||
|
||||
|
@ -2684,9 +2682,7 @@ test_connection_normalize_virtual_iface_name (void)
|
|||
g_assert (G_VALUE_HOLDS_STRING (value));
|
||||
g_assert_cmpstr (g_value_get_string (value), ==, IFACE_NAME);
|
||||
|
||||
/* If vlan.interface-name is invalid, deserialization should fail, with the
|
||||
* correct error.
|
||||
*/
|
||||
/* If vlan.interface-name is invalid, deserialization will fail. */
|
||||
g_value_set_string (value, ":::this-is-not-a-valid-interface-name:::");
|
||||
con = nm_simple_connection_new_from_dbus (connection_hash, &error);
|
||||
g_assert_error (error, NM_SETTING_CONNECTION_ERROR, NM_SETTING_CONNECTION_ERROR_INVALID_PROPERTY);
|
||||
|
|
|
@ -463,38 +463,35 @@ nm_settings_connection_replace_settings (NMSettingsConnection *self,
|
|||
*/
|
||||
g_signal_handlers_block_by_func (self, G_CALLBACK (changed_cb), GUINT_TO_POINTER (TRUE));
|
||||
|
||||
if (nm_connection_replace_settings_from_connection (NM_CONNECTION (self),
|
||||
new_connection,
|
||||
error)) {
|
||||
priv->nm_generated = FALSE;
|
||||
nm_connection_replace_settings_from_connection (NM_CONNECTION (self), new_connection);
|
||||
priv->nm_generated = FALSE;
|
||||
|
||||
/* Cache the just-updated system secrets in case something calls
|
||||
* nm_connection_clear_secrets() and clears them.
|
||||
*/
|
||||
update_system_secrets_cache (self);
|
||||
success = TRUE;
|
||||
/* Cache the just-updated system secrets in case something calls
|
||||
* nm_connection_clear_secrets() and clears them.
|
||||
*/
|
||||
update_system_secrets_cache (self);
|
||||
success = TRUE;
|
||||
|
||||
/* Add agent and always-ask secrets back; they won't necessarily be
|
||||
* in the replacement connection data if it was eg reread from disk.
|
||||
*/
|
||||
if (priv->agent_secrets) {
|
||||
hash = nm_connection_to_dbus (priv->agent_secrets, NM_CONNECTION_SERIALIZE_ONLY_SECRETS);
|
||||
if (hash) {
|
||||
(void) nm_connection_update_secrets (NM_CONNECTION (self), NULL, hash, NULL);
|
||||
g_hash_table_destroy (hash);
|
||||
}
|
||||
/* Add agent and always-ask secrets back; they won't necessarily be
|
||||
* in the replacement connection data if it was eg reread from disk.
|
||||
*/
|
||||
if (priv->agent_secrets) {
|
||||
hash = nm_connection_to_dbus (priv->agent_secrets, NM_CONNECTION_SERIALIZE_ONLY_SECRETS);
|
||||
if (hash) {
|
||||
(void) nm_connection_update_secrets (NM_CONNECTION (self), NULL, hash, NULL);
|
||||
g_hash_table_destroy (hash);
|
||||
}
|
||||
|
||||
nm_settings_connection_recheck_visibility (self);
|
||||
|
||||
/* Manually emit changed signal since we disconnected the handler, but
|
||||
* only update Unsaved if the caller wanted us to.
|
||||
*/
|
||||
changed_cb (self, GUINT_TO_POINTER (update_unsaved));
|
||||
|
||||
g_signal_emit (self, signals[UPDATED_BY_USER], 0);
|
||||
}
|
||||
|
||||
nm_settings_connection_recheck_visibility (self);
|
||||
|
||||
/* Manually emit changed signal since we disconnected the handler, but
|
||||
* only update Unsaved if the caller wanted us to.
|
||||
*/
|
||||
changed_cb (self, GUINT_TO_POINTER (update_unsaved));
|
||||
|
||||
g_signal_emit (self, signals[UPDATED_BY_USER], 0);
|
||||
|
||||
g_signal_handlers_unblock_by_func (self, G_CALLBACK (changed_cb), GUINT_TO_POINTER (TRUE));
|
||||
|
||||
return success;
|
||||
|
|
Loading…
Reference in a new issue