From de322afcd988c71d2dab986da2a8ea6a6222e2a9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 18:07:01 +0100 Subject: [PATCH 1/9] glib-aux: add nm_g_array_append_val() g_array_append_val() takes the pointer of the macro argument, that only works with lvalues. Add nm_g_array_append_val() --- src/libnm-glib-aux/nm-shared-utils.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 008217036b..b65d9e93d0 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1989,6 +1989,20 @@ nm_g_array_unref(GArray *arr) &g_array_index(arr, Type, _len); \ }) +#define nm_g_array_append_simple(arr, val) \ + G_STMT_START \ + { \ + /* Similar to `g_array_append_val()`, but `g_array_append_val()` + * only works with lvalues. That makes sense if the value is a larger + * struct and you anyway have a pointer to it. It doesn't make sense + * if you have a list of int and want to append a number literal. + * + * nm_g_array_append_simple() is different. It depends on typeof(val) + * to be compatible. */ \ + (*nm_g_array_append_new((arr), typeof(val))) = (val); \ + } \ + G_STMT_END + /*****************************************************************************/ static inline GPtrArray * From 8bed2c9edc644287fa708d6837de6f601f4b5849 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Dec 2022 12:25:04 +0100 Subject: [PATCH 2/9] core: add "VersionInfo" property on D-Bus and NMClient This exposes NM_VERSION as number (contrary to the "Version", which is a string). That is in particular useful, because the number can be compared with <> due to the encoding of the version. While at it, don't make it a single number. Expose an array of numbers, where the following numbers are a bitfield of capabilities. Note that before commit 3c67a1ec5e8e ('cli: remove version check against NM'), we used to parse the "Version" string to detect the version. As such, the information that "VersionInfo" exposes now, was already (somewhat) available, you just had to parse the string. The main benefit of "VersionInfo" is that it can expose capabilities (patched behavior) in in a lightweight bitfield. To include the numerical version there is just useful on top. Currently no additional capabilities are exposed. The idea is of course to have a place in the future, where we can expose additional capabilities. Adding a capability flag is most useful for behavior that we backport to older branches. Otherwise, we could just check the daemon version alone. But since we only add "VersionInfo" property only now, we cannot backport any capability further than this, because the "VersionInfo" property itself won't be backported. As such, this will only be useful in the future by having a place where we can add (and backport) capabilities. Note that there is some overlap with the existing "Capability" property and NMCapability enum. The difference is that adding a capability via "VersionInfo" is only one bit, and thus cheaper. Most importantly, having it cheaper means the downsides of adding a capability flag is significantly removed. In practice, we could live without capabilities for a long time, so they must be very cheap for them to be worth to add. Another difference might be, that we will want that the VersionInfo is about compile time defaults (e.g. a certain patch/behavior that is in or not), while NM_CAPABILITY_TEAM depends on whether the team plugin is loaded at runtime. --- .../org.freedesktop.NetworkManager.xml | 16 +++ src/core/nm-manager.c | 40 +++++++ src/core/nm-manager.h | 1 + src/libnm-client-impl/libnm.ver | 2 + src/libnm-client-impl/nm-client.c | 105 +++++++++++++++--- src/libnm-client-public/nm-client.h | 11 +- src/libnm-core-public/nm-dbus-interface.h | 14 +++ 7 files changed, 172 insertions(+), 17 deletions(-) diff --git a/introspection/org.freedesktop.NetworkManager.xml b/introspection/org.freedesktop.NetworkManager.xml index 1618570623..c92c801741 100644 --- a/introspection/org.freedesktop.NetworkManager.xml +++ b/introspection/org.freedesktop.NetworkManager.xml @@ -456,6 +456,22 @@ --> + + + diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index abfb6c9e26..921c010340 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -12830,6 +12830,7 @@ reapply_connection(NMDevice *self, NMConnection *con_old, NMConnection *con_new) * the current settings connection * @version_id: either zero, or the current version id for the applied * connection. + * @reapply_flags: the #NMDeviceReapplyFlags. * @audit_args: on return, a string representing the changes * @error: the error if %FALSE is returned * @@ -12839,11 +12840,12 @@ reapply_connection(NMDevice *self, NMConnection *con_old, NMConnection *con_new) * Return: %FALSE if the new configuration can not be reapplied. */ static gboolean -check_and_reapply_connection(NMDevice *self, - NMConnection *connection, - guint64 version_id, - char **audit_args, - GError **error) +check_and_reapply_connection(NMDevice *self, + NMConnection *connection, + guint64 version_id, + NMDeviceReapplyFlags reapply_flags, + char **audit_args, + GError **error) { NMDeviceClass *klass = NM_DEVICE_GET_CLASS(self); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); @@ -13011,7 +13013,12 @@ check_and_reapply_connection(NMDevice *self, reactivate_proxy_config(self); - nm_device_l3cfg_commit(self, NM_L3_CFG_COMMIT_TYPE_REAPPLY, FALSE); + nm_device_l3cfg_commit( + self, + NM_FLAGS_HAS(reapply_flags, NM_DEVICE_REAPPLY_FLAGS_PRESERVE_EXTERNAL_IP) + ? NM_L3_CFG_COMMIT_TYPE_UPDATE + : NM_L3_CFG_COMMIT_TYPE_REAPPLY, + FALSE); } if (priv->state >= NM_DEVICE_STATE_IP_CHECK) @@ -13028,12 +13035,18 @@ nm_device_reapply(NMDevice *self, NMConnection *connection, GError **error) { g_return_val_if_fail(NM_IS_DEVICE(self), FALSE); - return check_and_reapply_connection(self, connection, 0, NULL, error); + return check_and_reapply_connection(self, + connection, + 0, + NM_DEVICE_REAPPLY_FLAGS_NONE, + NULL, + error); } typedef struct { - NMConnection *connection; - guint64 version_id; + NMConnection *connection; + guint64 version_id; + NMDeviceReapplyFlags reapply_flags; } ReapplyData; static void @@ -13044,16 +13057,16 @@ reapply_cb(NMDevice *self, gpointer user_data) { ReapplyData *reapply_data = user_data; - guint64 version_id = 0; - gs_unref_object NMConnection *connection = NULL; - GError *local = NULL; - gs_free char *audit_args = NULL; + guint64 version_id; + gs_unref_object NMConnection *connection = NULL; + NMDeviceReapplyFlags reapply_flags; + GError *local = NULL; + gs_free char *audit_args = NULL; - if (reapply_data) { - connection = reapply_data->connection; - version_id = reapply_data->version_id; - g_slice_free(ReapplyData, reapply_data); - } + connection = reapply_data->connection; + version_id = reapply_data->version_id; + reapply_flags = reapply_data->reapply_flags; + nm_g_slice_free(reapply_data); if (error) { nm_audit_log_device_op(NM_AUDIT_OP_DEVICE_REAPPLY, @@ -13073,6 +13086,7 @@ reapply_cb(NMDevice *self, connection ?: nm_device_get_settings_connection_get_connection(self), version_id, + reapply_flags, &audit_args, &local)) { nm_audit_log_device_op(NM_AUDIT_OP_DEVICE_REAPPLY, @@ -13106,12 +13120,12 @@ impl_device_reapply(NMDBusObject *obj, ReapplyData *reapply_data; gs_unref_variant GVariant *settings = NULL; guint64 version_id; - guint32 flags; + guint32 reapply_flags_u; + NMDeviceReapplyFlags reapply_flags; - g_variant_get(parameters, "(@a{sa{sv}}tu)", &settings, &version_id, &flags); + g_variant_get(parameters, "(@a{sa{sv}}tu)", &settings, &version_id, &reapply_flags_u); - /* No flags supported as of now. */ - if (flags != 0) { + if (NM_FLAGS_ANY(reapply_flags_u, ~((guint32) NM_DEVICE_REAPPLY_FLAGS_PRESERVE_EXTERNAL_IP))) { error = g_error_new_literal(NM_DEVICE_ERROR, NM_DEVICE_ERROR_FAILED, "Invalid flags specified"); nm_audit_log_device_op(NM_AUDIT_OP_DEVICE_REAPPLY, @@ -13124,6 +13138,9 @@ impl_device_reapply(NMDBusObject *obj, return; } + reapply_flags = reapply_flags_u; + nm_assert(reapply_flags_u == reapply_flags); + if (priv->state < NM_DEVICE_STATE_PREPARE || priv->state > NM_DEVICE_STATE_ACTIVATED) { error = g_error_new_literal(NM_DEVICE_ERROR, NM_DEVICE_ERROR_NOT_ACTIVE, @@ -13161,12 +13178,12 @@ impl_device_reapply(NMDBusObject *obj, nm_connection_clear_secrets(connection); } - if (connection || version_id) { - reapply_data = g_slice_new(ReapplyData); - reapply_data->connection = connection; - reapply_data->version_id = version_id; - } else - reapply_data = NULL; + reapply_data = g_slice_new(ReapplyData); + *reapply_data = (ReapplyData){ + .connection = connection, + .version_id = version_id, + .reapply_flags = reapply_flags, + }; nm_device_auth_request(self, invocation, diff --git a/src/libnm-client-impl/libnm.ver b/src/libnm-client-impl/libnm.ver index 9083ac3fd1..8c9a4ef158 100644 --- a/src/libnm-client-impl/libnm.ver +++ b/src/libnm-client-impl/libnm.ver @@ -1885,6 +1885,7 @@ global: nm_client_wait_shutdown; nm_client_wait_shutdown_finish; nm_device_loopback_get_type; + nm_device_reapply_flags_get_type; nm_range_cmp; nm_range_from_str; nm_range_get_range; diff --git a/src/libnm-client-impl/nm-device.c b/src/libnm-client-impl/nm-device.c index fcf07a6b52..ec246afc1a 100644 --- a/src/libnm-client-impl/nm-device.c +++ b/src/libnm-client-impl/nm-device.c @@ -2499,7 +2499,7 @@ nm_device_reapply_finish(NMDevice *device, GAsyncResult *result, GError **error) /** * nm_device_get_applied_connection: * @device: a #NMDevice - * @flags: the flags argument. Currently, this value must always be zero. + * @flags: the flags argument. See #NMDeviceReapplyFlags. * @version_id: (out) (allow-none): returns the current version id of * the applied connection * @cancellable: a #GCancellable, or %NULL @@ -2562,7 +2562,7 @@ nm_device_get_applied_connection(NMDevice *device, /** * nm_device_get_applied_connection_async: * @device: a #NMDevice - * @flags: the flags argument. Currently, this value must always be zero. + * @flags: the flags argument. See #NMDeviceReapplyFlags. * @cancellable: a #GCancellable, or %NULL * @callback: callback to be called when the reapply operation completes * @user_data: caller-specific data passed to @callback diff --git a/src/libnm-core-public/nm-dbus-interface.h b/src/libnm-core-public/nm-dbus-interface.h index bf8eba67d0..9f64f75f0b 100644 --- a/src/libnm-core-public/nm-dbus-interface.h +++ b/src/libnm-core-public/nm-dbus-interface.h @@ -1161,6 +1161,22 @@ typedef enum /*< flags >*/ { NM_SETTINGS_UPDATE2_FLAG_NO_REAPPLY = 0x40, } NMSettingsUpdate2Flags; +/** + * NMDeviceReapplyFlags: + * @NM_DEVICE_REAPPLY_FLAGS_NONE: no flag set. + * @NM_DEVICE_REAPPLY_FLAGS_PRESERVE_EXTERNAL_IP: during reapply, + * preserve external IP addresses and routes. + * + * Flags for the Reapply() D-Bus call of a device and + * nm_device_reapply_async(). + * + * Since: 1.42 + */ +typedef enum /*< flags >*/ { + NM_DEVICE_REAPPLY_FLAGS_NONE = 0, + NM_DEVICE_REAPPLY_FLAGS_PRESERVE_EXTERNAL_IP = 0x1, +} NMDeviceReapplyFlags; + /** * NMTernary: * @NM_TERNARY_DEFAULT: use the globally-configured default value. From a467f55bef47056fdd92832337007e5390de47f2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 7 Dec 2022 11:57:27 +0100 Subject: [PATCH 5/9] examples: add python example for reapply --- Makefile.examples | 1 + examples/python/gi/device-reapply.py | 147 +++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100755 examples/python/gi/device-reapply.py diff --git a/Makefile.examples b/Makefile.examples index 076af724ae..3b26220990 100644 --- a/Makefile.examples +++ b/Makefile.examples @@ -182,6 +182,7 @@ EXTRA_DIST += \ examples/python/gi/add_connection.py \ examples/python/gi/checkpoint.py \ examples/python/gi/deactivate-all.py \ + examples/python/gi/device-reapply.py \ examples/python/gi/device-state-ip4config.py \ examples/python/gi/dns.py \ examples/python/gi/firewall-zone.py \ diff --git a/examples/python/gi/device-reapply.py b/examples/python/gi/device-reapply.py new file mode 100755 index 0000000000..9f5d330677 --- /dev/null +++ b/examples/python/gi/device-reapply.py @@ -0,0 +1,147 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: LGPL-2.1-or-later + +import os +import sys + +import gi + +gi.require_version("NM", "1.0") +from gi.repository import NM, GLib, Gio, GObject + + +def eprint(*args, **kwargs): + print(*args, file=sys.stderr, **kwargs) + + +def kf_from_data(data): + kf = GLib.KeyFile.new() + kf.load_from_data(data, 18446744073709551615, GLib.KeyFileFlags.NONE) + return kf + + +def kf_to_data(kf): + data, l = kf.to_data() + return data + + +def connection_to_kf(connection): + return kf_to_data(NM.keyfile_write(connection, NM.KeyfileHandlerFlags.NONE)) + + +def connection_from_kf(data): + base_dir = os.getcwd() + return NM.keyfile_read(kf_from_data(data), base_dir, NM.KeyfileHandlerFlags.NONE) + + +def connection_from_stdin(): + return connection_from_kf(sys.stdin.read()) + + +def device_get_applied_connection(device): + mainloop = GLib.MainLoop() + r = [] + + def cb(device, result): + try: + connection, version_id = device.get_applied_connection_finish(result) + except Exception as e: + r.append(e) + else: + r.append(connection) + r.append(version_id) + mainloop.quit() + + device.get_applied_connection_async(0, None, cb) + mainloop.run() + if len(r) == 1: + raise r[0] + connection, version_id = r + return connection, version_id + + +def device_reapply(device, connection, version_id, reapply_flags): + mainloop = GLib.MainLoop() + r = [] + + def cb(device, result): + try: + device.reapply_finish(result) + except Exception as e: + r.append(e) + mainloop.quit() + + device.reapply_async(connection, version_id or 0, reapply_flags, None, cb) + mainloop.run() + if len(r) == 1: + raise r[0] + + +def parse_args(): + import argparse + + parser = argparse.ArgumentParser( + prog="device-reapply.py", + description="Example program to interact with the applied connection", + ) + + parser.add_argument("mode", choices=["get", "reapply", "modify"]) + parser.add_argument("device") + parser.add_argument("-V", "--version-id", type=int) + parser.add_argument("-s", "--stdin", action="store_true") + parser.add_argument("-p", "--preserve-external-ip", action="store_true") + + return parser.parse_args() + + +def main(): + args = parse_args() + + nmc = NM.Client.new() + + device = [d for d in nmc.get_devices() if d.get_iface() == args.device] + if not device: + raise Exception(f'Device "{args.device}" not found') + if len(device) != 1: + raise Exception(f'Not unique device "{args.device}" found') + device = device[0] + + assert not args.stdin or args.mode == "modify" + assert not args.preserve_external_ip or args.mode in ["modify", "reapply"] + + if args.mode == "get": + connection, version_id = device_get_applied_connection(device) + + version_id_matches = args.version_id is None or args.version_id == version_id + + print( + f'# Applied connection on "{device.get_iface()}": "{connection.get_id()}" ({connection.get_uuid()}, {connection.get_connection_type()})' + ) + s = "" if version_id_matches else f" (expected {args.version_id})" + print(f"# version-id={version_id}{s}") + print(f"#") + print(f"{connection_to_kf(connection)}") + + if not version_id_matches: + eprint( + f"Applied version-id does not match (expects {args.version_id} but got {version_id})" + ) + sys.exit(1) + sys.exit(0) + + if args.mode == "reapply": + new_connection = None + elif args.stdin: + new_connection = connection_from_stdin() + else: + new_connection, _ = device_get_applied_connection(device) + + reapply_flags = 0 + if args.preserve_external_ip: + reapply_flags = 1 # NM.DeviceReapplyFlags.PRESERVE_EXTERNAL_IP + + device_reapply(device, new_connection, args.version_id, reapply_flags) + + +if __name__ == "__main__": + main() From bc6098d441060c14cd816a0a62480d0dac698fae Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 19:18:41 +0100 Subject: [PATCH 6/9] libnm: add internal nmc_client_has_{version_info_v,version_info_capability,capability}() helper In the end, it turned out I don't need them. They still seem useful, because they show how to use this API. In particular for how the bitfield should be parsed. --- src/libnm-client-aux-extern/nm-libnm-aux.c | 61 ++++++++++++++++++++++ src/libnm-client-aux-extern/nm-libnm-aux.h | 4 ++ 2 files changed, 65 insertions(+) diff --git a/src/libnm-client-aux-extern/nm-libnm-aux.c b/src/libnm-client-aux-extern/nm-libnm-aux.c index 573eed1d48..82a75de697 100644 --- a/src/libnm-client-aux-extern/nm-libnm-aux.c +++ b/src/libnm-client-aux-extern/nm-libnm-aux.c @@ -135,3 +135,64 @@ nmc_client_new_waitsync(GCancellable *cancellable, } return TRUE; } + +/*****************************************************************************/ + +guint32 +nmc_client_has_version_info_v(NMClient *nmc) +{ + const guint32 *ver; + gsize len; + + ver = nm_client_get_version_info(nmc, &len); + if (len < 1) + return 0; + return ver[0]; +} + +gboolean +nmc_client_has_version_info_capability(NMClient *nmc, NMVersionInfoCapability capability) +{ + const guint32 *ver; + gsize len; + gsize idx; + gsize idx_hi; + gsize idx_lo; + + ver = nm_client_get_version_info(nmc, &len); + + if (len < 2) + return FALSE; + + len--; + ver++; + + idx = (gsize) capability; + if (idx >= G_MAXSIZE - 31u) + return FALSE; + + idx_hi = ((idx + 31u) / 32u); + idx_lo = (idx % 32u); + + if (idx_hi > len) + return FALSE; + + return NM_FLAGS_ANY(ver[idx_hi], (1ull << idx_lo)); +} + +gboolean +nmc_client_has_capability(NMClient *nmc, NMCapability capability) +{ + const guint32 *caps; + gsize len; + gsize i; + + caps = nm_client_get_capabilities(nmc, &len); + + for (i = 0; i < len; i++) { + if (caps[i] == capability) + return TRUE; + } + + return FALSE; +} diff --git a/src/libnm-client-aux-extern/nm-libnm-aux.h b/src/libnm-client-aux-extern/nm-libnm-aux.h index 44344fd0b8..ae949cc5c6 100644 --- a/src/libnm-client-aux-extern/nm-libnm-aux.h +++ b/src/libnm-client-aux-extern/nm-libnm-aux.h @@ -21,4 +21,8 @@ gboolean nmc_client_new_waitsync(GCancellable *cancellable, const char *first_property_name, ...); +guint32 nmc_client_has_version_info_v(NMClient *nmc); +gboolean nmc_client_has_version_info_capability(NMClient *nmc, NMVersionInfoCapability capability); +gboolean nmc_client_has_capability(NMClient *nmc, NMCapability capability); + #endif /* __NM_LIBNM_AUX_H__ */ From 29b0420be72fa4ae6878f6dd82d34c245a538351 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 19:30:02 +0100 Subject: [PATCH 7/9] nm-cloud-setup: set preserve-external-ip flag during reapply Externally added IP addresses/routes should be preserved by nm-cloud-setup. This allows other tools to also configure the interface and the Reapply() call from nm-cloud-setup would not interfere with those tools. https://bugzilla.redhat.com/show_bug.cgi?id=2132754 --- src/nm-cloud-setup/main.c | 8 ++++++++ src/nm-cloud-setup/nm-cloud-setup-utils.c | 16 +++++++++++++++- src/nm-cloud-setup/nm-cloud-setup-utils.h | 1 + 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index 026c6701b4..c3f3332095 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -450,6 +450,7 @@ _config_one(GCancellable *sigterm_cancellable, gboolean version_id_changed; guint try_count; gboolean any_changes = FALSE; + gboolean maybe_no_preserved_external_ip; g_main_context_iteration(NULL, FALSE); @@ -538,10 +539,17 @@ try_again: /* we are about to call Reapply(). Even if that fails, it counts as if we changed something. */ any_changes = TRUE; + /* "preserve-external-ip" flag was only introduced in 1.41.6 (but maybe backported!). + * If we run 1.41.6+, we are sure that it's gonna work. Otherwise, we take into account + * that the call might fail due to the invalid flag and we retry. */ + maybe_no_preserved_external_ip = + (nmc_client_has_version_info_v(nmc) < NM_ENCODE_VERSION(1, 41, 6)); + if (!nmcs_device_reapply(device, sigterm_cancellable, applied_connection, applied_version_id, + maybe_no_preserved_external_ip, &version_id_changed, &error)) { if (version_id_changed && try_count < 5) { diff --git a/src/nm-cloud-setup/nm-cloud-setup-utils.c b/src/nm-cloud-setup/nm-cloud-setup-utils.c index e8c9943d78..cb8e9f559c 100644 --- a/src/nm-cloud-setup/nm-cloud-setup-utils.c +++ b/src/nm-cloud-setup/nm-cloud-setup-utils.c @@ -822,6 +822,7 @@ nmcs_device_reapply(NMDevice *device, GCancellable *sigterm_cancellable, NMConnection *connection, guint64 version_id, + gboolean maybe_no_preserved_external_ip, gboolean *out_version_id_changed, GError **error) { @@ -829,11 +830,13 @@ nmcs_device_reapply(NMDevice *device, DeviceReapplyData data = { .main_loop = main_loop, }; + NMDeviceReapplyFlags reapply_flags = NM_DEVICE_REAPPLY_FLAGS_PRESERVE_EXTERNAL_IP; +again: nm_device_reapply_async(device, connection, version_id, - 0, + reapply_flags, sigterm_cancellable, _nmcs_device_reapply_cb, &data); @@ -841,6 +844,17 @@ nmcs_device_reapply(NMDevice *device, g_main_loop_run(main_loop); if (data.error) { + if (maybe_no_preserved_external_ip + && reapply_flags == NM_DEVICE_REAPPLY_FLAGS_PRESERVE_EXTERNAL_IP + && nm_g_error_matches(data.error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_FAILED)) { + /* Hm? Maybe we running against an older version of NetworkManager that + * doesn't support "preserve-external-ip" flags? Retry without the flag. + * + * Note that recent version would reject invalid flags with NM_DEVICE_ERROR_INVALID_ARGUMENT, + * but we want to detect old daemon versions here. */ + reapply_flags = NM_DEVICE_REAPPLY_FLAGS_NONE; + goto again; + } NM_SET_OUT( out_version_id_changed, g_error_matches(data.error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_VERSION_ID_MISMATCH)); diff --git a/src/nm-cloud-setup/nm-cloud-setup-utils.h b/src/nm-cloud-setup/nm-cloud-setup-utils.h index 4131abfe7c..fed0f4b6ed 100644 --- a/src/nm-cloud-setup/nm-cloud-setup-utils.h +++ b/src/nm-cloud-setup/nm-cloud-setup-utils.h @@ -136,6 +136,7 @@ gboolean nmcs_device_reapply(NMDevice *device, GCancellable *sigterm_cancellable, NMConnection *connection, guint64 version_id, + gboolean maybe_no_preserved_external_ip, gboolean *out_version_id_changed, GError **error); From bbd32fba15cfa8e71dd6cf9018355e441e326a9d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 12:11:18 +0100 Subject: [PATCH 8/9] nm-cloud-setup: refactor skipping reapply be checking for skip first There should be no change in behavior, but this way seems nicer. Now _nmc_mangle_connection() doesn't return FALSE, it always will try to mangle the connection and requires the caller to first check whether that is appropriate. Just move some code outside of _nmc_mangle_connection() and let the caller check for the skip first. The point is consistency, as the caller already does some checks to whether skip the reapply. So it should do all the checks, so that "mangle" never fails/skips. --- src/nm-cloud-setup/main.c | 51 ++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index c3f3332095..c0c78e0fc2 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -251,24 +251,38 @@ _get_config(GCancellable *sigterm_cancellable, NMCSProvider *provider, NMClient /*****************************************************************************/ static gboolean -_nmc_skip_connection(NMConnection *connection) +_nmc_skip_connection_by_user_data(NMConnection *connection) { NMSettingUser *s_user; const char *v; - s_user = NM_SETTING_USER(nm_connection_get_setting(connection, NM_TYPE_SETTING_USER)); - if (!s_user) - return FALSE; - #define USER_TAG_SKIP "org.freedesktop.nm-cloud-setup.skip" nm_assert(nm_setting_user_check_key(USER_TAG_SKIP, NULL)); - v = nm_setting_user_get_data(s_user, USER_TAG_SKIP); - return _nm_utils_ascii_str_to_bool(v, FALSE); + s_user = NM_SETTING_USER(nm_connection_get_setting(connection, NM_TYPE_SETTING_USER)); + if (s_user) { + v = nm_setting_user_get_data(s_user, USER_TAG_SKIP); + if (_nm_utils_ascii_str_to_bool(v, FALSE)) + return TRUE; + } + + return FALSE; } static gboolean +_nmc_skip_connection_by_type(NMConnection *connection) +{ + if (!nm_streq0(nm_connection_get_connection_type(connection), NM_SETTING_WIRED_SETTING_NAME)) + return TRUE; + + if (!nm_connection_get_setting_ip4_config(connection)) + return TRUE; + + return FALSE; +} + +static void _nmc_mangle_connection(NMDevice *device, NMConnection *connection, const NMCSProviderGetConfigResult *result, @@ -291,12 +305,8 @@ _nmc_mangle_connection(NMDevice *device, NM_SET_OUT(out_skipped_single_addr, FALSE); NM_SET_OUT(out_changed, FALSE); - if (!nm_streq0(nm_connection_get_connection_type(connection), NM_SETTING_WIRED_SETTING_NAME)) - return FALSE; - s_ip = nm_connection_get_setting_ip4_config(connection); - if (!s_ip) - return FALSE; + nm_assert(NM_IS_SETTING_IP4_CONFIG(s_ip)); if ((ac = nm_device_get_active_connection(device)) && (remote_connection = NM_CONNECTION(nm_active_connection_get_connection(ac)))) @@ -428,7 +438,6 @@ _nmc_mangle_connection(NMDevice *device, rules_new->len); NM_SET_OUT(out_changed, addrs_changed || routes_changed || rules_changed); - return TRUE; } /*****************************************************************************/ @@ -497,23 +506,25 @@ try_again: return any_changes; } - if (_nmc_skip_connection(applied_connection)) { + if (_nmc_skip_connection_by_user_data(applied_connection)) { _LOGD("config device %s: skip applied connection due to user data %s", hwaddr, USER_TAG_SKIP); return any_changes; } - if (!_nmc_mangle_connection(device, - applied_connection, - result, - config_data, - &skipped_single_addr, - &changed)) { + if (_nmc_skip_connection_by_type(applied_connection)) { _LOGD("config device %s: device has no suitable applied connection. Skip", hwaddr); return any_changes; } + _nmc_mangle_connection(device, + applied_connection, + result, + config_data, + &skipped_single_addr, + &changed); + if (!changed) { if (skipped_single_addr) { _LOGD("config device %s: device needs no update to applied connection \"%s\" (%s) " From 911b55014082558ace27acc32853c45fdf09874e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 14:17:29 +0100 Subject: [PATCH 9/9] nm-cloud-setup: simplify clearing variables in retry loop The label "try_again" is only reached by one goto. So it was correct and sufficient to reset the state only there. It is still error prone. The slighlty clearer approach is to clear the state at each begin of the "try_again" step. There should be no change in behavior. I didn't confirm, but an optimizing compiler should (could) be able to see that the cleanup is only necessary on retry, and generate the same code as before. In any case, we should write code that is easier to read, not optimize for something that a compiler should be able to optimize itself. --- src/nm-cloud-setup/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index c0c78e0fc2..d6802634bd 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -493,6 +493,8 @@ _config_one(GCancellable *sigterm_cancellable, try_count = 0; try_again: + g_clear_object(&applied_connection); + g_clear_error(&error); applied_connection = nmcs_device_get_applied_connection(device, sigterm_cancellable, @@ -565,8 +567,6 @@ try_again: &error)) { if (version_id_changed && try_count < 5) { _LOGD("config device %s: applied connection changed in the meantime. Retry...", hwaddr); - g_clear_object(&applied_connection); - g_clear_error(&error); try_count++; goto try_again; }