From 2e70391033b5b3414491edcd8656499512342619 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 --- shared/nm-std-aux/nm-std-aux.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/shared/nm-std-aux/nm-std-aux.h b/shared/nm-std-aux/nm-std-aux.h index 0133a7964..7f240a793 100644 --- a/shared/nm-std-aux/nm-std-aux.h +++ b/shared/nm-std-aux/nm-std-aux.h @@ -337,6 +337,10 @@ nm_streq0 (const char *s1, const char *s2) #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, ...) \ @@ -386,6 +390,10 @@ nm_streq0 (const char *s1, const char *s2) #define _NM_IN_SETOP_EVAL_14(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_13 (op, op_eq, _x, __VA_ARGS__) #define _NM_IN_SETOP_EVAL_15(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_14 (op, op_eq, _x, __VA_ARGS__) #define _NM_IN_SETOP_EVAL_16(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_15 (op, op_eq, _x, __VA_ARGS__) +#define _NM_IN_SETOP_EVAL_17(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_16 (op, op_eq, _x, __VA_ARGS__) +#define _NM_IN_SETOP_EVAL_18(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_17 (op, op_eq, _x, __VA_ARGS__) +#define _NM_IN_SETOP_EVAL_19(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_18 (op, op_eq, _x, __VA_ARGS__) +#define _NM_IN_SETOP_EVAL_20(op, op_eq, _x, y, ...) (op_eq (_x, y)) op _NM_IN_SETOP_EVAL_19 (op, op_eq, _x, __VA_ARGS__) /*****************************************************************************/ From 04d6ca1fb8bdbfffd70a257424f9e8c29fcb8037 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') --- 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 e36eba61b..164f6aaa7 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 746dc119a6bceb6a08b4dc9f3798d0b59a4b8575 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 --- 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 164f6aaa7..71332ba39 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)); } /*****************************************************************************/