From 908d1f6cb79739f3307bfcfd64b338e215f9e4a6 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 4 Aug 2020 18:36:39 +0200 Subject: [PATCH 1/3] shared: extend NM_IN_STRSET and NM_IN_SET to support up to 20 args https://bugzilla.redhat.com/show_bug.cgi?id=1847814 (cherry picked from commit 2e70391033b5b3414491edcd8656499512342619) --- shared/nm-glib-aux/nm-macros-internal.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/shared/nm-glib-aux/nm-macros-internal.h b/shared/nm-glib-aux/nm-macros-internal.h index f56ed85699..15bcd7e585 100644 --- a/shared/nm-glib-aux/nm-macros-internal.h +++ b/shared/nm-glib-aux/nm-macros-internal.h @@ -732,6 +732,10 @@ NM_G_ERROR_MSG (GError *error) #define _NM_IN_SET_EVAL_14(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_13 (op, _x, __VA_ARGS__) #define _NM_IN_SET_EVAL_15(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_14 (op, _x, __VA_ARGS__) #define _NM_IN_SET_EVAL_16(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_15 (op, _x, __VA_ARGS__) +#define _NM_IN_SET_EVAL_17(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_16 (op, _x, __VA_ARGS__) +#define _NM_IN_SET_EVAL_18(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_17 (op, _x, __VA_ARGS__) +#define _NM_IN_SET_EVAL_19(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_18 (op, _x, __VA_ARGS__) +#define _NM_IN_SET_EVAL_20(op, _x, y, ...) (_x == (y)) op _NM_IN_SET_EVAL_19 (op, _x, __VA_ARGS__) #define _NM_IN_SET_EVAL_N2(op, _x, n, ...) (_NM_IN_SET_EVAL_##n(op, _x, __VA_ARGS__)) #define _NM_IN_SET_EVAL_N(op, type, x, n, ...) \ @@ -798,6 +802,10 @@ _NM_IN_STRSET_streq (const char *x, const char *s) #define _NM_IN_STRSET_EVAL_14(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_13 (op, _x, __VA_ARGS__) #define _NM_IN_STRSET_EVAL_15(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_14 (op, _x, __VA_ARGS__) #define _NM_IN_STRSET_EVAL_16(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_15 (op, _x, __VA_ARGS__) +#define _NM_IN_STRSET_EVAL_17(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_16 (op, _x, __VA_ARGS__) +#define _NM_IN_STRSET_EVAL_18(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_17 (op, _x, __VA_ARGS__) +#define _NM_IN_STRSET_EVAL_19(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_18 (op, _x, __VA_ARGS__) +#define _NM_IN_STRSET_EVAL_20(op, _x, y, ...) _NM_IN_STRSET_streq (_x, y) op _NM_IN_STRSET_EVAL_19 (op, _x, __VA_ARGS__) #define _NM_IN_STRSET_EVAL_N2(op, _x, n, ...) (_NM_IN_STRSET_EVAL_##n(op, _x, __VA_ARGS__)) #define _NM_IN_STRSET_EVAL_N(op, x, n, ...) \ From 63b5274dda0c52148ec8e8ca41e94e47b1e7d653 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 4 Aug 2020 18:19:47 +0200 Subject: [PATCH 2/3] bond: fix can_reapply_change() false positives can_reapply_change() would wrongly return true for unsupported reapply values because it used 'nm_setting_bond_get_option_default()' that is ill-named because it returns the overriden option other than its default value. https://bugzilla.redhat.com/show_bug.cgi?id=1847814 Fixes: 9bd07336ef16 ('bond: bond options logic rework') (cherry picked from commit 04d6ca1fb8bdbfffd70a257424f9e8c29fcb8037) --- src/devices/nm-device-bond.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index e36eba61b0..164f6aaa72 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -516,14 +516,12 @@ create_and_realize (NMDevice *device, static gboolean check_changed_options (NMSettingBond *s_a, NMSettingBond *s_b, GError **error) { - guint i, num; - const char *name = NULL, *value_a = NULL, *value_b = NULL; + const char **option_list; - /* Check that options in @s_a have compatible changes in @s_b */ + option_list = nm_setting_bond_get_valid_options (NULL); - num = nm_setting_bond_get_num_options (s_a); - for (i = 0; i < num; i++) { - nm_setting_bond_get_option (s_a, i, &name, &value_a); + for (; *option_list; ++option_list) { + const char *name = *option_list; /* We support changes to these */ if (NM_IN_STRSET (name, @@ -532,15 +530,9 @@ check_changed_options (NMSettingBond *s_a, NMSettingBond *s_b, GError **error) continue; } - /* Missing in @s_b, but has a default value in @s_a */ - value_b = nm_setting_bond_get_option_by_name (s_b, name); - if ( !value_b - && nm_streq0 (value_a, nm_setting_bond_get_option_default (s_a, name))) { - continue; - } - /* Reject any other changes */ - if (!nm_streq0 (value_a, value_b)) { + if (!nm_streq0 (nm_setting_bond_get_option_normalized (s_a, name), + nm_setting_bond_get_option_normalized (s_b, name))) { g_set_error (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_INCOMPATIBLE_CONNECTION, @@ -562,7 +554,6 @@ can_reapply_change (NMDevice *device, GError **error) { NMDeviceClass *device_class; - NMSettingBond *s_bond_old, *s_bond_new; /* Only handle bond setting here, delegate other settings to parent class */ if (nm_streq (setting_name, NM_SETTING_BOND_SETTING_NAME)) { @@ -572,15 +563,7 @@ can_reapply_change (NMDevice *device, NM_SETTING_BOND_OPTIONS)) return FALSE; - s_bond_old = NM_SETTING_BOND (s_old); - s_bond_new = NM_SETTING_BOND (s_new); - - if ( !check_changed_options (s_bond_old, s_bond_new, error) - || !check_changed_options (s_bond_new, s_bond_old, error)) { - return FALSE; - } - - return TRUE; + return check_changed_options (NM_SETTING_BOND (s_old), NM_SETTING_BOND (s_new), error); } device_class = NM_DEVICE_CLASS (nm_device_bond_parent_class); From 88a399637a5279f43fd8fc7c511547fa1d179295 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 4 Aug 2020 17:49:04 +0200 Subject: [PATCH 3/3] bond: let 'reapply()' reapply all supported options Reapply now handles all the options supported by kernel and NM, meaning that some options are simply not allowed to be set while keeping the bond up, one of those options is the mode for instance. https://bugzilla.redhat.com/show_bug.cgi?id=1847814 (cherry picked from commit 746dc119a6bceb6a08b4dc9f3798d0b59a4b8575) --- src/devices/nm-device-bond.c | 141 +++++++++++++++++++++++++---------- 1 file changed, 101 insertions(+), 40 deletions(-) diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 164f6aaa72..71332ba390 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -183,7 +183,6 @@ master_update_slave_connection (NMDevice *self, static void set_arp_targets (NMDevice *device, - NMBondMode mode, const char *cur_arp_ip_target, const char *new_arp_ip_target) { @@ -296,15 +295,39 @@ set_bond_attr_active_slave (NMDevice *device, NMSettingBond *s_bond) _set_bond_attr (device, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, value); } +static void +set_bond_attrs_or_default (NMDevice *device, NMSettingBond *s_bond, const char *const *attr_v) +{ + nm_assert (NM_IS_DEVICE (device)); + nm_assert (s_bond); + nm_assert (attr_v); + + for ( ; *attr_v ; ++attr_v) + set_bond_attr_or_default (device, s_bond, *attr_v); +} + +static void +set_bond_arp_ip_targets (NMDevice *device, NMSettingBond *s_bond) +{ + int ifindex = nm_device_get_ifindex (device); + gs_free char *cur_arp_ip_target = NULL; + + /* ARP targets: clear and initialize the list */ + cur_arp_ip_target = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), + ifindex, + NM_SETTING_BOND_OPTION_ARP_IP_TARGET); + set_arp_targets (device, + cur_arp_ip_target, + nm_setting_bond_get_option_or_default (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET)); +} + static gboolean apply_bonding_config (NMDeviceBond *self) { NMDevice *device = NM_DEVICE (self); - int ifindex = nm_device_get_ifindex (device); NMSettingBond *s_bond; NMBondMode mode; const char *mode_str; - gs_free char *cur_arp_ip_target = NULL; s_bond = nm_device_get_applied_setting (device, NM_TYPE_SETTING_BOND); g_return_val_if_fail (s_bond, FALSE); @@ -318,40 +341,34 @@ apply_bonding_config (NMDeviceBond *self) */ set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MODE); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MIIMON); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_UPDELAY); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ARP_VALIDATE); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PRIMARY); - - /* ARP targets: clear and initialize the list */ - cur_arp_ip_target = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), - ifindex, - NM_SETTING_BOND_OPTION_ARP_IP_TARGET); - set_arp_targets (device, - mode, - cur_arp_ip_target, - nm_setting_bond_get_option_or_default (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET)); - - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM); + set_bond_arp_ip_targets (device, s_bond); set_bond_attr_active_slave (device, s_bond); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_SELECT); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_LACP_RATE); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_LP_INTERVAL); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MIN_LINKS); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); + + set_bond_attrs_or_default (device, + s_bond, + NM_MAKE_STRV (NM_SETTING_BOND_OPTION_MIIMON, + NM_SETTING_BOND_OPTION_UPDELAY, + NM_SETTING_BOND_OPTION_DOWNDELAY, + NM_SETTING_BOND_OPTION_ARP_INTERVAL, + NM_SETTING_BOND_OPTION_ARP_VALIDATE, + NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, + NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, + NM_SETTING_BOND_OPTION_AD_SELECT, + NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, + NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE, + NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS, + NM_SETTING_BOND_OPTION_FAIL_OVER_MAC, + NM_SETTING_BOND_OPTION_LACP_RATE, + NM_SETTING_BOND_OPTION_LP_INTERVAL, + NM_SETTING_BOND_OPTION_MIN_LINKS, + NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, + NM_SETTING_BOND_OPTION_PRIMARY_RESELECT, + NM_SETTING_BOND_OPTION_RESEND_IGMP, + NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB, + NM_SETTING_BOND_OPTION_USE_CARRIER, + NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY, + NM_SETTING_BOND_OPTION_NUM_GRAT_ARP)); return TRUE; } @@ -525,8 +542,26 @@ check_changed_options (NMSettingBond *s_a, NMSettingBond *s_b, GError **error) /* We support changes to these */ if (NM_IN_STRSET (name, - NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, - NM_SETTING_BOND_OPTION_PRIMARY)) { + NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_MIIMON, + NM_SETTING_BOND_OPTION_UPDELAY, + NM_SETTING_BOND_OPTION_DOWNDELAY, + NM_SETTING_BOND_OPTION_ARP_INTERVAL, + NM_SETTING_BOND_OPTION_ARP_VALIDATE, + NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, + NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, + NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE, + NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS, + NM_SETTING_BOND_OPTION_FAIL_OVER_MAC, + NM_SETTING_BOND_OPTION_LP_INTERVAL, + NM_SETTING_BOND_OPTION_MIN_LINKS, + NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, + NM_SETTING_BOND_OPTION_PRIMARY_RESELECT, + NM_SETTING_BOND_OPTION_RESEND_IGMP, + NM_SETTING_BOND_OPTION_USE_CARRIER, + NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY, + NM_SETTING_BOND_OPTION_NUM_GRAT_ARP)) { continue; } @@ -579,8 +614,8 @@ static void reapply_connection (NMDevice *device, NMConnection *con_old, NMConnection *con_new) { NMDeviceBond *self = NM_DEVICE_BOND (device); - const char *value; NMSettingBond *s_bond; + const char *value; NMBondMode mode; NM_DEVICE_CLASS (nm_device_bond_parent_class)->reapply_connection (device, @@ -595,8 +630,34 @@ reapply_connection (NMDevice *device, NMConnection *con_old, NMConnection *con_n mode = _nm_setting_bond_mode_from_string (value); g_return_if_fail (mode != NM_BOND_MODE_UNKNOWN); - set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_PRIMARY); + /* Below we set only the bond options that kernel allows to modify + * while keeping the bond interface up */ + set_bond_attr_active_slave (device, s_bond); + set_bond_arp_ip_targets (device, s_bond); + + set_bond_attrs_or_default (device, + s_bond, + NM_MAKE_STRV (NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_MIIMON, + NM_SETTING_BOND_OPTION_UPDELAY, + NM_SETTING_BOND_OPTION_DOWNDELAY, + NM_SETTING_BOND_OPTION_ARP_INTERVAL, + NM_SETTING_BOND_OPTION_ARP_VALIDATE, + NM_SETTING_BOND_OPTION_PRIMARY, + NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, + NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, + NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE, + NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS, + NM_SETTING_BOND_OPTION_FAIL_OVER_MAC, + NM_SETTING_BOND_OPTION_LP_INTERVAL, + NM_SETTING_BOND_OPTION_MIN_LINKS, + NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, + NM_SETTING_BOND_OPTION_PRIMARY_RESELECT, + NM_SETTING_BOND_OPTION_RESEND_IGMP, + NM_SETTING_BOND_OPTION_USE_CARRIER, + NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY, + NM_SETTING_BOND_OPTION_NUM_GRAT_ARP)); } /*****************************************************************************/