From 10353a996f752af03000497160e3abce1b4c8477 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 23 Jul 2019 08:38:37 +0200 Subject: [PATCH 01/14] examples: add examples/python/gi/nm-update2.py example script This is useful for manually testing Update2() D-Bus API. --- Makefile.examples | 1 + examples/python/gi/nm-update2.py | 155 +++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+) create mode 100755 examples/python/gi/nm-update2.py diff --git a/Makefile.examples b/Makefile.examples index dcd1ac9d18..385a8353ed 100644 --- a/Makefile.examples +++ b/Makefile.examples @@ -178,6 +178,7 @@ EXTRA_DIST += \ examples/python/gi/list-connections.py \ examples/python/gi/nm-add-connection2.py \ examples/python/gi/nm-connection-update-stable-id.py \ + examples/python/gi/nm-update2.py \ examples/python/gi/nm-wg-set \ examples/python/gi/setting-user-data.py \ examples/python/gi/show-wifi-networks.py \ diff --git a/examples/python/gi/nm-update2.py b/examples/python/gi/nm-update2.py new file mode 100755 index 0000000000..36449fd893 --- /dev/null +++ b/examples/python/gi/nm-update2.py @@ -0,0 +1,155 @@ +#!/usr/bin/env python +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Copyright 2019 Red Hat, Inc. +# + +import sys +import re + +import gi +gi.require_version('NM', '1.0') +from gi.repository import GLib, NM + +def find_connections(nm_client, arg_type, arg_id): + for c in nm_client.get_connections(): + if arg_type in [None, 'id'] and c.get_id() == arg_id: + yield c + if arg_type in [None, 'uuid'] and c.get_uuid() == arg_id: + yield c + +def find_connection_first(nm_client, arg_type, arg_id): + for f in find_connections(nm_client, arg_type, arg_id): + return f + +def con_to_str(con): + s_con = con.get_setting_connection() + return '"%s" (%s)' % (s_con.get_id(), s_con.get_uuid()) + +def usage(): + print('Usage: %s [[id] ]' % (sys.argv[0])) + print(' %s [[uuid] ]' % (sys.argv[0])) + return 1 + +def die(msg, print_usage=False): + print(msg) + if print_usage: + usage() + sys.exit(1) + +def main(): + + main_loop = GLib.MainLoop() + + nm_client = NM.Client.new(None) + + arg_mode = None + arg_block_autoconnect = NM.SettingsUpdate2Flags.NONE + arg_volatile = NM.SettingsUpdate2Flags.NONE + arg_no_reapply = NM.SettingsUpdate2Flags.NONE + + cons = [] + + argv = list(sys.argv[1:]) + while argv: + if argv[0] in ['id', 'uuid']: + if cons: + die('cannot specify multiple connections') + if len(argv) < 2: + die('missing argument for "%s" specifier' % (argv[0])) + cons.extend(find_connections(nm_client, argv[0], argv[1])) + if len(cons) == 0: + die('could not find connection for "%s %s"' % (argv[0], argv[1])) + if len(cons) != 1: + die('could not find unique connection for "%s %s"' % (argv[0], argv[1])) + argv = argv[2:] + continue + if argv[0] in ['--block-autoconnect']: + arg_block_autoconnect = NM.SettingsUpdate2Flags.BLOCK_AUTOCONNECT + argv = argv[1:] + continue + if argv[0] in ['--volatile']: + arg_volatile = NM.SettingsUpdate2Flags.VOLATILE + argv = argv[1:] + continue + if argv[0] in ['--no-reapply']: + arg_no_reapply = NM.SettingsUpdate2Flags.NO_REAPPLY + argv = argv[1:] + continue + if argv[0] in ['--to-disk', '--in-memory', '--in-memory-detached', '--in-memory-only']: + if argv[0] == '--to-disk': + v = NM.SettingsUpdate2Flags.TO_DISK + elif argv[0] == '--in-memory': + v = NM.SettingsUpdate2Flags.IN_MEMORY + elif argv[0] == '--in-memory-detached': + v = NM.SettingsUpdate2Flags.IN_MEMORY_DETACHED + elif argv[0] == '--in-memory-only': + v = NM.SettingsUpdate2Flags.IN_MEMORY_ONLY + elif argv[0] == '--keep': + v = NM.SettingsUpdate2Flags.NONE + else: + assert(False) + if arg_mode is not None: + die('duplicate storage modes ("%s")' % (argv[0])) + arg_mode = v + argv = argv[1:] + continue + if cons: + die('unknown argument "%s"' % (argv[0])) + cons.extend(find_connections(nm_client, None, argv[0])) + if len(cons) == 0: + die('could not find connection for "%s"' % (argv[0])) + if len(cons) != 1: + die('could not find unique connection for "%s"' % (argv[0])) + argv = argv[1:] + continue + + if len(cons) != 1: + die('missing connection argument', True) + + con = cons[0] + + con2 = NM.SimpleConnection.new_clone(con) + + result = {} + def _update2_cb(con, async_result, user_data): + try: + r = con.update2_finish(async_result) + except Exception as e: + result['error'] = e + else: + result['result'] = r + main_loop.quit() + + con.update2(con2.to_dbus(NM.ConnectionSerializationFlags.ALL), + (arg_mode if arg_mode is not None else NM.SettingsUpdate2Flags.NONE) + | arg_block_autoconnect + | arg_volatile + | arg_no_reapply, + None, + None, + _update2_cb, + None) + + main_loop.run() + + if 'error' in result: + die('update connection %s failed [%s]: %s' % (con_to_str(con2), ' '.join(sys.argv), result['error'])) + + print('update connection %s succeeded [%s]: %s' % (con_to_str(con2), ' '.join(sys.argv), result['result'])) + +if __name__ == '__main__': + main() From e32d80ea29ecbdef29f0ca6eb0b232d389bc3170 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 22 Jul 2019 21:56:05 +0200 Subject: [PATCH 02/14] settings/trivial: rename NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK to NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK really directly corresponds to NM_SETTINGS_UPDATE2_FLAG_TO_DISK. Rename, so that this is better reflected. --- src/nm-checkpoint.c | 2 +- src/nm-manager.c | 4 ++-- src/settings/nm-settings-connection.c | 2 +- src/settings/nm-settings-connection.h | 2 +- src/settings/nm-settings.c | 14 +++++++------- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index 1f7723c539..9bc979bb94 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -254,7 +254,7 @@ restore_and_activate_connection (NMCheckpoint *self, _LOGD ("rollback: adding connection %s again", nm_connection_get_uuid (dev_checkpoint->settings_connection)); - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; if (!nm_settings_add_connection (NM_SETTINGS_GET, dev_checkpoint->settings_connection, persist_mode, diff --git a/src/nm-manager.c b/src/nm-manager.c index 4f114e4d13..6ea535021a 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5454,7 +5454,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, const char *device_path; const char *specific_object_path; gs_free NMConnection **conns = NULL; - NMSettingsConnectionPersistMode persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + NMSettingsConnectionPersistMode persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; gboolean is_volatile = FALSE; gboolean bind_dbus_client = FALSE; AsyncOpType async_op_type; @@ -5485,7 +5485,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, s = g_variant_get_string (option_value, NULL); is_volatile = FALSE; - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; if (nm_streq (s, "volatile")) { persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY; diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 2093342c9c..f042feac52 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1505,7 +1505,7 @@ update_auth_cb (NMSettingsConnection *self, } if (NM_FLAGS_HAS (info->flags, NM_SETTINGS_UPDATE2_FLAG_TO_DISK)) - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; else if (NM_FLAGS_ANY (info->flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED)) persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED; diff --git a/src/settings/nm-settings-connection.h b/src/settings/nm-settings-connection.h index ab23dc1ff5..ee58919abc 100644 --- a/src/settings/nm-settings-connection.h +++ b/src/settings/nm-settings-connection.h @@ -87,7 +87,7 @@ typedef enum { /* persist to disk. If the profile is currenly in-memory, remove * it from /run. */ - NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK, + NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, /* persist to /run (in-memory). If the profile is currently on disk, * delete it from disk. */ diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 6961134c6c..66858ed99b 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1404,7 +1404,7 @@ nm_settings_add_connection (NMSettings *self, const char *uuid; StorageData *sd; - nm_assert (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK, + nm_assert (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)); nm_assert (!NM_FLAGS_ANY (sett_flags, ~_NM_SETTINGS_CONNECTION_INT_FLAGS_PERSISTENT_MASK)); @@ -1448,7 +1448,7 @@ nm_settings_add_connection (NMSettings *self, if (!_add_connection_to_first_plugin (self, connection, - ( persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK + ( persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK || NM_FLAGS_ANY (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE | NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED)), NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), @@ -1539,7 +1539,7 @@ nm_settings_update_connection (NMSettings *self, nm_assert (!NM_FLAGS_ANY (sett_flags, ~sett_mask)); nm_assert (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP, NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST, - NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK, + NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)); @@ -1582,7 +1582,7 @@ nm_settings_update_connection (NMSettings *self, if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP) { persist_mode = cur_in_memory ? NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED - : NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + : NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; } if ( NM_FLAGS_HAS (sett_mask, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED) @@ -1606,7 +1606,7 @@ nm_settings_update_connection (NMSettings *self, NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST)) { /* making a default-wired-connection a regulard connection implies persisting * it to disk (unless specified differently). */ - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; } } } @@ -1624,7 +1624,7 @@ nm_settings_update_connection (NMSettings *self, persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED; } - if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK) + if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK) new_in_memory = FALSE; else if (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)) @@ -2164,7 +2164,7 @@ settings_add_connection_helper (NMSettings *self, } if (NM_FLAGS_HAS (flags, NM_SETTINGS_ADD_CONNECTION2_FLAG_TO_DISK)) - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; else { nm_assert (NM_FLAGS_HAS (flags, NM_SETTINGS_ADD_CONNECTION2_FLAG_IN_MEMORY)); persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY; From c7e2fe7da6899b09e24791e8a82c6de4e623e2b3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 23 Jul 2019 15:46:49 +0200 Subject: [PATCH 03/14] settings/trivial: rename NMS_KEYFILE_FILETYPE_NMLOADED to NMS_KEYFILE_FILETYPE_NMMETA This name is better suited for the file with extension ".nmmeta". --- src/settings/plugins/keyfile/nms-keyfile-utils.c | 6 +++--- src/settings/plugins/keyfile/nms-keyfile-utils.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/settings/plugins/keyfile/nms-keyfile-utils.c b/src/settings/plugins/keyfile/nms-keyfile-utils.c index 0d03132793..e9d5351573 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/settings/plugins/keyfile/nms-keyfile-utils.c @@ -122,7 +122,7 @@ nms_keyfile_nmmeta_read (const char *dirname, full_filename = g_build_filename (dirname, filename, NULL); - if (!nms_keyfile_utils_check_file_permissions (NMS_KEYFILE_FILETYPE_NMLOADED, + if (!nms_keyfile_utils_check_file_permissions (NMS_KEYFILE_FILETYPE_NMMETA, full_filename, out_st, NULL)) @@ -242,7 +242,7 @@ nms_keyfile_utils_check_file_permissions_stat (NMSKeyfileFiletype filetype, "file is not a regular file"); return FALSE; } - } else if (filetype == NMS_KEYFILE_FILETYPE_NMLOADED) { + } else if (filetype == NMS_KEYFILE_FILETYPE_NMMETA) { if (!S_ISLNK (st->st_mode)) { g_set_error_literal (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "file is not a slink"); @@ -289,7 +289,7 @@ nms_keyfile_utils_check_file_permissions (NMSKeyfileFiletype filetype, "cannot access file: %s", nm_strerror_native (errsv)); return FALSE; } - } else if (filetype == NMS_KEYFILE_FILETYPE_NMLOADED) { + } else if (filetype == NMS_KEYFILE_FILETYPE_NMMETA) { if (lstat (filename, &st) != 0) { errsv = errno; g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, diff --git a/src/settings/plugins/keyfile/nms-keyfile-utils.h b/src/settings/plugins/keyfile/nms-keyfile-utils.h index 1f5280f727..4f4ad54091 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-utils.h +++ b/src/settings/plugins/keyfile/nms-keyfile-utils.h @@ -24,7 +24,7 @@ typedef enum { NMS_KEYFILE_FILETYPE_KEYFILE, - NMS_KEYFILE_FILETYPE_NMLOADED, + NMS_KEYFILE_FILETYPE_NMMETA, } NMSKeyfileFiletype; typedef enum { From aade7e5317253be58c4a5091bbb978e2a9adc0cd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 14:37:57 +0200 Subject: [PATCH 04/14] settings: refactor handling of storages with meta-data/tombstones Currently, meta-data has a very narrow use: as tombstones. Later, we will need to store additional per UUID meta-data. For example, when a profile becomes unsaved, we may need to remember the original filename. Refactor the code for that. This is for the most part just renaming and slightly different handling of the fields. --- src/settings/nm-settings.c | 46 +++++--- .../plugins/keyfile/nms-keyfile-plugin.c | 56 +++++---- .../plugins/keyfile/nms-keyfile-storage.c | 95 +++++++++------ .../plugins/keyfile/nms-keyfile-storage.h | 108 ++++++++++++------ 4 files changed, 190 insertions(+), 115 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 66858ed99b..3e7d847b58 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -168,16 +168,12 @@ nm_assert_storage_data_lst (CList *head) static gboolean _storage_data_is_alive (StorageData *sd) { - if (sd->connection) - return TRUE; - - if (nm_settings_storage_is_keyfile_tombstone (sd->storage)) { - /* entry does not have a profile, but it's here as a tombstone to - * hide/shadow other connections. That's also relevant. */ - return TRUE; - } - - return FALSE; + /* If the storage tracks a connection, it is considered alive. + * + * Meta-data storages are special: they never track a connection. + * We need to check them specially to know when to drop them. */ + return sd->connection + || nm_settings_storage_is_meta_data_alive (sd->storage); } /*****************************************************************************/ @@ -1192,6 +1188,7 @@ _connection_changed_track (NMSettings *self, if (_LOGT_ENABLED ()) { const char *filename; + const NMSettingsMetaData *meta_data; filename = nm_settings_storage_get_filename (storage); if (connection) { @@ -1200,11 +1197,18 @@ _connection_changed_track (NMSettings *self, NM_SETTINGS_STORAGE_PRINT_ARG (storage), nm_connection_get_id (connection), NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); - } else if (nm_settings_storage_is_keyfile_tombstone (storage)) { - _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event for hiding profile%s%s%s", - sett_conn_entry->uuid, - NM_SETTINGS_STORAGE_PRINT_ARG (storage), - NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); + } else if ((meta_data = nm_settings_storage_is_meta_data (storage))) { + if (meta_data->is_tombstone) { + _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event for hiding profile%s%s%s", + sett_conn_entry->uuid, + NM_SETTINGS_STORAGE_PRINT_ARG (storage), + NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); + } else { + _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event with meta data for profile%s%s%s", + sett_conn_entry->uuid, + NM_SETTINGS_STORAGE_PRINT_ARG (storage), + NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); + } } else { _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event for dropping profile%s%s%s", sett_conn_entry->uuid, @@ -1467,8 +1471,12 @@ nm_settings_add_connection (NMSettings *self, sett_conn_entry = _connection_changed_track (self, new_storage, new_connection, TRUE); c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { + const NMSettingsMetaData *meta_data; - if (!nm_settings_storage_is_keyfile_tombstone (sd->storage)) + meta_data = nm_settings_storage_is_meta_data_alive (sd->storage); + + if ( !meta_data + || !meta_data->is_tombstone) continue; if (nm_settings_storage_is_keyfile_run (sd->storage)) { @@ -1484,7 +1492,7 @@ nm_settings_add_connection (NMSettings *self, sd->storage, NULL); - nm_assert (!nm_settings_storage_is_keyfile_tombstone (sd->storage)); + nm_assert (!_storage_data_is_alive (sd)); _connection_changed_track (self, sd->storage, NULL, FALSE); } @@ -1871,10 +1879,10 @@ nm_settings_delete_connection (NMSettings *self, c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { if (sd->storage == cur_storage) continue; - if (nm_settings_storage_is_keyfile_tombstone (sd->storage)) - continue; if (!_storage_data_is_alive (sd)) continue; + if (nm_settings_storage_is_meta_data (sd->storage)) + continue; /* we have still conflicting storages. We need to hide them with tombstones. */ if (nm_settings_storage_is_keyfile_run (sd->storage)) { diff --git a/src/settings/plugins/keyfile/nms-keyfile-plugin.c b/src/settings/plugins/keyfile/nms-keyfile-plugin.c index de076c0122..a7048a8dd1 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-plugin.c +++ b/src/settings/plugins/keyfile/nms-keyfile-plugin.c @@ -244,7 +244,7 @@ _nm_assert_storage (gpointer plugin /* NMSKeyfilePlugin */, nm_assert (!plugin || NMS_IS_KEYFILE_PLUGIN (plugin)); nm_assert (NMS_IS_KEYFILE_STORAGE (storage)); nm_assert (!plugin || plugin == nm_settings_storage_get_plugin (storage)); - nm_assert (!((NMSKeyfileStorage *) storage)->is_tombstone || !(((NMSKeyfileStorage *) storage)->connection)); + nm_assert (({ const char *f = nms_keyfile_storage_get_filename (storage); f && f[0] == '/'; @@ -254,6 +254,11 @@ _nm_assert_storage (gpointer plugin /* NMSKeyfilePlugin */, nm_assert (nm_utils_is_uuid (uuid)); + nm_assert ( ((NMSKeyfileStorage *) storage)->is_meta_data + || !(((NMSKeyfileStorage *) storage)->u.conn_data.connection) + || ( NM_IS_CONNECTION ((((NMSKeyfileStorage *) storage)->u.conn_data.connection)) + && nm_streq0 (uuid, nm_connection_get_uuid ((((NMSKeyfileStorage *) storage)->u.conn_data.connection))))); + nm_assert ( !tracked || !plugin || c_list_contains (&NMS_KEYFILE_PLUGIN_GET_PRIVATE (plugin)->storages._storage_lst_head, @@ -264,7 +269,8 @@ _nm_assert_storage (gpointer plugin /* NMSKeyfilePlugin */, || storage == g_hash_table_lookup (NMS_KEYFILE_PLUGIN_GET_PRIVATE (plugin)->storages.idx_by_filename, nms_keyfile_storage_get_filename (storage))); - if (tracked && plugin) { + if ( tracked + && plugin) { sbuh = g_hash_table_lookup (NMS_KEYFILE_PLUGIN_GET_PRIVATE (plugin)->storages.idx_by_uuid, &uuid); nm_assert (sbuh); nm_assert (c_list_contains (&sbuh->_storage_by_uuid_lst_head, &((NMSKeyfileStorage *) storage)->parent._storage_by_uuid_lst)); @@ -454,7 +460,7 @@ _storages_consolidate (NMSKeyfilePlugin *self, c_list_init (&storages_deleted); c_list_for_each_entry (storage_old, &priv->storages._storage_lst_head, parent._storage_lst) - storage_old->dirty = TRUE; + storage_old->is_dirty = TRUE; c_list_for_each_entry_safe (storage_new, storage_safe, &storages_new->_storage_lst_head, parent._storage_lst) { storage_old = nm_sett_util_storages_lookup_by_filename (&priv->storages, nms_keyfile_storage_get_filename (storage_new)); @@ -465,28 +471,28 @@ _storages_consolidate (NMSKeyfilePlugin *self, || !nm_streq (nms_keyfile_storage_get_uuid (storage_new), nms_keyfile_storage_get_uuid (storage_old))) { if (storage_old) { nm_sett_util_storages_steal (&priv->storages, storage_old); - c_list_link_tail (&storages_deleted, &storage_old->parent._storage_lst); + c_list_link_tail (&storages_deleted, &storage_old->parent._storage_by_uuid_lst); } - storage_new->dirty = FALSE; + storage_new->is_dirty = FALSE; nm_sett_util_storages_add_take (&priv->storages, storage_new); g_ptr_array_add (storages_modified, g_object_ref (storage_new)); continue; } - storage_old->dirty = FALSE; + storage_old->is_dirty = FALSE; nms_keyfile_storage_copy_content (storage_old, storage_new); nms_keyfile_storage_destroy (storage_new); g_ptr_array_add (storages_modified, g_object_ref (storage_old)); } c_list_for_each_entry_safe (storage_old, storage_safe, &priv->storages._storage_lst_head, parent._storage_lst) { - if (!storage_old->dirty) + if (!storage_old->is_dirty) continue; if ( replace_all || ( storages_replaced && g_hash_table_contains (storages_replaced, storage_old))) { nm_sett_util_storages_steal (&priv->storages, storage_old); - c_list_link_tail (&storages_deleted, &storage_old->parent._storage_lst); + c_list_link_tail (&storages_deleted, &storage_old->parent._storage_by_uuid_lst); } } @@ -494,7 +500,7 @@ _storages_consolidate (NMSKeyfilePlugin *self, for (i = 0; i < storages_modified->len; i++) { storage = storages_modified->pdata[i]; - storage->dirty = TRUE; + storage->is_dirty = TRUE; } for (i = 0; i < storages_modified->len; i++) { @@ -502,19 +508,22 @@ _storages_consolidate (NMSKeyfilePlugin *self, storage = storages_modified->pdata[i]; - if (!storage->dirty) { - /* the entry is no longer dirty. In the meantime we already emited + if (!storage->is_dirty) { + /* the entry is no longer is_dirty. In the meantime we already emited * another signal for it. */ continue; } - storage->dirty = FALSE; - if (storage != nm_sett_util_storages_lookup_by_filename (&priv->storages, nms_keyfile_storage_get_filename (storage))) { + storage->is_dirty = FALSE; + + if (c_list_is_empty (&storage->parent._storage_lst)) { /* hm? The profile was deleted in the meantime? That is only possible * if the signal handler called again into the plugin. In any case, the event * was already emitted. Skip. */ continue; } + nm_assert (storage == nm_sett_util_storages_lookup_by_filename (&priv->storages, nms_keyfile_storage_get_filename (storage))); + connection = nms_keyfile_storage_steal_connection (storage); callback (NM_SETTINGS_PLUGIN (self), @@ -523,9 +532,8 @@ _storages_consolidate (NMSKeyfilePlugin *self, user_data); } - while ((storage = c_list_first_entry (&storages_deleted, NMSKeyfileStorage, parent._storage_lst))) { - c_list_unlink (&storage->parent._storage_lst); - storage->is_tombstone = FALSE; + while ((storage = c_list_first_entry (&storages_deleted, NMSKeyfileStorage, parent._storage_by_uuid_lst))) { + c_list_unlink (&storage->parent._storage_by_uuid_lst); callback (NM_SETTINGS_PLUGIN (self), NM_SETTINGS_STORAGE (storage), NULL, @@ -851,6 +859,7 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, || storage->storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN); nm_assert ( priv->dirname_etc || storage->storage_type != NMS_KEYFILE_STORAGE_TYPE_ETC); + nm_assert (!storage->is_meta_data); previous_filename = nms_keyfile_storage_get_filename (storage); uuid = nms_keyfile_storage_get_uuid (storage); @@ -895,9 +904,9 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, uuid, nm_connection_get_id (connection)); - storage->is_nm_generated = is_nm_generated; - storage->is_volatile = is_volatile; - storage->stat_mtime = *nm_sett_util_stat_mtime (full_filename, FALSE, &mtime); + storage->u.conn_data.is_nm_generated = is_nm_generated; + storage->u.conn_data.is_volatile = is_volatile; + storage->u.conn_data.stat_mtime = *nm_sett_util_stat_mtime (full_filename, FALSE, &mtime); *out_storage = g_object_ref (NM_SETTINGS_STORAGE (storage)); *out_connection = g_steal_pointer (&reread); @@ -969,14 +978,13 @@ delete_connection (NMSettingsPlugin *plugin, _LOGT ("commit: deleted \"%s\", %s %s (%s%s%s%s)", previous_filename, - storage->is_tombstone ? "tombstone" : "profile", + storage->is_meta_data ? "meta-data" : "profile", uuid, operation_message, NM_PRINT_FMT_QUOTED (remove_from_disk_errmsg, ": ", remove_from_disk_errmsg, "", "")); if (success) { nm_sett_util_storages_steal (&priv->storages, storage); - storage->is_tombstone = FALSE; nms_keyfile_storage_destroy (storage); } @@ -1088,7 +1096,7 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, storage = nm_sett_util_storages_lookup_by_filename (&priv->storages, nmmeta_filename); nm_assert ( !storage - || ( storage->is_tombstone + || ( storage->is_meta_data && storage->storage_type == storage_type && nm_streq (nms_keyfile_storage_get_uuid (storage), uuid))); @@ -1104,10 +1112,8 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, storage_result = g_object_ref (storage); } else { - if (storage) { + if (storage) storage_result = nm_sett_util_storages_steal (&priv->storages, storage); - storage_result->is_tombstone = FALSE; - } } out: diff --git a/src/settings/plugins/keyfile/nms-keyfile-storage.c b/src/settings/plugins/keyfile/nms-keyfile-storage.c index d38e187d22..33ebb8f941 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-storage.c +++ b/src/settings/plugins/keyfile/nms-keyfile-storage.c @@ -41,24 +41,32 @@ nms_keyfile_storage_copy_content (NMSKeyfileStorage *dst, { nm_assert (src != dst); nm_assert (nm_streq (nms_keyfile_storage_get_uuid (dst), nms_keyfile_storage_get_uuid (src))); - nm_assert (nms_keyfile_storage_get_filename (dst) && nm_streq (nms_keyfile_storage_get_filename (dst), nms_keyfile_storage_get_filename (src))); + nm_assert ( nms_keyfile_storage_get_filename (dst) + && nm_streq (nms_keyfile_storage_get_filename (dst), nms_keyfile_storage_get_filename (src))); + nm_assert (dst->storage_type == src->storage_type); + nm_assert (dst->is_meta_data == src->is_meta_data); - nm_g_object_ref_set (&dst->connection, src->connection); - dst->storage_type = src->storage_type; - dst->stat_mtime = src->stat_mtime; - dst->is_nm_generated = src->is_nm_generated; - dst->is_volatile = src->is_volatile; - dst->is_tombstone = src->is_tombstone; + if (dst->is_meta_data) + dst->u.meta_data = src->u.meta_data; + else { + gs_unref_object NMConnection *connection_to_free = NULL; + + connection_to_free = g_steal_pointer (&dst->u.conn_data.connection); + dst->u.conn_data = src->u.conn_data; + nm_g_object_ref (dst->u.conn_data.connection); + } } NMConnection * nms_keyfile_storage_steal_connection (NMSKeyfileStorage *self) { nm_assert (NMS_IS_KEYFILE_STORAGE (self)); - nm_assert ( (!self->connection && self->is_tombstone) - || NM_IS_CONNECTION (self->connection)); + nm_assert ( self->is_meta_data + || NM_IS_CONNECTION (self->u.conn_data.connection)); - return g_steal_pointer (&self->connection); + return self->is_meta_data + ? NULL + : g_steal_pointer (&self->u.conn_data.connection); } /*****************************************************************************/ @@ -75,16 +83,19 @@ cmp_fcn (const NMSKeyfileStorage *a, * (inverse) priority. */ NM_CMP_FIELD_UNSAFE (b, a, storage_type); - /* tombstones are more important. */ - nm_assert (a->is_tombstone == nm_settings_storage_is_keyfile_tombstone (NM_SETTINGS_STORAGE (a))); - nm_assert (b->is_tombstone == nm_settings_storage_is_keyfile_tombstone (NM_SETTINGS_STORAGE (b))); - NM_CMP_FIELD_UNSAFE (a, b, is_tombstone); + /* meta-data is more important. */ + NM_CMP_FIELD_UNSAFE (a, b, is_meta_data); - /* newer files are more important. */ - NM_CMP_FIELD (a, b, stat_mtime.tv_sec); - NM_CMP_FIELD (a, b, stat_mtime.tv_nsec); + if (a->is_meta_data) { + nm_assert (nm_streq (nms_keyfile_storage_get_filename (a), nms_keyfile_storage_get_filename (b))); + NM_CMP_FIELD_UNSAFE (a, b, u.meta_data.is_tombstone); + } else { + /* newer files are more important. */ + NM_CMP_FIELD (a, b, u.conn_data.stat_mtime.tv_sec); + NM_CMP_FIELD (a, b, u.conn_data.stat_mtime.tv_nsec); - NM_CMP_DIRECT_STRCMP (nms_keyfile_storage_get_filename (a), nms_keyfile_storage_get_filename (b)); + NM_CMP_DIRECT_STRCMP (nms_keyfile_storage_get_filename (a), nms_keyfile_storage_get_filename (b)); + } return 0; } @@ -99,17 +110,27 @@ nms_keyfile_storage_init (NMSKeyfileStorage *self) static NMSKeyfileStorage * _storage_new (NMSKeyfilePlugin *plugin, const char *uuid, - const char *filename) + const char *filename, + gboolean is_meta_data, + NMSKeyfileStorageType storage_type) + { + NMSKeyfileStorage *self; + nm_assert (NMS_IS_KEYFILE_PLUGIN (plugin)); nm_assert (nm_utils_is_uuid (uuid)); nm_assert (filename && filename[0] == '/'); - return g_object_new (NMS_TYPE_KEYFILE_STORAGE, + self = g_object_new (NMS_TYPE_KEYFILE_STORAGE, NM_SETTINGS_STORAGE_PLUGIN, plugin, NM_SETTINGS_STORAGE_UUID, uuid, NM_SETTINGS_STORAGE_FILENAME, filename, NULL); + + *((bool *) &self->is_meta_data) = is_meta_data; + *((NMSKeyfileStorageType *) &self->storage_type) = storage_type; + + return self; } NMSKeyfileStorage * @@ -126,12 +147,8 @@ nms_keyfile_storage_new_tombstone (NMSKeyfilePlugin *plugin, nm_assert (NM_IN_SET (storage_type, NMS_KEYFILE_STORAGE_TYPE_ETC, NMS_KEYFILE_STORAGE_TYPE_RUN)); - self = _storage_new (plugin, uuid, filename); - - self->is_tombstone = TRUE; - - self->storage_type = storage_type; - + self = _storage_new (plugin, uuid, filename, TRUE, storage_type); + self->u.meta_data.is_tombstone = TRUE; return self; } @@ -154,19 +171,17 @@ nms_keyfile_storage_new_connection (NMSKeyfilePlugin *plugin, && storage_type <= _NMS_KEYFILE_STORAGE_TYPE_LIB_LAST); nmtst_connection_assert_unchanging (connection_take); - self = _storage_new (plugin, nm_connection_get_uuid (connection_take), filename); + self = _storage_new (plugin, nm_connection_get_uuid (connection_take), filename, FALSE, storage_type); - self->connection = connection_take; /* take reference. */ - - if (storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN) { - self->is_nm_generated = (is_nm_generated_opt == NM_TERNARY_TRUE); - self->is_volatile = (is_volatile_opt == NM_TERNARY_TRUE); - } + self->u.conn_data.connection = connection_take; /* take reference. */ if (stat_mtime) - self->stat_mtime = *stat_mtime; + self->u.conn_data.stat_mtime = *stat_mtime; - self->storage_type = storage_type; + if (storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN) { + self->u.conn_data.is_nm_generated = (is_nm_generated_opt == NM_TERNARY_TRUE); + self->u.conn_data.is_volatile = (is_volatile_opt == NM_TERNARY_TRUE); + } return self; } @@ -176,7 +191,8 @@ _storage_clear (NMSKeyfileStorage *self) { c_list_unlink (&self->parent._storage_lst); c_list_unlink (&self->parent._storage_by_uuid_lst); - g_clear_object (&self->connection); + if (!self->is_meta_data) + g_clear_object (&self->u.conn_data.connection); } static void @@ -226,12 +242,15 @@ nm_settings_storage_load_sett_flags (NMSettingsStorage *self, return; s = NMS_KEYFILE_STORAGE (self); + + if (s->is_meta_data) + return; if (s->storage_type != NMS_KEYFILE_STORAGE_TYPE_RUN) return; - if (s->is_nm_generated) + if (s->u.conn_data.is_nm_generated) *sett_flags |= NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED; - if (s->is_volatile) + if (s->u.conn_data.is_volatile) *sett_flags |= NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE; } diff --git a/src/settings/plugins/keyfile/nms-keyfile-storage.h b/src/settings/plugins/keyfile/nms-keyfile-storage.h index 0cc537b7bf..7968da09ac 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-storage.h +++ b/src/settings/plugins/keyfile/nms-keyfile-storage.h @@ -33,6 +33,11 @@ #define NMS_IS_KEYFILE_STORAGE_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NMS_TYPE_KEYFILE_STORAGE)) #define NMS_KEYFILE_STORAGE_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), NMS_TYPE_KEYFILE_STORAGE, NMSKeyfileStorageClass)) +typedef struct { + /* whether this is a tombstone to hide a UUID (via symlink to /dev/null). */ + bool is_tombstone:1; +} NMSettingsMetaData; + typedef struct { NMSettingsStorage parent; @@ -42,35 +47,45 @@ typedef struct { * Also, we don't actually remember the loaded connection after returning it * to NMSettings. So, also for regular storages (non-tombstones) this field * is often cleared. */ - NMConnection *connection; + union { + struct { + NMConnection *connection; - NMSKeyfileStorageType storage_type; + /* the timestamp (stat's mtime) of the keyfile. For meta-data this + * is irrelevant. The purpose is that if the same storage type (directory) has + * multiple files with the same UUID, then the newer file gets preferred. */ + struct timespec stat_mtime; - /* the timestamp (stat's mtime) of the keyfile. For tombstones this - * is irrelevant. The purpose is that if the same storage type (directory) has - * multiple files with the same UUID, then the newer file gets preferred. */ - struct timespec stat_mtime; + /* these flags are only relevant for storages with %NMS_KEYFILE_STORAGE_TYPE_RUN + * (and non-metadata). This is to persist and reload these settings flags to + * /run. + * + * Note that these flags are not stored in as meta-data. The reason is that meta-data + * is per UUID. But these flags are only relevant for a particular keyfile on disk. + * That is, it must be tied to the actual keyfile, and not to the UUID. */ + bool is_nm_generated:1; + bool is_volatile:1; - /* these flags are only relevant for storages with %NMS_KEYFILE_STORAGE_TYPE_RUN - * (and non-tombstones). This is to persist and reload these settings flags to - * /run. */ - bool is_nm_generated:1; - bool is_volatile:1; + } conn_data; - /* whether this is a tombstone to hide a UUID (via the loaded uuid symlinks). - * If this is falls, the storage contains a profile, though note that - * the connection field will be cleared when it's not used. So, a non-tombstone - * has a connection in principle, but the connection field may still be %NULL. - * - * Note that a tombstone instance doesn't have a connection, but NMSettings - * considers it alive because is_tombstone is %TRUE. That means, once a tombstone - * gets removed, this flag is cleared. Then the storage instance has no connnection - * and is no longer a tombstone, and NMSettings considers it ready for deletion. - */ - bool is_tombstone:1; + /* the content from the .nmmeta file. Note that the nmmeta file has the UUID + * in the filename, that means there can be only two variants of this file: + * in /etc and in /run. As such, this is really meta-data about the entire profile + * (the UUID), and not about the individual keyfile. */ + NMSettingsMetaData meta_data; + + } u; + + /* The storage type. This is directly related to the filename. Since + * the filename cannot change, this value is unchanging. */ + const NMSKeyfileStorageType storage_type; + + /* whether union "u" has meta_data or conn_data. Since the type of the storage + * depends on the (immutable) filename, this is also const. */ + const bool is_meta_data; /* this flag is only used during reload to mark and prune old entries. */ - bool dirty:1; + bool is_dirty:1; } NMSKeyfileStorage; @@ -132,18 +147,45 @@ nm_settings_storage_is_keyfile_lib (const NMSettingsStorage *self) && (((NMSKeyfileStorage *) self)->storage_type >= NMS_KEYFILE_STORAGE_TYPE_LIB_BASE); } -static inline gboolean -nm_settings_storage_is_keyfile_tombstone (const NMSettingsStorage *self) +static inline const NMSettingsMetaData * +nm_settings_storage_is_meta_data (const NMSettingsStorage *storage) { - /* Only keyfile storage supports tombstones. They indicate that a uuid - * is shadowed via a symlink to /dev/null. + const NMSKeyfileStorage *self; + + if (!NMS_IS_KEYFILE_STORAGE (storage)) + return NULL; + + self = (NMSKeyfileStorage *) storage; + + if (!self->is_meta_data) + return NULL; + + return &self->u.meta_data; +} + +static inline const NMSettingsMetaData * +nm_settings_storage_is_meta_data_alive (const NMSettingsStorage *storage) +{ + const NMSettingsMetaData *meta_data; + + meta_data = nm_settings_storage_is_meta_data (storage); + + if (!meta_data) + return NULL; + + /* Regular (all other) storages are alive as long as they report a NMConnection, and + * they will be dropped, once they have no more connection. * - * Note that tombstones don't have a NMConnection instead they shadow - * a UUID. As such, NMSettings considers them alive also if they have - * not profile. That means, when a tombstone gets removed for good, - * the is_tombstone must be cleared (so that it becomes truly dead). */ - return NMS_IS_KEYFILE_STORAGE (self) - && ((NMSKeyfileStorage *) self)->is_tombstone; + * Meta-data storages are special: they never report a NMConnection. + * So, a meta-data storage is alive as long as it is tracked by the + * settings plugin. + * + * This function is used to ckeck for that. */ + + if (c_list_is_empty (&storage->_storage_lst)) + return NULL; + + return meta_data; } /*****************************************************************************/ From e3b5b1e64b9afe56ba5cd452775ba02a9d92dd2f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 16:55:47 +0200 Subject: [PATCH 05/14] settings: support storing "shadowed-storage" to [.nmmeta] section for keyfiles in /run When we make runtime only changes, we may leave the profile in persistent storage and store a overlay profile in /run. However, in various cases we need to remember the original path. Add code to store and retrieve that path from the keyfile. Note that this data is written inside the keyfile in /run. That is because this piece of information really depends on that particular keyfile, and not on the profile/UUID. That is why we write it to the [.nmmeta] section of the keyfile and not to the .nmmeta file (which is per-UUID). This patch only adds the backend to write and load the setting from disk. It's not yet used. --- libnm-core/nm-keyfile-internal.h | 2 + src/settings/nm-settings.c | 17 ++++- .../plugins/keyfile/nms-keyfile-plugin.c | 64 ++++++++++++++++--- .../plugins/keyfile/nms-keyfile-plugin.h | 6 +- .../plugins/keyfile/nms-keyfile-reader.c | 12 ++++ .../plugins/keyfile/nms-keyfile-reader.h | 2 + .../plugins/keyfile/nms-keyfile-storage.c | 14 +++- .../plugins/keyfile/nms-keyfile-storage.h | 60 +++++++++++++++++ .../plugins/keyfile/nms-keyfile-writer.c | 24 +++++++ .../plugins/keyfile/nms-keyfile-writer.h | 2 + .../keyfile/tests/test-keyfile-settings.c | 2 + 11 files changed, 193 insertions(+), 12 deletions(-) diff --git a/libnm-core/nm-keyfile-internal.h b/libnm-core/nm-keyfile-internal.h index e1fb10c2b4..ced4ed975f 100644 --- a/libnm-core/nm-keyfile-internal.h +++ b/libnm-core/nm-keyfile-internal.h @@ -174,6 +174,8 @@ gboolean _nm_keyfile_has_values (GKeyFile *keyfile); #define NM_KEYFILE_GROUP_NMMETA ".nmmeta" #define NM_KEYFILE_KEY_NMMETA_NM_GENERATED "nm-generated" #define NM_KEYFILE_KEY_NMMETA_VOLATILE "volatile" +#define NM_KEYFILE_KEY_NMMETA_SHADOWED_STORAGE "shadowed-storage" +#define NM_KEYFILE_KEY_NMMETA_SHADOWED_OWNED "shadowed-owned" #define NM_KEYFILE_PATH_NAME_LIB NMLIBDIR "/system-connections" #define NM_KEYFILE_PATH_NAME_ETC_DEFAULT NMCONFDIR "/system-connections" diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 3e7d847b58..8679128a23 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1284,6 +1284,8 @@ _add_connection_to_first_plugin (NMSettings *self, gboolean in_memory, gboolean is_nm_generated, gboolean is_volatile, + const char *shadowed_storage, + gboolean shadowed_owned, NMSettingsStorage **out_new_storage, NMConnection **out_new_connection, GError **error) @@ -1311,9 +1313,11 @@ _add_connection_to_first_plugin (NMSettings *self, if (plugin == (NMSettingsPlugin *) priv->keyfile_plugin) { success = nms_keyfile_plugin_add_connection (priv->keyfile_plugin, new_connection, + in_memory, is_nm_generated, is_volatile, - in_memory, + shadowed_storage, + shadowed_owned, &storage, &connection_to_add, &add_error); @@ -1457,6 +1461,8 @@ nm_settings_add_connection (NMSettings *self, | NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED)), NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), + NULL, + FALSE, &new_storage, &new_connection, &local)) { @@ -1538,6 +1544,8 @@ nm_settings_update_connection (NMSettings *self, gboolean cur_in_memory; gboolean new_in_memory; const char *uuid; + const char *shadowed_storage; + gboolean shadowed_owned; g_return_val_if_fail (NM_IS_SETTINGS (self), FALSE); g_return_val_if_fail (NM_IS_SETTINGS_CONNECTION (sett_conn), FALSE); @@ -1659,6 +1667,9 @@ nm_settings_update_connection (NMSettings *self, | NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE); } + /* TODO: set and handle shadowed-storages. */ + shadowed_storage = NULL; + shadowed_owned = FALSE; if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST) { new_storage = g_object_ref (cur_storage); @@ -1688,6 +1699,8 @@ nm_settings_update_connection (NMSettings *self, new_in_memory, NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), + shadowed_storage, + shadowed_owned, &new_storage, &new_connection, &local); @@ -1701,6 +1714,8 @@ nm_settings_update_connection (NMSettings *self, connection, NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), + shadowed_storage, + shadowed_owned, NM_FLAGS_HAS (update_reason, NM_SETTINGS_CONNECTION_UPDATE_REASON_FORCE_RENAME), &new_storage, &new_connection, diff --git a/src/settings/plugins/keyfile/nms-keyfile-plugin.c b/src/settings/plugins/keyfile/nms-keyfile-plugin.c index a7048a8dd1..eb3ae5f100 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-plugin.c +++ b/src/settings/plugins/keyfile/nms-keyfile-plugin.c @@ -216,13 +216,22 @@ _read_from_file (const char *full_filename, struct stat *out_stat, NMTernary *out_is_nm_generated, NMTernary *out_is_volatile, + char **out_shadowed_storage, + NMTernary *out_shadowed_owned, GError **error) { NMConnection *connection; nm_assert (full_filename && full_filename[0] == '/'); - connection = nms_keyfile_reader_from_file (full_filename, plugin_dir, out_stat, out_is_nm_generated, out_is_volatile, error); + connection = nms_keyfile_reader_from_file (full_filename, + plugin_dir, + out_stat, + out_is_nm_generated, + out_is_volatile, + out_shadowed_storage, + out_shadowed_owned, + error); nm_assert (!connection || (_nm_connection_verify (connection, NULL) == NM_SETTING_VERIFY_SUCCESS)); nm_assert (!connection || nm_utils_is_uuid (nm_connection_get_uuid (connection))); @@ -291,6 +300,8 @@ _load_file (NMSKeyfilePlugin *self, gs_unref_object NMConnection *connection = NULL; NMTernary is_volatile_opt; NMTernary is_nm_generated_opt; + NMTernary shadowed_owned_opt; + gs_free char *shadowed_storage = NULL; gs_free_error GError *local = NULL; gs_free char *full_filename = NULL; struct stat st; @@ -350,6 +361,8 @@ _load_file (NMSKeyfilePlugin *self, &st, &is_nm_generated_opt, &is_volatile_opt, + &shadowed_storage, + &shadowed_owned_opt, &local); if (!connection) { if (error) @@ -365,6 +378,8 @@ _load_file (NMSKeyfilePlugin *self, storage_type, is_nm_generated_opt, is_volatile_opt, + shadowed_storage, + shadowed_owned_opt, &st.st_mtim); } @@ -725,9 +740,11 @@ load_connections (NMSettingsPlugin *plugin, gboolean nms_keyfile_plugin_add_connection (NMSKeyfilePlugin *self, NMConnection *connection, + gboolean in_memory, gboolean is_nm_generated, gboolean is_volatile, - gboolean in_memory, + const char *shadowed_storage, + gboolean shadowed_owned, NMSettingsStorage **out_storage, NMConnection **out_connection, GError **error) @@ -747,8 +764,16 @@ nms_keyfile_plugin_add_connection (NMSKeyfilePlugin *self, nm_assert (out_storage && !*out_storage); nm_assert (out_connection && !*out_connection); + nm_assert ( in_memory + || ( !is_nm_generated + && !is_volatile + && !shadowed_storage + && !shadowed_owned)); + uuid = nm_connection_get_uuid (connection); + /* Note that even if the caller requests persistent storage, we may switch to in-memory, if + * no /etc directory is configured. */ storage_type = !in_memory && priv->dirname_etc ? NMS_KEYFILE_STORAGE_TYPE_ETC : NMS_KEYFILE_STORAGE_TYPE_RUN; @@ -756,6 +781,8 @@ nms_keyfile_plugin_add_connection (NMSKeyfilePlugin *self, if (!nms_keyfile_writer_connection (connection, is_nm_generated, is_volatile, + shadowed_storage, + shadowed_owned, storage_type == NMS_KEYFILE_STORAGE_TYPE_ETC ? priv->dirname_etc : priv->dirname_run, @@ -787,11 +814,12 @@ nms_keyfile_plugin_add_connection (NMSKeyfilePlugin *self, nm_assert (full_filename && full_filename[0] == '/'); nm_assert (!nm_sett_util_storages_lookup_by_filename (&priv->storages, full_filename)); - _LOGT ("commit: %s (%s) added as \"%s\"%s", + _LOGT ("commit: %s (%s) added as \"%s\"%s%s%s%s", uuid, nm_connection_get_id (connection), full_filename, - _extra_flags_to_string (strbuf, sizeof (strbuf), is_nm_generated, is_volatile)); + _extra_flags_to_string (strbuf, sizeof (strbuf), is_nm_generated, is_volatile), + NM_PRINT_FMT_QUOTED (shadowed_storage, " (shadows \"", shadowed_storage, shadowed_owned ? "\", owned)" : "\")", "")); storage = nms_keyfile_storage_new_connection (self, g_steal_pointer (&reread), @@ -799,6 +827,8 @@ nms_keyfile_plugin_add_connection (NMSKeyfilePlugin *self, storage_type, is_nm_generated ? NM_TERNARY_TRUE : NM_TERNARY_FALSE, is_volatile ? NM_TERNARY_TRUE : NM_TERNARY_FALSE, + shadowed_storage, + shadowed_owned ? NM_TERNARY_TRUE : NM_TERNARY_FALSE, nm_sett_util_stat_mtime (full_filename, FALSE, &mtime)); nm_sett_util_storages_add_take (&priv->storages, g_object_ref (storage)); @@ -821,6 +851,8 @@ add_connection (NMSettingsPlugin *plugin, FALSE, FALSE, FALSE, + NULL, + FALSE, out_storage, out_connection, error); @@ -832,6 +864,8 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, NMConnection *connection, gboolean is_nm_generated, gboolean is_volatile, + const char *shadowed_storage, + gboolean shadowed_owned, gboolean force_rename, NMSettingsStorage **out_storage, NMConnection **out_connection, @@ -847,6 +881,7 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, const char *previous_filename; gboolean reread_same; const char *uuid; + char strbuf[100]; _nm_assert_storage (self, storage, TRUE); nm_assert (NM_IS_CONNECTION (connection)); @@ -855,11 +890,15 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, nm_assert (!error || !*error); nm_assert (NM_IN_SET (storage->storage_type, NMS_KEYFILE_STORAGE_TYPE_ETC, NMS_KEYFILE_STORAGE_TYPE_RUN)); - nm_assert ( (!is_nm_generated && !is_volatile) - || storage->storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN); + nm_assert (!storage->is_meta_data); + nm_assert ( storage->storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN + || ( !is_nm_generated + && !is_volatile + && !shadowed_storage + && !shadowed_owned)); + nm_assert (!shadowed_owned || shadowed_storage); nm_assert ( priv->dirname_etc || storage->storage_type != NMS_KEYFILE_STORAGE_TYPE_ETC); - nm_assert (!storage->is_meta_data); previous_filename = nms_keyfile_storage_get_filename (storage); uuid = nms_keyfile_storage_get_uuid (storage); @@ -867,6 +906,8 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, if (!nms_keyfile_writer_connection (connection, is_nm_generated, is_volatile, + shadowed_storage, + shadowed_owned, storage->storage_type == NMS_KEYFILE_STORAGE_TYPE_ETC ? priv->dirname_etc : priv->dirname_run, @@ -899,14 +940,17 @@ nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, nm_assert (_nm_connection_verify (reread, NULL) == NM_SETTING_VERIFY_SUCCESS); nm_assert (nm_streq (nm_connection_get_uuid (reread), uuid)); - _LOGT ("commit: \"%s\": profile %s (%s) written", + _LOGT ("commit: \"%s\": profile %s (%s) written%s%s%s%s", full_filename, uuid, - nm_connection_get_id (connection)); + nm_connection_get_id (connection), + _extra_flags_to_string (strbuf, sizeof (strbuf), is_nm_generated, is_volatile), + NM_PRINT_FMT_QUOTED (shadowed_storage, shadowed_owned ? " (owns \"" : " (shadows \"", shadowed_storage, "\")", "")); storage->u.conn_data.is_nm_generated = is_nm_generated; storage->u.conn_data.is_volatile = is_volatile; storage->u.conn_data.stat_mtime = *nm_sett_util_stat_mtime (full_filename, FALSE, &mtime); + storage->u.conn_data.shadowed_owned = shadowed_owned; *out_storage = g_object_ref (NM_SETTINGS_STORAGE (storage)); *out_connection = g_steal_pointer (&reread); @@ -926,6 +970,8 @@ update_connection (NMSettingsPlugin *plugin, connection, FALSE, FALSE, + NULL, + FALSE, FALSE, out_storage, out_connection, diff --git a/src/settings/plugins/keyfile/nms-keyfile-plugin.h b/src/settings/plugins/keyfile/nms-keyfile-plugin.h index f3e6870861..b2a4a23864 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-plugin.h +++ b/src/settings/plugins/keyfile/nms-keyfile-plugin.h @@ -42,9 +42,11 @@ NMSKeyfilePlugin *nms_keyfile_plugin_new (void); gboolean nms_keyfile_plugin_add_connection (NMSKeyfilePlugin *self, NMConnection *connection, + gboolean in_memory, gboolean is_nm_generated, gboolean is_volatile, - gboolean in_memory, + const char *shadowed_storage, + gboolean shadowed_owned, NMSettingsStorage **out_storage, NMConnection **out_connection, GError **error); @@ -54,6 +56,8 @@ gboolean nms_keyfile_plugin_update_connection (NMSKeyfilePlugin *self, NMConnection *connection, gboolean is_nm_generated, gboolean is_volatile, + const char *shadowed_storage, + gboolean shadowed_owned, gboolean force_rename, NMSettingsStorage **out_storage, NMConnection **out_connection, diff --git a/src/settings/plugins/keyfile/nms-keyfile-reader.c b/src/settings/plugins/keyfile/nms-keyfile-reader.c index a6562a04dc..8d1f5599fe 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-reader.c +++ b/src/settings/plugins/keyfile/nms-keyfile-reader.c @@ -164,6 +164,8 @@ nms_keyfile_reader_from_file (const char *full_filename, struct stat *out_stat, NMTernary *out_is_nm_generated, NMTernary *out_is_volatile, + char **out_shadowed_storage, + NMTernary *out_shadowed_owned, GError **error) { gs_unref_keyfile GKeyFile *key_file = NULL; @@ -210,6 +212,16 @@ nms_keyfile_reader_from_file (const char *full_filename, NM_KEYFILE_KEY_NMMETA_VOLATILE, NM_TERNARY_DEFAULT)); + NM_SET_OUT (out_shadowed_storage, g_key_file_get_string (key_file, + NM_KEYFILE_GROUP_NMMETA, + NM_KEYFILE_KEY_NMMETA_SHADOWED_STORAGE, + NULL)); + + NM_SET_OUT (out_shadowed_owned, nm_key_file_get_boolean (key_file, + NM_KEYFILE_GROUP_NMMETA, + NM_KEYFILE_KEY_NMMETA_SHADOWED_OWNED, + NM_TERNARY_DEFAULT)); + return connection; } diff --git a/src/settings/plugins/keyfile/nms-keyfile-reader.h b/src/settings/plugins/keyfile/nms-keyfile-reader.h index b17b6dd77b..f20e6d93ee 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-reader.h +++ b/src/settings/plugins/keyfile/nms-keyfile-reader.h @@ -37,6 +37,8 @@ NMConnection *nms_keyfile_reader_from_file (const char *full_filename, struct stat *out_stat, NMTernary *out_is_nm_generated, NMTernary *out_is_volatile, + char **out_shadowed_storage, + NMTernary *out_shadowed_owned, GError **error); #endif /* __NMS_KEYFILE_READER_H__ */ diff --git a/src/settings/plugins/keyfile/nms-keyfile-storage.c b/src/settings/plugins/keyfile/nms-keyfile-storage.c index 33ebb8f941..b8bf3e6380 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-storage.c +++ b/src/settings/plugins/keyfile/nms-keyfile-storage.c @@ -50,10 +50,13 @@ nms_keyfile_storage_copy_content (NMSKeyfileStorage *dst, dst->u.meta_data = src->u.meta_data; else { gs_unref_object NMConnection *connection_to_free = NULL; + gs_free char *shadowed_storage_to_free = NULL; connection_to_free = g_steal_pointer (&dst->u.conn_data.connection); + shadowed_storage_to_free = g_steal_pointer (&dst->u.conn_data.shadowed_storage); dst->u.conn_data = src->u.conn_data; nm_g_object_ref (dst->u.conn_data.connection); + dst->u.conn_data.shadowed_storage = g_strdup (dst->u.conn_data.shadowed_storage); } } @@ -159,6 +162,8 @@ nms_keyfile_storage_new_connection (NMSKeyfilePlugin *plugin, NMSKeyfileStorageType storage_type, NMTernary is_nm_generated_opt, NMTernary is_volatile_opt, + const char *shadowed_storage, + NMTernary shadowed_owned_opt, const struct timespec *stat_mtime) { NMSKeyfileStorage *self; @@ -175,12 +180,16 @@ nms_keyfile_storage_new_connection (NMSKeyfilePlugin *plugin, self->u.conn_data.connection = connection_take; /* take reference. */ + self->u.conn_data.shadowed_storage = g_strdup (shadowed_storage); + if (stat_mtime) self->u.conn_data.stat_mtime = *stat_mtime; if (storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN) { self->u.conn_data.is_nm_generated = (is_nm_generated_opt == NM_TERNARY_TRUE); self->u.conn_data.is_volatile = (is_volatile_opt == NM_TERNARY_TRUE); + self->u.conn_data.shadowed_owned = shadowed_storage + && (shadowed_owned_opt == NM_TERNARY_TRUE); } return self; @@ -191,8 +200,11 @@ _storage_clear (NMSKeyfileStorage *self) { c_list_unlink (&self->parent._storage_lst); c_list_unlink (&self->parent._storage_by_uuid_lst); - if (!self->is_meta_data) + if (!self->is_meta_data) { g_clear_object (&self->u.conn_data.connection); + nm_clear_g_free (&self->u.conn_data.shadowed_storage); + self->u.conn_data.shadowed_owned = FALSE; + } } static void diff --git a/src/settings/plugins/keyfile/nms-keyfile-storage.h b/src/settings/plugins/keyfile/nms-keyfile-storage.h index 7968da09ac..5c43e79759 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-storage.h +++ b/src/settings/plugins/keyfile/nms-keyfile-storage.h @@ -51,6 +51,19 @@ typedef struct { struct { NMConnection *connection; + /* when we move a profile from permanent storage to unsaved (/run), then + * we may leave the profile on disk (depending on options for Update2()). + * + * Later, when we save the profile again to disk, we want to re-use that filename. + * Likewise, we delete the (now in-memory) profile, we may want to also delete + * the original filename. + * + * This is the original filename, and we store it inside [.nmmeta] in the + * keyfile in /run. Note that we don't store this in the .nmmeta file, because + * the information is tied to the particular keyfile in /run, not to all UUIDs + * in general. */ + char *shadowed_storage; + /* the timestamp (stat's mtime) of the keyfile. For meta-data this * is irrelevant. The purpose is that if the same storage type (directory) has * multiple files with the same UUID, then the newer file gets preferred. */ @@ -66,6 +79,11 @@ typedef struct { bool is_nm_generated:1; bool is_volatile:1; + /* if shadowed_storage is set, then this flag indicates whether the file + * is owned. The difference comes into play when deleting the in-memory, + * shadowing profile: a owned profile will also be deleted. */ + bool shadowed_owned:1; + } conn_data; /* the content from the .nmmeta file. Note that the nmmeta file has the UUID @@ -106,6 +124,8 @@ NMSKeyfileStorage *nms_keyfile_storage_new_connection (struct _NMSKeyfilePlugin NMSKeyfileStorageType storage_type, NMTernary is_nm_generated_opt, NMTernary is_volatile_opt, + const char *shadowed_storage, + NMTernary shadowed_owned_opt, const struct timespec *stat_mtime); void nms_keyfile_storage_destroy (NMSKeyfileStorage *storage); @@ -188,6 +208,46 @@ nm_settings_storage_is_meta_data_alive (const NMSettingsStorage *storage) return meta_data; } +static inline const char * +nm_settings_storage_get_shadowed_storage (const NMSettingsStorage *storage, + gboolean *out_shadowed_owned) +{ + if (NMS_IS_KEYFILE_STORAGE (storage)) { + const NMSKeyfileStorage *self = (const NMSKeyfileStorage *) storage; + + if (self->storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN) { + if (!self->is_meta_data) { + if (self->u.conn_data.shadowed_storage) { + NM_SET_OUT (out_shadowed_owned, self->u.conn_data.shadowed_owned); + return self->u.conn_data.shadowed_storage; + } + } + } + } + + NM_SET_OUT (out_shadowed_owned, FALSE); + return NULL; +} + +static inline const char * +nm_settings_storage_get_filename_for_shadowed_storage (const NMSettingsStorage *storage) +{ + g_return_val_if_fail (NM_IS_SETTINGS_STORAGE (storage), NULL); + + if (!storage->_filename) + return NULL; + + if (NMS_IS_KEYFILE_STORAGE (storage)) { + const NMSKeyfileStorage *self = (const NMSKeyfileStorage *) storage; + + if ( self->is_meta_data + || self->storage_type != NMS_KEYFILE_STORAGE_TYPE_ETC) + return NULL; + } + + return storage->_filename; +} + /*****************************************************************************/ enum _NMSettingsConnectionIntFlags; diff --git a/src/settings/plugins/keyfile/nms-keyfile-writer.c b/src/settings/plugins/keyfile/nms-keyfile-writer.c index 54e62b7373..5fbbb7a1c8 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-writer.c +++ b/src/settings/plugins/keyfile/nms-keyfile-writer.c @@ -170,6 +170,8 @@ static gboolean _internal_write_connection (NMConnection *connection, gboolean is_nm_generated, gboolean is_volatile, + const char *shadowed_storage, + gboolean shadowed_owned, const char *keyfile_dir, const char *profile_dir, gboolean with_extension, @@ -201,6 +203,8 @@ _internal_write_connection (NMConnection *connection, nm_assert (_nm_connection_verify (connection, NULL) == NM_SETTING_VERIFY_SUCCESS); + nm_assert (!shadowed_owned || shadowed_storage); + rename = force_rename || existing_path_read_only || ( existing_path @@ -229,6 +233,20 @@ _internal_write_connection (NMConnection *connection, TRUE); } + if (shadowed_storage) { + g_key_file_set_string (kf_file, + NM_KEYFILE_GROUP_NMMETA, + NM_KEYFILE_KEY_NMMETA_SHADOWED_STORAGE, + shadowed_storage); + } + + if (shadowed_owned) { + g_key_file_set_boolean (kf_file, + NM_KEYFILE_GROUP_NMMETA, + NM_KEYFILE_KEY_NMMETA_SHADOWED_OWNED, + TRUE); + } + kf_content_buf = g_key_file_to_data (kf_file, &kf_content_len, error); if (!kf_content_buf) return FALSE; @@ -356,6 +374,8 @@ gboolean nms_keyfile_writer_connection (NMConnection *connection, gboolean is_nm_generated, gboolean is_volatile, + const char *shadowed_storage, + gboolean shadowed_owned, const char *keyfile_dir, const char *profile_dir, const char *existing_path, @@ -371,6 +391,8 @@ nms_keyfile_writer_connection (NMConnection *connection, return _internal_write_connection (connection, is_nm_generated, is_volatile, + shadowed_storage, + shadowed_owned, keyfile_dir, profile_dir, TRUE, @@ -400,6 +422,8 @@ nms_keyfile_writer_test_connection (NMConnection *connection, return _internal_write_connection (connection, FALSE, FALSE, + NULL, + FALSE, keyfile_dir, keyfile_dir, FALSE, diff --git a/src/settings/plugins/keyfile/nms-keyfile-writer.h b/src/settings/plugins/keyfile/nms-keyfile-writer.h index 4fb9a20638..99e86025ae 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-writer.h +++ b/src/settings/plugins/keyfile/nms-keyfile-writer.h @@ -29,6 +29,8 @@ typedef gboolean (*NMSKeyfileWriterAllowFilenameCb) (const char *check_filename, gboolean nms_keyfile_writer_connection (NMConnection *connection, gboolean is_nm_generated, gboolean is_volatile, + const char *shadowed_storage, + gboolean shadowed_owned, const char *keyfile_dir, const char *profile_dir, const char *existing_path, diff --git a/src/settings/plugins/keyfile/tests/test-keyfile-settings.c b/src/settings/plugins/keyfile/tests/test-keyfile-settings.c index 5942f04bb1..ce7540ebda 100644 --- a/src/settings/plugins/keyfile/tests/test-keyfile-settings.c +++ b/src/settings/plugins/keyfile/tests/test-keyfile-settings.c @@ -75,6 +75,8 @@ check_ip_route (NMSettingIPConfig *config, int idx, const char *destination, int NULL, \ NULL, \ NULL, \ + NULL, \ + NULL, \ (nmtst_get_rand_uint32 () % 2) ? &_error : NULL); \ nmtst_assert_success (_connection, _error); \ nmtst_assert_connection_verifies_without_normalization (_connection); \ From 064544cc07878caa248c3557c47f91736e5d2af1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Jul 2019 16:26:16 +0200 Subject: [PATCH 06/14] settings: support storing "shadowed-storage" to .nmmeta files Before, the .nmmeta file could only contain one piece of information: the loaded-path. This was persisted to disk by writing a "$UUID.nmmeta" symlink that links to the loaded-path. Also, in practice this is used for tombstones, so the only valid loaded-path is "/dev/null" (all other paths are ignored). Extend the .nmmeta file format to also be able to store additional data: the shadowed-storage path. We will need that later but the idea is that if we have a tombstone on disk, then this tombstone might explicitly shadow another file. The use is when re-adding a profile with the same UUID, then the existing storage is used (instead of creating a new file). This will be necessary with Update2(NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED) flag. This flag first allows to clone a profile from persistent storage to a profile in /run. Later, when this profile gets deleted, the original profile will be left on disk. If the same profile then gets re-created with AddConnection(), then the original filename must be taken over again. This is to avoid duplication of profiles on disk. Note that this piece of information is relevent per-UUID, and as such it's correct to store it in the .nmmeta file. That is related to the "shadowed-storage" information that we store in the [.nmmeta] section of keyfiles. --- src/settings/nm-settings.c | 3 + .../plugins/keyfile/nms-keyfile-plugin.c | 22 +++- .../plugins/keyfile/nms-keyfile-plugin.h | 1 + .../plugins/keyfile/nms-keyfile-storage.c | 17 ++- .../plugins/keyfile/nms-keyfile-storage.h | 7 +- .../plugins/keyfile/nms-keyfile-utils.c | 111 ++++++++++++++---- .../plugins/keyfile/nms-keyfile-utils.h | 7 +- .../keyfile/tests/test-keyfile-settings.c | 6 +- 8 files changed, 137 insertions(+), 37 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 8679128a23..7dadd4e3e3 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1914,6 +1914,7 @@ nm_settings_delete_connection (NMSettings *self, uuid, FALSE, TRUE, + NULL, &tombstone_1_storage, NULL)) tombstone_in_memory = TRUE; @@ -1927,6 +1928,7 @@ nm_settings_delete_connection (NMSettings *self, uuid, TRUE, TRUE, + NULL, &tombstone_2_storage, NULL)) { nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, @@ -1934,6 +1936,7 @@ nm_settings_delete_connection (NMSettings *self, uuid, TRUE, TRUE, + NULL, &tombstone_2_storage, NULL); } diff --git a/src/settings/plugins/keyfile/nms-keyfile-plugin.c b/src/settings/plugins/keyfile/nms-keyfile-plugin.c index eb3ae5f100..fbe70ef4d2 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-plugin.c +++ b/src/settings/plugins/keyfile/nms-keyfile-plugin.c @@ -309,6 +309,7 @@ _load_file (NMSKeyfilePlugin *self, if (_ignore_filename (storage_type, filename)) { gs_free char *nmmeta = NULL; gs_free char *loaded_path = NULL; + gs_free char *shadowed_storage_filename = NULL; if (!nms_keyfile_nmmeta_check_filename (filename, NULL)) { if (error) @@ -322,6 +323,7 @@ _load_file (NMSKeyfilePlugin *self, &full_filename, &nmmeta, &loaded_path, + &shadowed_storage_filename, NULL)) { if (error) nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, "skip unreadable nmmeta file"); @@ -349,7 +351,8 @@ _load_file (NMSKeyfilePlugin *self, return nms_keyfile_storage_new_tombstone (self, nmmeta, full_filename, - storage_type); + storage_type, + shadowed_storage_filename); } full_filename = g_build_filename (dirname, filename, NULL); @@ -1052,6 +1055,9 @@ delete_connection (NMSettingsPlugin *plugin, * has no /etc directory configured, this results in a hard failure. * @set: if %TRUE, write the symlink to point to /dev/null. If %FALSE, * delete the nmmeta file (if it exists). + * @shadowed_storage: a tombstone can also shadow an existing storage. + * In combination with @set and @in_memory, this is allowed to store + * the shadowed storage filename. * @out_storage: (transfer full) (allow-none): the storage element that changes, or * NULL if nothing changed. Note that the file on disk is already as * we want to write it, then this still counts as a change. No change only @@ -1076,6 +1082,7 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, const char *uuid, gboolean in_memory, gboolean set, + const char *shadowed_storage, NMSettingsStorage **out_storage, gboolean *out_hard_failure) { @@ -1092,6 +1099,7 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, nm_assert (NMS_IS_KEYFILE_PLUGIN (self)); nm_assert (nm_utils_is_uuid (uuid)); nm_assert (!out_storage || !*out_storage); + nm_assert (!shadowed_storage || (set && in_memory)); priv = NMS_KEYFILE_PLUGIN_GET_PRIVATE (self); @@ -1104,7 +1112,7 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, dirname = priv->dirname_run; } else { if (!priv->dirname_etc) { - _LOGT ("commit: cannot %s%s nmmeta symlink for %s as there is no /etc directory", + _LOGT ("commit: cannot %s%s nmmeta file for %s as there is no /etc directory", simulate ? "simulate " : "", loaded_path ? "write" : "delete", uuid); @@ -1123,13 +1131,15 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, uuid, loaded_path, FALSE, + shadowed_storage, &nmmeta_filename); } - _LOGT ("commit: %s nmmeta symlink \"%s\"%s%s%s %s", + _LOGT ("commit: %s nmmeta file \"%s\"%s%s%s%s%s%s %s", loaded_path ? "writing" : "deleting", nmmeta_filename, NM_PRINT_FMT_QUOTED (loaded_path, " (pointing to \"", loaded_path, "\")", ""), + NM_PRINT_FMT_QUOTED (shadowed_storage, " (shadows \"", shadowed_storage, "\")", ""), simulate ? "simulated" : ( nmmeta_success @@ -1152,8 +1162,12 @@ nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, storage = nms_keyfile_storage_new_tombstone (self, uuid, nmmeta_filename, - storage_type); + storage_type, + shadowed_storage); nm_sett_util_storages_add_take (&priv->storages, storage); + } else { + g_free (storage->u.meta_data.shadowed_storage); + storage->u.meta_data.shadowed_storage = g_strdup (shadowed_storage); } storage_result = g_object_ref (storage); diff --git a/src/settings/plugins/keyfile/nms-keyfile-plugin.h b/src/settings/plugins/keyfile/nms-keyfile-plugin.h index b2a4a23864..4844096456 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-plugin.h +++ b/src/settings/plugins/keyfile/nms-keyfile-plugin.h @@ -68,6 +68,7 @@ gboolean nms_keyfile_plugin_set_nmmeta_tombstone (NMSKeyfilePlugin *self, const char *uuid, gboolean in_memory, gboolean set, + const char *shadowed_storage, NMSettingsStorage **out_storage, gboolean *out_hard_failure); diff --git a/src/settings/plugins/keyfile/nms-keyfile-storage.c b/src/settings/plugins/keyfile/nms-keyfile-storage.c index b8bf3e6380..d68d60c8ec 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-storage.c +++ b/src/settings/plugins/keyfile/nms-keyfile-storage.c @@ -46,9 +46,13 @@ nms_keyfile_storage_copy_content (NMSKeyfileStorage *dst, nm_assert (dst->storage_type == src->storage_type); nm_assert (dst->is_meta_data == src->is_meta_data); - if (dst->is_meta_data) + if (dst->is_meta_data) { + gs_free char *shadowed_storage_to_free = NULL; + + shadowed_storage_to_free = g_steal_pointer (&dst->u.meta_data.shadowed_storage); dst->u.meta_data = src->u.meta_data; - else { + dst->u.meta_data.shadowed_storage = g_strdup (dst->u.meta_data.shadowed_storage); + } else { gs_unref_object NMConnection *connection_to_free = NULL; gs_free char *shadowed_storage_to_free = NULL; @@ -140,7 +144,8 @@ NMSKeyfileStorage * nms_keyfile_storage_new_tombstone (NMSKeyfilePlugin *plugin, const char *uuid, const char *filename, - NMSKeyfileStorageType storage_type) + NMSKeyfileStorageType storage_type, + const char *shadowed_storage) { NMSKeyfileStorage *self; @@ -152,6 +157,8 @@ nms_keyfile_storage_new_tombstone (NMSKeyfilePlugin *plugin, self = _storage_new (plugin, uuid, filename, TRUE, storage_type); self->u.meta_data.is_tombstone = TRUE; + if (storage_type == NMS_KEYFILE_STORAGE_TYPE_RUN) + self->u.meta_data.shadowed_storage = g_strdup (shadowed_storage); return self; } @@ -200,7 +207,9 @@ _storage_clear (NMSKeyfileStorage *self) { c_list_unlink (&self->parent._storage_lst); c_list_unlink (&self->parent._storage_by_uuid_lst); - if (!self->is_meta_data) { + if (self->is_meta_data) + nm_clear_g_free (&self->u.meta_data.shadowed_storage); + else { g_clear_object (&self->u.conn_data.connection); nm_clear_g_free (&self->u.conn_data.shadowed_storage); self->u.conn_data.shadowed_owned = FALSE; diff --git a/src/settings/plugins/keyfile/nms-keyfile-storage.h b/src/settings/plugins/keyfile/nms-keyfile-storage.h index 5c43e79759..2252b47b7e 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-storage.h +++ b/src/settings/plugins/keyfile/nms-keyfile-storage.h @@ -35,6 +35,7 @@ typedef struct { /* whether this is a tombstone to hide a UUID (via symlink to /dev/null). */ + char *shadowed_storage; bool is_tombstone:1; } NMSettingsMetaData; @@ -116,7 +117,8 @@ struct _NMSKeyfilePlugin; NMSKeyfileStorage *nms_keyfile_storage_new_tombstone (struct _NMSKeyfilePlugin *self, const char *uuid, const char *filename, - NMSKeyfileStorageType storage_type); + NMSKeyfileStorageType storage_type, + const char *shadowed_storage); NMSKeyfileStorage *nms_keyfile_storage_new_connection (struct _NMSKeyfilePlugin *self, NMConnection *connection_take /* pass reference */, @@ -221,6 +223,9 @@ nm_settings_storage_get_shadowed_storage (const NMSettingsStorage *storage, NM_SET_OUT (out_shadowed_owned, self->u.conn_data.shadowed_owned); return self->u.conn_data.shadowed_storage; } + } else { + NM_SET_OUT (out_shadowed_owned, FALSE); + return self->u.meta_data.shadowed_storage; } } } diff --git a/src/settings/plugins/keyfile/nms-keyfile-utils.c b/src/settings/plugins/keyfile/nms-keyfile-utils.c index e9d5351573..ea03e1b611 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/settings/plugins/keyfile/nms-keyfile-utils.c @@ -24,6 +24,7 @@ #include #include +#include "nm-glib-aux/nm-io-utils.h" #include "nm-keyfile-internal.h" #include "nm-utils.h" #include "nm-setting-wired.h" @@ -33,6 +34,13 @@ /*****************************************************************************/ +#define NMMETA_KF_GROUP_NAME_NMMETA "nmmeta" +#define NMMETA_KF_KEY_NAME_NMMETA_UUID "uuid" +#define NMMETA_KF_KEY_NAME_NMMETA_LOADED_PATH "loaded-path" +#define NMMETA_KF_KEY_NAME_NMMETA_SHADOWED_STORAGE "shadowed-storage" + +/*****************************************************************************/ + const char * nms_keyfile_nmmeta_check_filename (const char *filename, guint *out_uuid_len) @@ -106,12 +114,16 @@ nms_keyfile_nmmeta_read (const char *dirname, char **out_full_filename, char **out_uuid, char **out_loaded_path, + char **out_shadowed_storage, struct stat *out_st) { const char *uuid; guint uuid_len; gs_free char *full_filename = NULL; - gs_free char *ln = NULL; + gs_free char *loaded_path = NULL; + gs_free char *shadowed_storage = NULL; + struct stat st_stack; + struct stat *st = out_st ?: &st_stack; nm_assert (dirname && dirname[0] == '/'); nm_assert (filename && filename[0] && !strchr (filename, '/')); @@ -124,17 +136,43 @@ nms_keyfile_nmmeta_read (const char *dirname, if (!nms_keyfile_utils_check_file_permissions (NMS_KEYFILE_FILETYPE_NMMETA, full_filename, - out_st, + st, NULL)) return FALSE; - ln = nm_utils_read_link_absolute (full_filename, NULL); - if (!ln) - return FALSE; + if (S_ISREG (st->st_mode)) { + gs_unref_keyfile GKeyFile *kf = NULL; + gs_free char *v_uuid = NULL; + + kf = g_key_file_new (); + + if (!g_key_file_load_from_file (kf, full_filename, G_KEY_FILE_NONE, NULL)) + return FALSE; + + v_uuid = g_key_file_get_string (kf, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_UUID, NULL); + if (!nm_streq0 (v_uuid, uuid)) + return FALSE; + + loaded_path = g_key_file_get_string (kf, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_LOADED_PATH, NULL); + shadowed_storage = g_key_file_get_string (kf, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_SHADOWED_STORAGE, NULL); + + if ( !loaded_path + && !shadowed_storage) { + /* if there is no useful information in the file, it is the same as if + * the file is not present. Signal failure. */ + return FALSE; + } + + } else { + loaded_path = nm_utils_read_link_absolute (full_filename, NULL); + if (!loaded_path) + return FALSE; + } NM_SET_OUT (out_uuid, g_strndup (uuid, uuid_len)); NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename)); - NM_SET_OUT (out_loaded_path, g_steal_pointer (&ln)); + NM_SET_OUT (out_loaded_path, g_steal_pointer (&loaded_path)); + NM_SET_OUT (out_shadowed_storage, g_steal_pointer (&shadowed_storage)); return TRUE; } @@ -143,7 +181,8 @@ nms_keyfile_nmmeta_read_from_file (const char *full_filename, char **out_dirname, char **out_filename, char **out_uuid, - char **out_loaded_path) + char **out_loaded_path, + char **out_shadowed_storage) { gs_free char *dirname = NULL; gs_free char *filename = NULL; @@ -158,6 +197,7 @@ nms_keyfile_nmmeta_read_from_file (const char *full_filename, NULL, out_uuid, out_loaded_path, + out_shadowed_storage, NULL)) return FALSE; @@ -170,7 +210,8 @@ gboolean nms_keyfile_nmmeta_write (const char *dirname, const char *uuid, const char *loaded_path, - gboolean allow_relative, + gboolean loaded_path_allow_relative, + const char *shadowed_storage, char **out_full_filename) { gs_free char *full_filename_tmp = NULL; @@ -180,6 +221,7 @@ nms_keyfile_nmmeta_write (const char *dirname, nm_assert ( nm_utils_is_uuid (uuid) && !strchr (uuid, '/')); nm_assert (!loaded_path || loaded_path[0] == '/'); + nm_assert (!shadowed_storage || loaded_path); full_filename_tmp = nms_keyfile_nmmeta_filename (dirname, uuid, TRUE); @@ -198,7 +240,7 @@ nms_keyfile_nmmeta_write (const char *dirname, return success; } - if (allow_relative) { + if (loaded_path_allow_relative) { const char *f; f = nm_utils_file_is_in_path (loaded_path, dirname); @@ -209,18 +251,40 @@ nms_keyfile_nmmeta_write (const char *dirname, } } - if (symlink (loaded_path, full_filename_tmp) != 0) { - full_filename_tmp[strlen (full_filename_tmp) - 1] = '\0'; - NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename_tmp)); - return FALSE; - } + full_filename = g_strndup (full_filename_tmp, strlen (full_filename_tmp) - 1); - full_filename = g_strdup (full_filename_tmp); - full_filename[strlen (full_filename) - 1] = '\0'; - if (rename (full_filename_tmp, full_filename) != 0) { - (void) unlink (full_filename_tmp); - NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename)); - return FALSE; + if (shadowed_storage) { + gs_unref_keyfile GKeyFile *kf = NULL; + gs_free char *contents = NULL; + gsize length; + + kf = g_key_file_new (); + + g_key_file_set_string (kf, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_UUID, uuid); + g_key_file_set_string (kf, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_LOADED_PATH, loaded_path); + g_key_file_set_string (kf, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_SHADOWED_STORAGE, shadowed_storage); + + contents = g_key_file_to_data (kf, &length, NULL); + + if (!nm_utils_file_set_contents (full_filename, contents, length, 0600, NULL)) { + NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename_tmp)); + return FALSE; + } + } else { + /* we only have the "loaded_path" to store. That is commonly used for the tombstones to + * link to /dev/null. A symlink is sufficient to store that ammount of information. + * No need to bother with a keyfile. */ + if (symlink (loaded_path, full_filename_tmp) != 0) { + full_filename_tmp[strlen (full_filename_tmp) - 1] = '\0'; + NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename_tmp)); + return FALSE; + } + + if (rename (full_filename_tmp, full_filename) != 0) { + (void) unlink (full_filename_tmp); + NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename)); + return FALSE; + } } NM_SET_OUT (out_full_filename, g_steal_pointer (&full_filename)); @@ -243,9 +307,10 @@ nms_keyfile_utils_check_file_permissions_stat (NMSKeyfileFiletype filetype, return FALSE; } } else if (filetype == NMS_KEYFILE_FILETYPE_NMMETA) { - if (!S_ISLNK (st->st_mode)) { + if ( !S_ISLNK (st->st_mode) + && !S_ISREG (st->st_mode)) { g_set_error_literal (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "file is not a slink"); + "file is neither a symlink nor a regular file"); return FALSE; } } else @@ -259,7 +324,7 @@ nms_keyfile_utils_check_file_permissions_stat (NMSKeyfileFiletype filetype, return FALSE; } - if ( filetype == NMS_KEYFILE_FILETYPE_KEYFILE + if ( S_ISREG (st->st_mode) && (st->st_mode & 0077)) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "File permissions (%03o) are insecure", diff --git a/src/settings/plugins/keyfile/nms-keyfile-utils.h b/src/settings/plugins/keyfile/nms-keyfile-utils.h index 4f4ad54091..723c443625 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-utils.h +++ b/src/settings/plugins/keyfile/nms-keyfile-utils.h @@ -56,18 +56,21 @@ gboolean nms_keyfile_nmmeta_read (const char *dirname, char **out_full_filename, char **out_uuid, char **out_loaded_path, + char **out_shadowed_storage, struct stat *out_st); gboolean nms_keyfile_nmmeta_read_from_file (const char *full_filename, char **out_dirname, char **out_filename, char **out_uuid, - char **out_loaded_path); + char **out_loaded_path, + char **out_shadowed_storage); gboolean nms_keyfile_nmmeta_write (const char *dirname, const char *uuid, const char *loaded_path, - gboolean allow_relative, + gboolean loaded_path_allow_relative, + const char *shadowed_storage, char **out_full_filename); /*****************************************************************************/ diff --git a/src/settings/plugins/keyfile/tests/test-keyfile-settings.c b/src/settings/plugins/keyfile/tests/test-keyfile-settings.c index ce7540ebda..f96111a2e4 100644 --- a/src/settings/plugins/keyfile/tests/test-keyfile-settings.c +++ b/src/settings/plugins/keyfile/tests/test-keyfile-settings.c @@ -2543,7 +2543,7 @@ _assert_keyfile_nmmeta (const char *dirname, nm_clear_g_free (&full_filename); - g_assert (nms_keyfile_nmmeta_write (dirname, uuid, loaded_path, allow_relative, &full_filename)); + g_assert (nms_keyfile_nmmeta_write (dirname, uuid, loaded_path, allow_relative, NULL, &full_filename)); g_assert_cmpstr (full_filename, ==, exp_full_filename); nm_clear_g_free (&full_filename); @@ -2555,7 +2555,7 @@ _assert_keyfile_nmmeta (const char *dirname, g_assert_cmpstr (symlink_target, ==, exp_symlink_target); - success = nms_keyfile_nmmeta_read (dirname, filename, &full_filename, &uuid2, &loaded_path2, NULL); + success = nms_keyfile_nmmeta_read (dirname, filename, &full_filename, &uuid2, &loaded_path2, NULL, NULL); g_assert_cmpint (!!exp_uuid, ==, success); if (success) g_assert_cmpstr (full_filename, ==, exp_full_filename); @@ -2566,7 +2566,7 @@ _assert_keyfile_nmmeta (const char *dirname, g_assert_cmpstr (loaded_path2, ==, exp_loaded_path); - success = nms_keyfile_nmmeta_read_from_file (exp_full_filename, &dirname3, &filename3, &uuid3, &loaded_path3); + success = nms_keyfile_nmmeta_read_from_file (exp_full_filename, &dirname3, &filename3, &uuid3, &loaded_path3, NULL); g_assert_cmpint (!!exp_uuid, ==, success); if (success) { g_assert_cmpstr (dirname3, ==, dirname); From 82d5845eb58c451d1b6529b8a72a9472a22b83d1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Jul 2019 16:54:52 +0200 Subject: [PATCH 07/14] settings: minor refactoring handling NMSettingsUpdate2Flags in "nm-settings-connection.c" --- src/settings/nm-settings-connection.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index f042feac52..5da772ed96 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -44,6 +44,11 @@ #define AUTOCONNECT_RETRIES_FOREVER -1 #define AUTOCONNECT_RESET_RETRIES_TIMER 300 +#define _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES ((NMSettingsUpdate2Flags) ( NM_SETTINGS_UPDATE2_FLAG_TO_DISK \ + | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY \ + | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED \ + | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY)) + /*****************************************************************************/ NMConnection ** @@ -1504,6 +1509,9 @@ update_auth_cb (NMSettingsConnection *self, } } + nm_assert ( !NM_FLAGS_ANY (info->flags, _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES) + || nm_utils_is_power_of_two (info->flags & _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES)); + if (NM_FLAGS_HAS (info->flags, NM_SETTINGS_UPDATE2_FLAG_TO_DISK)) persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; else if (NM_FLAGS_ANY (info->flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY @@ -1725,14 +1733,10 @@ impl_settings_connection_update2 (NMDBusObject *obj, GVariantIter iter; const char *args_name; NMSettingsUpdate2Flags flags; - const NMSettingsUpdate2Flags ALL_PERSIST_MODES = NM_SETTINGS_UPDATE2_FLAG_TO_DISK - | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY - | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED - | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY; g_variant_get (parameters, "(@a{sa{sv}}u@a{sv})", &settings, &flags_u, &args); - if (NM_FLAGS_ANY (flags_u, ~((guint32) ( ALL_PERSIST_MODES + if (NM_FLAGS_ANY (flags_u, ~((guint32) ( _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES | NM_SETTINGS_UPDATE2_FLAG_VOLATILE | NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT | NM_SETTINGS_UPDATE2_FLAG_NO_REAPPLY)))) { @@ -1745,11 +1749,11 @@ impl_settings_connection_update2 (NMDBusObject *obj, flags = (NMSettingsUpdate2Flags) flags_u; - if ( ( NM_FLAGS_ANY (flags, ALL_PERSIST_MODES) - && !nm_utils_is_power_of_two (flags & ALL_PERSIST_MODES)) + if ( ( NM_FLAGS_ANY (flags, _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES) + && !nm_utils_is_power_of_two (flags & _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES)) || ( NM_FLAGS_HAS (flags, NM_SETTINGS_UPDATE2_FLAG_VOLATILE) - && !NM_FLAGS_ANY (flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED | - NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY))) { + && !NM_FLAGS_ANY (flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED + | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY))) { error = g_error_new_literal (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_ARGUMENTS, "Conflicting flags"); From ea9627b9ea8ad45453f7d587c89b3c60582ae88b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Jul 2019 17:16:45 +0200 Subject: [PATCH 08/14] settings: refactor call to nm_settings_plugin_update_connection() in "nm-settings.c" The function will be re-used later, because also during "add-connection" we might need to update an existing storage instead of creating a new one. --- src/settings/nm-settings.c | 97 ++++++++++++++++++++++++-------------- 1 file changed, 61 insertions(+), 36 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 7dadd4e3e3..c4cc1d8fd3 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1282,8 +1282,7 @@ static gboolean _add_connection_to_first_plugin (NMSettings *self, NMConnection *new_connection, gboolean in_memory, - gboolean is_nm_generated, - gboolean is_volatile, + NMSettingsConnectionIntFlags sett_flags, const char *shadowed_storage, gboolean shadowed_owned, NMSettingsStorage **out_new_storage, @@ -1314,8 +1313,8 @@ _add_connection_to_first_plugin (NMSettings *self, success = nms_keyfile_plugin_add_connection (priv->keyfile_plugin, new_connection, in_memory, - is_nm_generated, - is_volatile, + NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), + NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), shadowed_storage, shadowed_owned, &storage, @@ -1324,8 +1323,8 @@ _add_connection_to_first_plugin (NMSettings *self, } else { if (in_memory) continue; - nm_assert (!is_nm_generated); - nm_assert (!is_volatile); + nm_assert (!NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED)); + nm_assert (!NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE)); success = nm_settings_plugin_add_connection (plugin, new_connection, &storage, @@ -1379,6 +1378,50 @@ _add_connection_to_first_plugin (NMSettings *self, return FALSE; } +static gboolean +_update_connection_to_plugin (NMSettings *self, + NMSettingsStorage *storage, + NMConnection *connection, + NMSettingsConnectionIntFlags sett_flags, + gboolean force_rename, + const char *shadowed_storage, + gboolean shadowed_owned, + NMSettingsStorage **out_new_storage, + NMConnection **out_new_connection, + GError **error) +{ + NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); + NMSettingsPlugin *plugin; + gboolean success; + + plugin = nm_settings_storage_get_plugin (storage); + + if (plugin == (NMSettingsPlugin *) priv->keyfile_plugin) { + success = nms_keyfile_plugin_update_connection (priv->keyfile_plugin, + storage, + connection, + NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), + NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), + shadowed_storage, + shadowed_owned, + force_rename, + out_new_storage, + out_new_connection, + error); + } else { + nm_assert (!shadowed_storage); + nm_assert (!shadowed_owned); + success = nm_settings_plugin_update_connection (plugin, + storage, + connection, + out_new_storage, + out_new_connection, + error); + } + + return success; +} + /** * nm_settings_add_connection: * @self: the #NMSettings object @@ -1459,8 +1502,7 @@ nm_settings_add_connection (NMSettings *self, ( persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK || NM_FLAGS_ANY (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE | NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED)), - NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), - NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), + sett_flags, NULL, FALSE, &new_storage, @@ -1534,7 +1576,6 @@ nm_settings_update_connection (NMSettings *self, const char *log_context_name, GError **error) { - NMSettingsPrivate *priv; gs_unref_object NMConnection *connection_cloned_1 = NULL; gs_unref_object NMConnection *new_connection_cloned = NULL; gs_unref_object NMConnection *new_connection = NULL; @@ -1559,8 +1600,6 @@ nm_settings_update_connection (NMSettings *self, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)); - priv = NM_SETTINGS_GET_PRIVATE (self); - cur_storage = g_object_ref (nm_settings_connection_get_storage (sett_conn)); uuid = nm_settings_storage_get_uuid (cur_storage); @@ -1697,37 +1736,23 @@ nm_settings_update_connection (NMSettings *self, success = _add_connection_to_first_plugin (self, connection, new_in_memory, - NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), - NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), + sett_flags, shadowed_storage, shadowed_owned, &new_storage, &new_connection, &local); } else { - NMSettingsPlugin *plugin; - - plugin = nm_settings_storage_get_plugin (cur_storage); - if (plugin == (NMSettingsPlugin *) priv->keyfile_plugin) { - success = nms_keyfile_plugin_update_connection (priv->keyfile_plugin, - cur_storage, - connection, - NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED), - NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE), - shadowed_storage, - shadowed_owned, - NM_FLAGS_HAS (update_reason, NM_SETTINGS_CONNECTION_UPDATE_REASON_FORCE_RENAME), - &new_storage, - &new_connection, - &local); - } else { - success = nm_settings_plugin_update_connection (nm_settings_storage_get_plugin (cur_storage), - cur_storage, - connection, - &new_storage, - &new_connection, - &local); - } + success = _update_connection_to_plugin (self, + cur_storage, + connection, + sett_flags, + update_reason, + shadowed_storage, + shadowed_owned, + &new_storage, + &new_connection, + &local); } if (!success) { gboolean ignore_failure; From ea5813ebf042c55b8d2a63eabf934e9ebf90ad63 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Jul 2019 17:41:42 +0200 Subject: [PATCH 09/14] settings: log information about shadowed-storage for change events --- src/settings/nm-settings.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index c4cc1d8fd3..5a2417b043 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1189,26 +1189,27 @@ _connection_changed_track (NMSettings *self, if (_LOGT_ENABLED ()) { const char *filename; const NMSettingsMetaData *meta_data; + const char *shadowed_storage; + gboolean shadowed_owned; filename = nm_settings_storage_get_filename (storage); if (connection) { - _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event with connection \"%s\"%s%s%s", + shadowed_storage = nm_settings_storage_get_shadowed_storage (storage, &shadowed_owned); + _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event with connection \"%s\"%s%s%s%s%s%s", sett_conn_entry->uuid, NM_SETTINGS_STORAGE_PRINT_ARG (storage), nm_connection_get_id (connection), - NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); + NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", ""), + NM_PRINT_FMT_QUOTED (shadowed_storage, shadowed_owned ? " (owns \"" : " (shadows \"", shadowed_storage, "\")", "")); } else if ((meta_data = nm_settings_storage_is_meta_data (storage))) { - if (meta_data->is_tombstone) { - _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event for hiding profile%s%s%s", - sett_conn_entry->uuid, - NM_SETTINGS_STORAGE_PRINT_ARG (storage), - NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); - } else { - _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event with meta data for profile%s%s%s", - sett_conn_entry->uuid, - NM_SETTINGS_STORAGE_PRINT_ARG (storage), - NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); - } + nm_assert (meta_data->is_tombstone); + shadowed_storage = nm_settings_storage_get_shadowed_storage (storage, &shadowed_owned); + _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event for %shiding profile%s%s%s%s%s%s", + sett_conn_entry->uuid, + NM_SETTINGS_STORAGE_PRINT_ARG (storage), + nm_settings_storage_is_meta_data_alive (storage) ? "" : "dropping ", + NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", ""), + NM_PRINT_FMT_QUOTED (shadowed_storage, shadowed_owned ? " (owns \"" : " (shadows \"", shadowed_storage, "\")", "")); } else { _LOGT ("storage[%s,"NM_SETTINGS_STORAGE_PRINT_FMT"]: change event for dropping profile%s%s%s", sett_conn_entry->uuid, From c3bf51a9b21fb5ea1d7e09c79f44b2e265d60a7d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Jul 2019 18:47:14 +0200 Subject: [PATCH 10/14] settings: avoid adding a profile that would be hidden by an existing profile Say, you configure "plugins=ifcfg-rh,keyfile" and have an ifcfg file for a certain profile. Then you make it IN-MEMORY-DETACHED and delete it. The result is that the ifcfg file still exists, but there is a tombstone nmmeta file that shadows the profile. Now, if you want to re-add the same profile (same UUID) and store it to persistent storage, then first it will try to persist the profile via the ifcfg-rh plugin. That may not be possible, for example because the connection type is not supported by the plugin. Now, you could write it to /etc as keyfile, but then there would still be a higher priority profile. Note that after calling _add_connection_to_first_plugin() we issue _connection_changed_track (self, new_storage, new_connection, TRUE); (note the prioritize=TRUE parameter). So, in the first moment, all looks good. However it is not because upon first reload the change gets reverted and the other profile resurfaces. The problem are that all settings plugins (except keyfile) may be completely unable to persist a profile. The real and only solution is not to use any settings plugins except keyfile. In a previous version (that never was merged), the "loaded-path" of nmmeta files was used to prioritize profiles. Since that was not done, we should not add profiles that we know have lower priority than existing profiles. --- src/settings/nm-settings.c | 104 ++++++++++++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 2 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 5a2417b043..4075ca6040 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -228,6 +228,81 @@ _sett_conn_entry_get_conn (SettConnEntry *sett_conn_entry) return sett_conn_entry ? sett_conn_entry->sett_conn : NULL; } +/** + * _sett_conn_entry_storage_find_conflicting_storage: + * @sett_conn_entry: the list of settings-storages for the given UUID. + * @target_plugin: the settings plugin to check + * @storage_check_including: (allow-none): optionally compare against this storage. + * @plugins: the list of plugins sorted in descending priority. This determines + * the priority and whether a storage conflicts. + * + * If we were to add the a storage to @target_plugin, then this function checks + * whether there are already other storages that would hide the storage after we + * add it. Those conflicting/hiding storages are a problem, because they have higher + * priority, so we cannot add the storage. + * + * @storage_check_including is optional, and if given then it checks whether updating + * the profile in this storage would result in confict. This is the check before + * update-connection. If this parameter is omitted, then it's about what happens + * when adding a new profile (add-connection). + * + * Returns: the conflicting storage or %NULL if there is none. + */ +static NMSettingsStorage * +_sett_conn_entry_storage_find_conflicting_storage (SettConnEntry *sett_conn_entry, + NMSettingsPlugin *target_plugin, + NMSettingsStorage *storage_check_including, + const GSList *plugins) +{ + StorageData *sd; + + if (!sett_conn_entry) + return NULL; + + if ( storage_check_including + && nm_settings_storage_is_keyfile_run (storage_check_including)) { + /* the storage we check against is in-memory. It always has highest + * priority, so there can be no other conflicting storages. */ + return NULL; + } + + /* Finds the first (highest priority) storage that has a connection. + * Note that due to tombstones (that have a high priority), the connection + * may not actually be exposed. This is to find hidden/shadowed storages + * that provide a connection. */ + c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { + nm_assert (NM_IS_SETTINGS_STORAGE (sd->storage)); + + if (!sd->connection) { + /* We only consider storages with connection. In particular, + * tombstones are not relevant, because we can delete them to + * resolve the conflict. */ + continue; + } + + if (sd->storage == storage_check_including) { + /* ok, the storage is the one we are about to check. All other + * storages are lower priority, so there is no storage that hides + * our storage_check_including. */ + return NULL; + } + + if (nm_settings_plugin_cmp_by_priority (nm_settings_storage_get_plugin (sd->storage), + target_plugin, + plugins) <= 0) { + /* the plugin of the existing storage is less important than @target_plugin. + * We have no conflicting/hiding storage. */ + return NULL; + } + + /* Found. If we would add the profile to @target_plugin, then it would be hidden + * by existing_storage. */ + return sd->storage; + } + + return NULL; +} + /*****************************************************************************/ NM_GOBJECT_PROPERTIES_DEFINE (NMSettings, @@ -1281,6 +1356,7 @@ _plugin_connections_reload (NMSettings *self) static gboolean _add_connection_to_first_plugin (NMSettings *self, + SettConnEntry *sett_conn_entry, NMConnection *new_connection, gboolean in_memory, NMSettingsConnectionIntFlags sett_flags, @@ -1310,6 +1386,28 @@ _add_connection_to_first_plugin (NMSettings *self, gboolean success; const char *filename; + if (!in_memory) { + NMSettingsStorage *conflicting_storage; + + conflicting_storage = _sett_conn_entry_storage_find_conflicting_storage (sett_conn_entry, plugin, NULL, priv->plugins); + if (conflicting_storage) { + /* we have a connection provided by a plugin with higher priority than the one + * we would want to add the connection. We cannot do that, because doing so + * would result in adding a connection that gets hidden by the existing profile. + * Also, since we test the plugins in order of priority, all following plugins + * are unsuitable. + * + * Multiple connection plugins are so cumbersome, especially if they are unable + * to add the connection. I suggest to disable all plugins except keyfile. */ + _LOGT ("add-connection: failed to add %s/'%s': there is an existing storage "NM_SETTINGS_STORAGE_PRINT_FMT" with higher priority", + nm_connection_get_uuid (new_connection), + nm_connection_get_id (new_connection), + NM_SETTINGS_STORAGE_PRINT_ARG (conflicting_storage)); + nm_assert (first_error); + break; + } + } + if (plugin == (NMSettingsPlugin *) priv->keyfile_plugin) { success = nms_keyfile_plugin_add_connection (priv->keyfile_plugin, new_connection, @@ -1473,8 +1571,8 @@ nm_settings_add_connection (NMSettings *self, uuid = nm_connection_get_uuid (connection); - /* Make sure a connection with this UUID doesn't already exist */ - if (_sett_conn_entry_get_conn (_sett_conn_entries_get (self, uuid))) { + sett_conn_entry = _sett_conn_entries_get (self, uuid); + if (_sett_conn_entry_get_conn (sett_conn_entry)) { g_set_error_literal (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_UUID_EXISTS, @@ -1499,6 +1597,7 @@ nm_settings_add_connection (NMSettings *self, connection = connection_cloned_1; if (!_add_connection_to_first_plugin (self, + sett_conn_entry, connection, ( persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK || NM_FLAGS_ANY (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE @@ -1735,6 +1834,7 @@ nm_settings_update_connection (NMSettings *self, if (migrate_storage) { success = _add_connection_to_first_plugin (self, + _sett_conn_entries_get (self, uuid), connection, new_in_memory, sett_flags, From 9eddf9fb0943be57c85bbdf9c237bb3c406aeb53 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 17:36:43 +0200 Subject: [PATCH 11/14] settings: track profiles on disk that are shadowed by in-memory connections Via Update2() D-Bus API there are three ways how a profile can be stored (or migrated) to in-memory: - NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY - NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED - NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY With the recent rework of settings I dropped NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY and it had the same meaning as NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED. However, the way NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED was implemented is problematic. The problem is that it leaves the profile on disk but creates an in-memory representation which shadows the persistent storage. Later, when storing the profile to disk again, a new filename is chosen. This allows via D-Bus API to toggle between NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED and NM_SETTINGS_UPDATE2_FLAG_TO_DISK, and thereby pilling up profiles on disk. Also, there is no D-Bus API to do anything sensible with these leaked, shadowed profiles on disk. Note that if we have a read-only profile in /usr/lib or in ifupdown plugin, then the problem is not made any worse. That is, because via D-Bus API such profiles can be made in-memory, and afterwards stored to /etc. Thereby too the profile gets duplicate on disk, but this game only works once. Afterwards, you cannot repeat it to create additional profiles on disk. It means, you can only leak profiles once, and only if they already exist in read-only storage to begin with. This problem with NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED already existed before the settings-delegate-storage rework, and is unrelated to whether in-memory profiles now happen to be persisted to /run. Note that NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY is simple and does not suffer from this problem. When you move a profile to in-memory-only, it gets deleted from persistent storage and no duplication happens. The problem is that NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED used to forget about the profile that it shadows, and that is wrong. So, first re-add proper support for NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. This works by remembering the "shadowed-storage" path for in-memory profiles. When later saving such a profile to disk again, the shadowed-storage will be re-used. Likewise, when deleting such a profile, the shadowed storage will be deleted. Note that we keep NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED and it also remembers the shadowed storage (but without "owning" it). That means, when such a profile gets saved to disk again, the orginal storage is reused too. As such, during future updates it behaves just like NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. The difference is when deleting such a profile. In this case, the profile is left on storage and a tombstone gets written. So, how is this better than before and why even keep this complicated flag? First, we keep this flag because we really want the ansible role to be able to do in-memory changes only. That implies being able to delete a profile from NetworkManager's view, but not from persistent storage. Without this flag there is no way to do that. You can only modify an on-disk profile by shadowing it, but you could not delete it form NetworkManager's view while keeping it on disk. The new form of NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED is safe and avoids the duplication problem because also for tombstones it remembers the original "shadowed-storage". That is, when the profile gets recreated later via D-Bus API AddConnection, then the re-created profile will still reference and reuse the shadowed storage that it had before deletion. --- libnm-core/nm-dbus-interface.h | 41 +- src/devices/nm-device.c | 2 +- src/settings/nm-settings-connection.c | 8 +- src/settings/nm-settings-connection.h | 45 +- src/settings/nm-settings.c | 573 ++++++++++++++++++-------- 5 files changed, 468 insertions(+), 201 deletions(-) diff --git a/libnm-core/nm-dbus-interface.h b/libnm-core/nm-dbus-interface.h index b2569af1ef..35496e82c2 100644 --- a/libnm-core/nm-dbus-interface.h +++ b/libnm-core/nm-dbus-interface.h @@ -1030,26 +1030,31 @@ typedef enum { /*< flags >*/ * NMSettingsUpdate2Flags: * @NM_SETTINGS_UPDATE2_FLAG_NONE: an alias for numeric zero, no flags set. * @NM_SETTINGS_UPDATE2_FLAG_TO_DISK: to persist the connection to disk. - * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY: to make the connection in-memory only. - * If the connection was previously persistent, the corresponding file on disk - * is not deleted but merely the connection is decoupled from the file - * on disk. If you later delete an in-memory connection, the connection - * on disk will be deleted as well. - * Note: with 1.20, this flag is no longer implemented because in-memory connections - * are also persisted under /run. For the moment, this behaves the same as - * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED. - * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED: this is like @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY, - * but if the connection has a corresponding file on disk, the association between - * the connection and the file is forgotten but the file is not modified. - * The difference to %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY is if you later - * save the connection again to disk, a new file name will be chosen without - * overwriting the remaining file on disk. Also, if you delete the connection - * later, the file on disk will not be deleted. + * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY: makes the profile in-memory. + * Note that such profiles are stored in keyfile format under /run. + * If the file is already in-memory, the file in /run is updated in-place. + * Otherwise, the previous storage for the profile is left unchanged + * on disk, and the in-memory copy shadows it. + * Note that the original filename of the previous persistent storage (if any) + * is remembered. That means, when later persisting the profile again to disk, + * the file on disk will be overwritten again. + * Likewise, when finally deleting the profile, both the storage from /run + * and persistent storage are deleted (or if the persistent storage does not + * allow deletion, and nmmeta file is written to mark the UUID as deleted). + * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED: this is almost the same as + * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY, with one difference: when later deleting + * the profile, the original profile will not be deleted. Instead a nmmeta + * file is written to /run to indicate that the profile is gone. + * Note that if such a nmmeta tombstone file exists and hides a file in persistant + * storage, then when re-adding the profile with the same UUID, then the original + * storage is taken over again. * @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY: this is like @NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY, - * but if the connection has a corresponding file on disk, the file on - * disk will be deleted. + * but if the connection has a corresponding file on persistent storage, the file + * will be deleted right away. If the profile is later again persisted to disk, + * a new, unused filename will be chosen. * @NM_SETTINGS_UPDATE2_FLAG_VOLATILE: This can be specified with either - * %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED or %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY. + * %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY, %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED + * or %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY. * After making the connection in-memory only, the connection is marked * as volatile. That means, if the connection is currently not active * it will be deleted right away. Otherwise, it is marked to for deletion diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 04a94db8a1..da34602427 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -12420,7 +12420,7 @@ nm_device_set_ip_config (NMDevice *self, nm_settings_connection_update (settings_connection, new_connection, - NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, + NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY, NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, NM_SETTINGS_CONNECTION_INT_FLAGS_NONE, NM_SETTINGS_CONNECTION_UPDATE_REASON_NONE, diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 5da772ed96..2c1105258f 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1514,8 +1514,9 @@ update_auth_cb (NMSettingsConnection *self, if (NM_FLAGS_HAS (info->flags, NM_SETTINGS_UPDATE2_FLAG_TO_DISK)) persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; - else if (NM_FLAGS_ANY (info->flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY - | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED)) + else if (NM_FLAGS_ANY (info->flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY)) + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY; + else if (NM_FLAGS_ANY (info->flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED)) persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED; else if (NM_FLAGS_HAS (info->flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY)) { persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY; @@ -1752,7 +1753,8 @@ impl_settings_connection_update2 (NMDBusObject *obj, if ( ( NM_FLAGS_ANY (flags, _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES) && !nm_utils_is_power_of_two (flags & _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES)) || ( NM_FLAGS_HAS (flags, NM_SETTINGS_UPDATE2_FLAG_VOLATILE) - && !NM_FLAGS_ANY (flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED + && !NM_FLAGS_ANY (flags, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY + | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY))) { error = g_error_new_literal (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_ARGUMENTS, diff --git a/src/settings/nm-settings-connection.h b/src/settings/nm-settings-connection.h index ee58919abc..61d5c246c3 100644 --- a/src/settings/nm-settings-connection.h +++ b/src/settings/nm-settings-connection.h @@ -85,21 +85,46 @@ typedef enum { * if the profile is on-disk, update it on-disk, and keep it. */ NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP, - /* persist to disk. If the profile is currenly in-memory, remove - * it from /run. */ + /* persist to disk. If the profile is currently in-memory, remove + * it from /run. Depending on the shadowed-storage, the pre-existing + * file is reused when moving the storage. + * + * Corresponds to %NM_SETTINGS_UPDATE2_FLAG_TO_DISK. */ NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, - /* persist to /run (in-memory). If the profile is currently on disk, - * delete it from disk. */ - NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY, + /* Update in-memory (i.e. persist to /run). If the profile is currently on disk, + * then a reference to the profile is remembered as "shadowed-storage". + * Later, when storing again to persistant storage, the shawowed-storage is + * updated. When deleting the profile, the shadowed-storage is also deleted + * from disk. + * + * Corresponds to %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. */ + NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY, - /* persist to /run (in-memory). If the profile is currently on disk, - * forget about it, but don't delete it from disk. */ + /* Update in-memory (i.e. persist to /run). This is almost like + * %NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY, except the in-memory profile + * remembers not to own the shadowed-storage ("shadowed-owned"). + * The diffrence is that when deleting the in-memory profile, the original + * profile is not deleted but instead the nmmeta tombstone remembers the + * shadowed-storage and re-used it when re-adding the profile. + * + * Corresponds to %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED. */ NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, - /* this only updates the connection in-memory. Note that "in-memory" above - * means to write to keyfile in /run. This really means to not notify the - * settings plugin about the change. */ + /* Update in-memory (i.e. persist to /run). If the profile is currently on disk, + * delete it from disk. + * + * If the profile is in-memory and has a shadowed-storage, the original profile + * will be deleted from disk. + * + * Corresponds to %NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY. */ + NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY, + + /* This only updates the connection in-memory. Note that "in-memory" above + * means to write to keyfile in /run. This mode really means to not notify the + * settings plugin about the change. This should be only used for updating + * secrets. + */ NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST, } NMSettingsConnectionPersistMode; diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 4075ca6040..85a3678a33 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -303,6 +303,35 @@ _sett_conn_entry_storage_find_conflicting_storage (SettConnEntry *sett_conn_entr return NULL; } +static NMSettingsStorage * +_sett_conn_entry_find_shadowed_storage (SettConnEntry *sett_conn_entry, + const char *shadowed_storage_filename, + NMSettingsStorage *blacklisted_storage) +{ + StorageData *sd; + + if (!shadowed_storage_filename) + return NULL; + + c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { + + nm_assert (NM_IS_SETTINGS_STORAGE (sd->storage)); + + if (!sd->connection) + continue; + + if (blacklisted_storage == sd->storage) + continue; + + if (!nm_streq0 (nm_settings_storage_get_filename_for_shadowed_storage (sd->storage), shadowed_storage_filename)) + continue; + + return sd->storage; + } + + return NULL; +} + /*****************************************************************************/ NM_GOBJECT_PROPERTIES_DEFINE (NMSettings, @@ -1521,6 +1550,53 @@ _update_connection_to_plugin (NMSettings *self, return success; } +static void +_set_nmmeta_tombstone (NMSettings *self, + const char *uuid, + gboolean tombstone_on_disk, + gboolean tombstone_in_memory, + const char *shadowed_storage) +{ + NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); + gs_unref_object NMSettingsStorage *tombstone_1_storage = NULL; + gs_unref_object NMSettingsStorage *tombstone_2_storage = NULL; + + if (tombstone_on_disk) { + if (!nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, + FALSE, + uuid, + FALSE, + TRUE, + NULL, + &tombstone_1_storage, + NULL)) + tombstone_in_memory = TRUE; + if (tombstone_1_storage) + _connection_changed_track (self, tombstone_1_storage, NULL, FALSE); + } + + if (tombstone_in_memory) { + if (!nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, + FALSE, + uuid, + TRUE, + TRUE, + shadowed_storage, + &tombstone_2_storage, + NULL)) { + nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, + TRUE, + uuid, + TRUE, + TRUE, + shadowed_storage, + &tombstone_2_storage, + NULL); + } + _connection_changed_track (self, tombstone_2_storage, NULL, FALSE); + } +} + /** * nm_settings_add_connection: * @self: the #NMSettings object @@ -1546,24 +1622,34 @@ nm_settings_add_connection (NMSettings *self, NMSettingsConnection **out_sett_conn, GError **error) { + NMSettingsPrivate *priv; gs_unref_object NMConnection *connection_cloned_1 = NULL; gs_unref_object NMConnection *new_connection = NULL; gs_unref_object NMSettingsStorage *new_storage = NULL; + gs_unref_object NMSettingsStorage *shadowed_storage = NULL; + NMSettingsStorage *update_storage = NULL; gs_free_error GError *local = NULL; SettConnEntry *sett_conn_entry; const char *uuid; StorageData *sd; + gboolean new_in_memory; + gboolean success; + const char *shadowed_storage_filename = NULL; + + priv = NM_SETTINGS_GET_PRIVATE (self); nm_assert (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)); + new_in_memory = (persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK); + nm_assert (!NM_FLAGS_ANY (sett_flags, ~_NM_SETTINGS_CONNECTION_INT_FLAGS_PERSISTENT_MASK)); - nm_assert ( !NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED) - || persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY); - - nm_assert ( !NM_FLAGS_HAS (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE) - || persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY); + if (NM_FLAGS_ANY (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE + | NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED)) { + nm_assert (new_in_memory); + new_in_memory = TRUE; + } nm_assert (!NM_FLAGS_ANY (add_reason, ~NM_SETTINGS_CONNECTION_ADD_REASON_BLOCK_AUTOCONNECT)); @@ -1596,23 +1682,106 @@ nm_settings_add_connection (NMSettings *self, if (connection_cloned_1) connection = connection_cloned_1; - if (!_add_connection_to_first_plugin (self, - sett_conn_entry, - connection, - ( persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK - || NM_FLAGS_ANY (sett_flags, NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE - | NM_SETTINGS_CONNECTION_INT_FLAGS_NM_GENERATED)), - sett_flags, - NULL, - FALSE, - &new_storage, - &new_connection, - &local)) { - g_set_error (error, - NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_FAILED, - "failure adding connection: %s", - local->message); + if (sett_conn_entry) { + c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { + if (!nm_settings_storage_is_meta_data (sd->storage)) + continue; + shadowed_storage = nm_g_object_ref (_sett_conn_entry_find_shadowed_storage (sett_conn_entry, + nm_settings_storage_get_shadowed_storage (sd->storage, NULL), + NULL)); + if (shadowed_storage) { + /* We have a nmmeta tombstone that indicates that a storage is shadowed. + * + * This happens when deleting a in-memory profile that was decoupled from + * the persitant storage with NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED. + * We need to take over this storage again... */ + break; + } + } + } + + if ( shadowed_storage + && !new_in_memory) { + NMSettingsStorage *conflicting_storage; + + conflicting_storage = _sett_conn_entry_storage_find_conflicting_storage (sett_conn_entry, + nm_settings_storage_get_plugin (shadowed_storage), + shadowed_storage, + priv->plugins); + if (conflicting_storage) { + /* We cannot add the profile as @shadowed_storage, because there is another, existing storage + * that would hide it. Just add it as new storage. In general, this leads to duplication of profiles, + * but the circumstances where this happens are very exotic (you need at least one additional settings + * plugin, then going through the paths of making shadowed_storage in-memory-detached and delete it, + * and finally adding the conflicting storage outside of NM and restart/reload). */ + _LOGT ("ignore shadowed storage "NM_SETTINGS_STORAGE_PRINT_FMT" due to conflicting storage "NM_SETTINGS_STORAGE_PRINT_FMT, + NM_SETTINGS_STORAGE_PRINT_ARG (shadowed_storage), + NM_SETTINGS_STORAGE_PRINT_ARG (conflicting_storage)); + } else + update_storage = shadowed_storage; + } + + shadowed_storage_filename = ( shadowed_storage + && !update_storage) + ? nm_settings_storage_get_filename_for_shadowed_storage (shadowed_storage) + : NULL; + +again_add_connection: + + if (!update_storage) { + success = _add_connection_to_first_plugin (self, + sett_conn_entry, + connection, + new_in_memory, + sett_flags, + shadowed_storage_filename, + FALSE, + &new_storage, + &new_connection, + &local); + } else { + success = _update_connection_to_plugin (self, + update_storage, + connection, + sett_flags, + FALSE, + shadowed_storage_filename, + FALSE, + &new_storage, + &new_connection, + &local); + if (!success) { + if (!NMS_IS_KEYFILE_STORAGE (update_storage)) { + /* hm, the intended storage is not keyfile (it's ifcfg-rh). This settings + * plugin may not support the new connection. So step back and retry adding + * the profile anew. */ + _LOGT ("failure to add profile as existing storage \"%s\": %s", + nm_settings_storage_get_filename (update_storage), + local->message); + update_storage = NULL; + g_clear_object (&shadowed_storage); + shadowed_storage_filename = NULL; + g_clear_error (&local); + goto again_add_connection; + } + } + } + + if (!success) { + if (!update_storage) { + g_set_error (error, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_FAILED, + "failure adding connection: %s", + local->message); + } else { + g_set_error (error, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_FAILED, + "failure writing connection to existing storage \"%s\": %s", + nm_settings_storage_get_filename (update_storage), + local->message); + } return FALSE; } @@ -1620,29 +1789,45 @@ nm_settings_add_connection (NMSettings *self, c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { const NMSettingsMetaData *meta_data; + gs_unref_object NMSettingsStorage *new_tombstone_storage = NULL; + gboolean in_memory; + gboolean simulate; meta_data = nm_settings_storage_is_meta_data_alive (sd->storage); - if ( !meta_data || !meta_data->is_tombstone) continue; - if (nm_settings_storage_is_keyfile_run (sd->storage)) { - /* We remove this file from /run. */ - } else { + if (nm_settings_storage_is_keyfile_run (sd->storage)) + in_memory = TRUE; + else { if (nm_settings_storage_is_keyfile_run (new_storage)) { /* Don't remove the file from /etc if we just wrote an in-memory connection */ continue; } + in_memory = FALSE; } - nm_settings_plugin_delete_connection (nm_settings_storage_get_plugin (sd->storage), - sd->storage, - NULL); - - nm_assert (!_storage_data_is_alive (sd)); - - _connection_changed_track (self, sd->storage, NULL, FALSE); + simulate = FALSE; +again_delete_tombstone: + if (!nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, + simulate, + uuid, + in_memory, + FALSE, + NULL, + &new_tombstone_storage, + NULL)) { + /* Ups, something went wrong. We really need to get rid of the tombstone. At least + * forget about it in-memory. Upong next restart/reload, this might be reverted + * however :( .*/ + if (!simulate) { + simulate = TRUE; + goto again_delete_tombstone; + } + } + if (new_tombstone_storage) + _connection_changed_track (self, new_tombstone_storage, NULL, FALSE); } _connection_changed_process_all_dirty (self, @@ -1682,11 +1867,13 @@ nm_settings_update_connection (NMSettings *self, NMConnection *new_connection_real; gs_unref_object NMSettingsStorage *cur_storage = NULL; gs_unref_object NMSettingsStorage *new_storage = NULL; + NMSettingsStorage *drop_storage = NULL; + SettConnEntry *sett_conn_entry; gboolean cur_in_memory; gboolean new_in_memory; const char *uuid; - const char *shadowed_storage; - gboolean shadowed_owned; + gboolean tombstone_in_memory = FALSE; + gboolean tombstone_on_disk = FALSE; g_return_val_if_fail (NM_IS_SETTINGS (self), FALSE); g_return_val_if_fail (NM_IS_SETTINGS_CONNECTION (sett_conn), FALSE); @@ -1697,6 +1884,7 @@ nm_settings_update_connection (NMSettings *self, nm_assert (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP, NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST, NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, + NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)); @@ -1705,7 +1893,10 @@ nm_settings_update_connection (NMSettings *self, uuid = nm_settings_storage_get_uuid (cur_storage); nm_assert (NM_IS_SETTINGS_STORAGE (cur_storage)); - nm_assert (_sett_conn_entry_get_conn (_sett_conn_entries_get (self, uuid)) == sett_conn); + + sett_conn_entry = _sett_conn_entries_get (self, uuid); + + nm_assert (_sett_conn_entry_get_conn (sett_conn_entry) == sett_conn); if (connection) { gs_free_error GError *local = NULL; @@ -1736,7 +1927,7 @@ nm_settings_update_connection (NMSettings *self, if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP) { persist_mode = cur_in_memory - ? NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED + ? NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY : NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; } @@ -1757,10 +1948,14 @@ nm_settings_update_connection (NMSettings *self, default_wired_clear_tag (self, device, sett_conn, FALSE); - if (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP, - NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST)) { - /* making a default-wired-connection a regulard connection implies persisting - * it to disk (unless specified differently). */ + if (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST)) { + /* making a default-wired-connection a regular connection implies persisting + * it to disk (unless specified differently). + * + * Actually, this line is probably unreached, because we should not use + * NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST to toggle the nm-generated + * flag. */ + nm_assert_not_reached (); persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; } } @@ -1776,12 +1971,13 @@ nm_settings_update_connection (NMSettings *self, * in-memory. The caller did not request to persist this to disk, however we need * to store the flags in run. */ nm_assert (cur_in_memory); - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED; + persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY; } if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK) new_in_memory = FALSE; - else if (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, + else if (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY, + NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY)) new_in_memory = TRUE; else { @@ -1806,51 +2002,93 @@ nm_settings_update_connection (NMSettings *self, | NM_SETTINGS_CONNECTION_INT_FLAGS_VOLATILE); } - /* TODO: set and handle shadowed-storages. */ - shadowed_storage = NULL; - shadowed_owned = FALSE; - if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST) { new_storage = g_object_ref (cur_storage); - new_connection = g_object_ref (connection); + new_connection_real = connection; _LOGT ("update[%s]: %s: update profile \"%s\" (not persisted)", nm_settings_storage_get_uuid (cur_storage), log_context_name, nm_connection_get_id (connection)); } else { - gboolean success; - gboolean migrate_storage; + NMSettingsStorage *shadowed_storage; + const char *cur_shadowed_storage_filename; + const char *new_shadowed_storage_filename = NULL; + gboolean cur_shadowed_owned; + gboolean new_shadowed_owned = FALSE; + NMSettingsStorage *update_storage = NULL; gs_free_error GError *local = NULL; + gboolean success; - if (new_in_memory != cur_in_memory) - migrate_storage = TRUE; - else if ( !new_in_memory - && nm_settings_storage_is_keyfile_lib (cur_storage)) { + cur_shadowed_storage_filename = nm_settings_storage_get_shadowed_storage (cur_storage, &cur_shadowed_owned); + + shadowed_storage = _sett_conn_entry_find_shadowed_storage (sett_conn_entry, cur_shadowed_storage_filename, cur_storage); + if (!shadowed_storage) { + cur_shadowed_storage_filename = NULL; + cur_shadowed_owned = FALSE; + } + + if ( new_in_memory + && persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY) { + if (cur_in_memory) { + drop_storage = shadowed_storage; + update_storage = cur_storage; + } else + drop_storage = cur_storage; + } else if ( !new_in_memory + && cur_in_memory + && shadowed_storage) { + drop_storage = cur_storage; + update_storage = shadowed_storage; + } else if (new_in_memory != cur_in_memory) { + if (!new_in_memory) + drop_storage = cur_storage; + else if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY) + drop_storage = cur_storage; + else { + nm_assert (NM_IN_SET (persist_mode, NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY, + NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED)); + } + } else if (nm_settings_storage_is_keyfile_lib (cur_storage)) { /* the profile is a keyfile in /usr/lib. It cannot be overwritten, we must migrate it * from /usr/lib to /etc. */ - migrate_storage = TRUE; } else - migrate_storage = FALSE; + update_storage = cur_storage; - if (migrate_storage) { + if (new_in_memory) { + if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY) { + /* pass */ + } else if (!cur_in_memory) { + new_shadowed_storage_filename = nm_settings_storage_get_filename_for_shadowed_storage (cur_storage); + if ( new_shadowed_storage_filename + && persist_mode != NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED) + new_shadowed_owned = TRUE; + } else { + new_shadowed_storage_filename = cur_shadowed_storage_filename; + if ( new_shadowed_storage_filename + && persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY) + new_shadowed_owned = TRUE; + } + } + + if (!update_storage) { success = _add_connection_to_first_plugin (self, - _sett_conn_entries_get (self, uuid), + sett_conn_entry, connection, new_in_memory, sett_flags, - shadowed_storage, - shadowed_owned, + new_shadowed_storage_filename, + new_shadowed_owned, &new_storage, &new_connection, &local); } else { success = _update_connection_to_plugin (self, - cur_storage, + update_storage, connection, sett_flags, update_reason, - shadowed_storage, - shadowed_owned, + new_shadowed_storage_filename, + new_shadowed_owned, &new_storage, &new_connection, &local); @@ -1864,7 +2102,7 @@ nm_settings_update_connection (NMSettings *self, nm_settings_storage_get_uuid (cur_storage), log_context_name, ignore_failure ? "ignore " : "", - migrate_storage ? "write" : "update", + update_storage ? "update" : "write", nm_connection_get_id (connection), local->message); if (!ignore_failure) { @@ -1872,76 +2110,77 @@ nm_settings_update_connection (NMSettings *self, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "failed to %s connection: %s", - migrate_storage ? "write" : "update", + update_storage ? "update" : "write", local->message); return FALSE; } + new_storage = g_object_ref (cur_storage); - new_connection = g_object_ref (connection); + new_connection_real = connection; } else { + gs_unref_variant GVariant *agent_owned_secrets = NULL; + _LOGT ("update[%s]: %s: %s profile \"%s\"", nm_settings_storage_get_uuid (cur_storage), log_context_name, - migrate_storage ? "write" : "update", + update_storage ? "update" : "write", nm_connection_get_id (connection)); + + nm_assert_valid_settings_storage (NULL, new_storage); + nm_assert (NM_IS_CONNECTION (new_connection)); + nm_assert (nm_streq (uuid, nm_settings_storage_get_uuid (new_storage))); + + + agent_owned_secrets = nm_connection_to_dbus (connection, + NM_CONNECTION_SERIALIZE_ONLY_SECRETS + | NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED); + new_connection_real = _connection_changed_normalize_connection (new_storage, + new_connection, + agent_owned_secrets, + &new_connection_cloned); + if (!new_connection_real) { + nm_assert_not_reached (); + new_connection_real = new_connection; + } } } - nm_assert_valid_settings_storage (NULL, new_storage); - nm_assert (NM_IS_CONNECTION (new_connection)); - nm_assert (nm_streq (uuid, nm_settings_storage_get_uuid (new_storage))); - - if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST) - new_connection_real = new_connection; - else { - gs_unref_variant GVariant *agent_owned_secrets = NULL; - - agent_owned_secrets = nm_connection_to_dbus (connection, - NM_CONNECTION_SERIALIZE_ONLY_SECRETS - | NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED); - new_connection_real = _connection_changed_normalize_connection (new_storage, - new_connection, - agent_owned_secrets, - &new_connection_cloned); - if (!new_connection_real) { - nm_assert_not_reached (); - new_connection_real = new_connection; - } - } - + nm_assert (NM_IS_SETTINGS_STORAGE (new_storage)); nm_assert (NM_IS_CONNECTION (new_connection_real)); _connection_changed_track (self, new_storage, new_connection_real, TRUE); - if (new_storage != cur_storage) { + if ( drop_storage + && drop_storage != new_storage) { gs_free_error GError *local = NULL; - gboolean remove_from_disk; - if (persist_mode == NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED) - remove_from_disk = FALSE; - else if (nm_settings_storage_is_keyfile_lib (cur_storage)) - remove_from_disk = FALSE; - else - remove_from_disk = TRUE; + if (!nm_settings_plugin_delete_connection (nm_settings_storage_get_plugin (drop_storage), + drop_storage, + &local)) { + const char *filename; - if (remove_from_disk) { - if (!nm_settings_plugin_delete_connection (nm_settings_storage_get_plugin (cur_storage), - cur_storage, - &local)) { - const char *filename; - - filename = nm_settings_storage_get_filename (cur_storage); - _LOGW ("update[%s]: failed to delete moved storage "NM_SETTINGS_STORAGE_PRINT_FMT"%s%s%s: %s", - nm_settings_storage_get_uuid (cur_storage), - NM_SETTINGS_STORAGE_PRINT_ARG (cur_storage), - local->message, - NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", "")); - } - - _connection_changed_track (self, cur_storage, NULL, FALSE); - } + filename = nm_settings_storage_get_filename (drop_storage); + _LOGT ("update[%s]: failed to delete moved storage "NM_SETTINGS_STORAGE_PRINT_FMT"%s%s%s: %s", + nm_settings_storage_get_uuid (drop_storage), + NM_SETTINGS_STORAGE_PRINT_ARG (drop_storage), + NM_PRINT_FMT_QUOTED (filename, " (file \"", filename, "\")", ""), + local->message); + /* there is no aborting back form this. We must get rid of the connection and + * cannot do better than log a message. Proceed, but remember to write tombstones. */ + if (nm_settings_storage_is_keyfile_run (cur_storage)) + tombstone_in_memory = TRUE; + else + tombstone_on_disk = TRUE; + } else + _connection_changed_track (self, drop_storage, NULL, FALSE); } + _set_nmmeta_tombstone (self, + uuid, + tombstone_on_disk, + tombstone_in_memory, + NULL); + _connection_changed_process_all_dirty (self, FALSE, sett_flags, @@ -1957,23 +2196,24 @@ nm_settings_delete_connection (NMSettings *self, NMSettingsConnection *sett_conn, gboolean allow_add_to_no_auto_default) { - NMSettingsPrivate *priv; NMSettingsStorage *cur_storage; + NMSettingsStorage *shadowed_storage; + NMSettingsStorage *shadowed_storage_unowned = NULL; + NMSettingsStorage *drop_storages[2] = { }; gs_free_error GError *local = NULL; SettConnEntry *sett_conn_entry; + const char *cur_shadowed_storage_filename; + const char *new_shadowed_storage_filename = NULL; + gboolean cur_shadowed_owned; const char *uuid; - gboolean delete; gboolean tombstone_in_memory = FALSE; gboolean tombstone_on_disk = FALSE; - gs_unref_object NMSettingsStorage *tombstone_1_storage = NULL; - gs_unref_object NMSettingsStorage *tombstone_2_storage = NULL; + int i; g_return_if_fail (NM_IS_SETTINGS (self)); g_return_if_fail (NM_IS_SETTINGS_CONNECTION (sett_conn)); g_return_if_fail (nm_settings_has_connection (self, sett_conn)); - priv = NM_SETTINGS_GET_PRIVATE (self); - cur_storage = nm_settings_connection_get_storage (sett_conn); nm_assert (NM_IS_SETTINGS_STORAGE (cur_storage)); @@ -1992,39 +2232,63 @@ nm_settings_delete_connection (NMSettings *self, if (NM_IN_SET (s->storage_type, NMS_KEYFILE_STORAGE_TYPE_RUN, NMS_KEYFILE_STORAGE_TYPE_ETC)) - delete = TRUE; - else { + drop_storages[0] = cur_storage; + else tombstone_on_disk = TRUE; - delete = FALSE; - } } else - delete = TRUE; + drop_storages[0] = cur_storage; - if (delete) { + cur_shadowed_storage_filename = nm_settings_storage_get_shadowed_storage (cur_storage, &cur_shadowed_owned); + + shadowed_storage = _sett_conn_entry_find_shadowed_storage (sett_conn_entry, cur_shadowed_storage_filename, cur_storage); + if (shadowed_storage) { + if (!cur_shadowed_owned) + shadowed_storage_unowned = g_steal_pointer (&shadowed_storage); + } + drop_storages[1] = shadowed_storage; + + for (i = 0; i < (int) G_N_ELEMENTS (drop_storages); i++) { + NMSettingsStorage *storage; StorageData *sd; - if (!nm_settings_plugin_delete_connection (nm_settings_storage_get_plugin (cur_storage), - cur_storage, + storage = drop_storages[i]; + if (!storage) + continue; + + if (!nm_settings_plugin_delete_connection (nm_settings_storage_get_plugin (storage), + storage, &local)) { - _LOGW ("delete-connection: failed to delete storage "NM_SETTINGS_STORAGE_PRINT_FMT": %s", - NM_SETTINGS_STORAGE_PRINT_ARG (cur_storage), + _LOGT ("delete-connection: failed to delete storage "NM_SETTINGS_STORAGE_PRINT_FMT": %s", + NM_SETTINGS_STORAGE_PRINT_ARG (storage), local->message); g_clear_error (&local); /* there is no aborting back form this. We must get rid of the connection and - * cannot do better than warn. Proceed... */ - tombstone_in_memory = TRUE; - } - - sett_conn_entry = _connection_changed_track (self, cur_storage, NULL, FALSE); + * cannot do better than log a message. Proceed, but remember to write tombstones. */ + if (nm_settings_storage_is_keyfile_run (cur_storage)) + tombstone_in_memory = TRUE; + else + tombstone_on_disk = TRUE; + sett_conn_entry = _sett_conn_entries_get (self, uuid); + } else + sett_conn_entry = _connection_changed_track (self, storage, NULL, FALSE); c_list_for_each_entry (sd, &sett_conn_entry->sd_lst_head, sd_lst) { - if (sd->storage == cur_storage) + if (NM_IN_SET (sd->storage, drop_storages[0], + drop_storages[1])) continue; if (!_storage_data_is_alive (sd)) continue; if (nm_settings_storage_is_meta_data (sd->storage)) continue; + if (sd->storage == shadowed_storage_unowned) { + /* this only happens if we leak a profile on disk after NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED. + * We need to write a tombstone and remember the shadowed-storage. */ + tombstone_in_memory = TRUE; + new_shadowed_storage_filename = nm_settings_storage_get_filename (shadowed_storage_unowned); + continue; + } + /* we have still conflicting storages. We need to hide them with tombstones. */ if (nm_settings_storage_is_keyfile_run (sd->storage)) { tombstone_in_memory = TRUE; @@ -2034,40 +2298,11 @@ nm_settings_delete_connection (NMSettings *self, } } - if (tombstone_on_disk) { - if (!nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, - FALSE, - uuid, - FALSE, - TRUE, - NULL, - &tombstone_1_storage, - NULL)) - tombstone_in_memory = TRUE; - if (tombstone_1_storage) - _connection_changed_track (self, tombstone_1_storage, NULL, FALSE); - } - - if (tombstone_in_memory) { - if (!nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, - FALSE, - uuid, - TRUE, - TRUE, - NULL, - &tombstone_2_storage, - NULL)) { - nms_keyfile_plugin_set_nmmeta_tombstone (priv->keyfile_plugin, - TRUE, - uuid, - TRUE, - TRUE, - NULL, - &tombstone_2_storage, - NULL); - } - _connection_changed_track (self, tombstone_2_storage, NULL, FALSE); - } + _set_nmmeta_tombstone (self, + uuid, + tombstone_on_disk, + tombstone_in_memory, + new_shadowed_storage_filename); _connection_changed_process_all_dirty (self, allow_add_to_no_auto_default, From ce44e120b421a44f96271684361c1817c4417084 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 25 Jul 2019 16:14:50 +0200 Subject: [PATCH 12/14] iwd: don't mark generated profiles as read-only First of all, the generated profile also gets generated as keyfile to /run, thereby it looses the read-only flag (because this flag gets lost when storing the profile to keyfile). Second, I don't think we should artificially limit the capabilities of the generated profile. If the user chooses to modify/delete it, so just allow it. --- src/devices/wifi/nm-iwd-manager.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index b696f128a7..5b550ea46d 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -436,7 +436,6 @@ mirror_8021x_connection (NMIwdManager *self, NM_SETTING_CONNECTION_TYPE, NM_SETTING_WIRELESS_SETTING_NAME, NM_SETTING_CONNECTION_ID, name, NM_SETTING_CONNECTION_UUID, nm_utils_uuid_generate_buf (uuid), - NM_SETTING_CONNECTION_READ_ONLY, TRUE, NULL)); nm_connection_add_setting (connection, setting); From 8d3ead72fdb27e3845c560c502fa486b60e826dc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 25 Jul 2019 16:17:01 +0200 Subject: [PATCH 13/14] settings: no longer honor read-only flag to prevent modifying connection profiles Note that we now support keyfiles from read-only storage in /usr/lib. Note also that we do support modifying and deleting these profiles. That works by placing a shadowing profile to /etc or /run. Surely this is questionable. It means that once the user uses D-Bus to modify/delete a profile in /usr/lib, that profile becomes forever shadowed by a different file, and there is no D-Bus API to return to the original file. The user would have to drop the shadowing storages from the file system. That is a problem. But on the other hand, disallowing changes to such read-only profiles is not very useful either. If you no longer can use D-Bus to modify such profiles, what's the value here? How are applications supposed to handle such profiles if there is no D-Bus API to do something sensible to them? So, whatever problems read-only profiles and this shadowing causes, I don't think that the solution is to entirely disallow changes via D-Bus. At that point, we can just as well allow changes to profiles from ifupdown. Note that you still cannot modify the profile directly (as the ifupdown plugin does not support that). But you can delete the profile (either temporarily via a tombstone in /run or permanently via a tombstone in /etc). You also can make the profile in-memory, and take it from there. Note that if you try to later store the in-memory profile to disk again, then it depends on the order of settings plugins whether that succeeds. If you have "plugins=keyfile,ifupdown", then the profile will be stored as keyfile to /etc. If you have "plugins=ifupdown,keyfile", then the modification will only be done in /run and the "to-disk" command silently ignored (there really is no better solution). --- src/settings/nm-settings-connection.c | 41 ------------------- .../plugins/ifupdown/nms-ifupdown-parser.c | 1 - 2 files changed, 42 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 2c1105258f..25a27e68cc 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1324,37 +1324,6 @@ auth_start (NMSettingsConnection *self, /**** DBus method handlers ************************************/ -static gboolean -check_writable (NMConnection *self, GError **error) -{ - NMSettingConnection *s_con; - - g_return_val_if_fail (NM_IS_CONNECTION (self), FALSE); - - s_con = nm_connection_get_setting_connection (self); - if (!s_con) { - g_set_error_literal (error, - NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_INVALID_CONNECTION, - "Connection did not have required 'connection' setting"); - return FALSE; - } - - /* If the connection is read-only, that has to be changed at the source of - * the problem (ex a system settings plugin that can't write connections out) - * instead of over D-Bus. - */ - if (nm_setting_connection_get_read_only (s_con)) { - g_set_error_literal (error, - NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_READ_ONLY_CONNECTION, - "Connection is read-only"); - return FALSE; - } - - return TRUE; -} - static void get_settings_auth_cb (NMSettingsConnection *self, GDBusMethodInvocation *context, @@ -1603,13 +1572,6 @@ settings_connection_update (NMSettingsConnection *self, UpdateInfo *info; const char *permission; - /* If the connection is read-only, that has to be changed at the source of - * the problem (ex a system settings plugin that can't write connections out) - * instead of over D-Bus. - */ - if (!check_writable (nm_settings_connection_get_connection (self), &error)) - goto error; - /* Check if the settings are valid first */ if (new_settings) { if (!g_variant_is_of_type (new_settings, NM_VARIANT_TYPE_CONNECTION)) { @@ -1837,9 +1799,6 @@ impl_settings_connection_delete (NMDBusObject *obj, nm_assert (nm_settings_connection_still_valid (self)); - if (!check_writable (nm_settings_connection_get_connection (self), &error)) - goto err; - subject = _new_auth_subject (invocation, &error); if (!subject) goto err; diff --git a/src/settings/plugins/ifupdown/nms-ifupdown-parser.c b/src/settings/plugins/ifupdown/nms-ifupdown-parser.c index b65013a431..41b20850ad 100644 --- a/src/settings/plugins/ifupdown/nms-ifupdown-parser.c +++ b/src/settings/plugins/ifupdown/nms-ifupdown-parser.c @@ -676,7 +676,6 @@ ifupdown_new_connection_from_if_block (if_block *block, NM_SETTING_CONNECTION_INTERFACE_NAME, block->name, NM_SETTING_CONNECTION_ID, idstr, NM_SETTING_CONNECTION_UUID, uuid, - NM_SETTING_CONNECTION_READ_ONLY, TRUE, NM_SETTING_CONNECTION_AUTOCONNECT, (gboolean) (!!autoconnect), NULL); From df252a620d570ac85dec40c68aac36fc380762a7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 25 Jul 2019 17:12:00 +0200 Subject: [PATCH 14/14] settings: fix priority for settings-storages for tombstones Tombstones in /etc are not only to hide storages of type keyfile. They are for hiding/shadowing any profile from persistant storage. That's why we need to compare them already in _sett_conn_entry_sds_update(). Fix the prioriy of storages for the same UUID. Note that the "$UUID.nmmeta" files (the tombstones) are handled by keyfile plugin. But that is only to load them together during `nmcli connection reload` when we iterate the files of the system-connections directory. For the most part, nmmeta/tombstones are not keyfile specific and handled by NMSettings. A tombstone in /run hides any profile (regardless of the settings plugin). And a tombstones in /etc hides any profile, except in-memory connections from keyfile /run directory. --- src/settings/nm-settings.c | 85 ++++++++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 13 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 85a3678a33..430d277693 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -824,24 +824,83 @@ _sett_conn_entries_remove_and_destroy (NMSettings *self, /*****************************************************************************/ +static int +_sett_conn_entry_sds_update_cmp_ascending (const StorageData *sd_a, + const StorageData *sd_b, + const GSList *plugins) +{ + const NMSettingsMetaData *meta_data_a; + const NMSettingsMetaData *meta_data_b; + bool is_keyfile_run_a; + bool is_keyfile_run_b; + + /* Sort storages by priority. More important storages are sorted + * higher (ascending sort). For example, if "sd_a" is more important than + * "sd_b" (sd_a>sd_b), a positive integer is returned. */ + + meta_data_a = nm_settings_storage_is_meta_data (sd_a->storage); + meta_data_b = nm_settings_storage_is_meta_data (sd_b->storage); + + /* runtime storages (both connections and meta-data) are always more + * important. */ + is_keyfile_run_a = nm_settings_storage_is_keyfile_run (sd_a->storage); + is_keyfile_run_b = nm_settings_storage_is_keyfile_run (sd_b->storage); + if (is_keyfile_run_a != is_keyfile_run_b) { + + if ( !meta_data_a + && !meta_data_b) { + /* Ok, both are non-meta-data providing actual profiles. But one is in /run and one is in + * another storage. In this case we first honor whether one of the storages is explicitly + * prioritized. The prioritize flag is an in-memory hack to overwrite relative priorities + * contrary to what exists on-disk. + * + * This is done because when we use explicit D-Bus API (like update-connection) + * to update a profile, then we really want to prioritize the candidate + * despite having multiple other profiles. + * + * The example is if you have the same UUID twice in /run (one of them shadowed). + * If you move it to disk, then one of the profiles gets deleted and re-created + * on disk, but that on-disk profile must win against the remainging profile in + * /run. At least until the next reload/restart. */ + NM_CMP_FIELD_UNSAFE (sd_a, sd_b, prioritize); + } + + /* in-memory has higher priority. That is regardless of whether any of + * them is meta-data/tombstone or a profile. + * + * That works, because if any of them are tombstones/metadata, then we are in full + * control. There can by only one meta-data file, which is fully owned (and accordingly + * created/deleted) by NetworkManager. + * + * The only case where this might not be right is if we have profiles + * in /run that are shadowed. When we move such a profile to disk, then + * a conflict might arise. That is handled by "prioritize" above! */ + NM_CMP_DIRECT (is_keyfile_run_a, is_keyfile_run_b); + } + + /* After we determined that both profiles are either in /run or not, + * tombstones are always more important than non-tombstones. */ + NM_CMP_DIRECT (meta_data_a && meta_data_a->is_tombstone, + meta_data_b && meta_data_b->is_tombstone); + + /* Again, prioritized entries are sorted first (higher priority). */ + NM_CMP_FIELD_UNSAFE (sd_a, sd_b, prioritize); + + /* finally, compare the storages. This basically honors the timestamp + * of the profile and the relative order of the source plugin (via the + * @plugins list). */ + return nm_settings_storage_cmp (sd_a->storage, sd_b->storage, plugins); +} + static int _sett_conn_entry_sds_update_cmp (const CList *ls_a, const CList *ls_b, gconstpointer user_data) { - const GSList *plugins = user_data; - StorageData *sd_a = c_list_entry (ls_a, StorageData, sd_lst); - StorageData *sd_b = c_list_entry (ls_b, StorageData, sd_lst); - - /* prioritized entries are sorted first (higher priority). */ - NM_CMP_FIELD_UNSAFE (sd_b, sd_a, prioritize); - - /* nm_settings_storage_cmp() compares in ascending order. Meaning, - * if the storage has higher priority, it gives a positive number (as one - * would expect). - * - * We want to sort the list in reverse though, with highest priority first. */ - return nm_settings_storage_cmp (sd_b->storage, sd_a->storage, plugins); + /* we sort highest priority storages first (descending). Hence, the order is swapped. */ + return _sett_conn_entry_sds_update_cmp_ascending (c_list_entry (ls_b, StorageData, sd_lst), + c_list_entry (ls_a, StorageData, sd_lst), + user_data); } static void