From 58da09439af83d5ff77c6fea574ac672e10e672e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Sep 2020 12:40:58 +0200 Subject: [PATCH] libnm,core: deprecate "active_slave" and alias it for "primary" Setting "active_slave" fails unless the slave is currently present and IFF_UP. That complicates the code, because we cannot set the property at any time, but only under the right circumstances. But really, "active_slave" option is something for debugging. It's not an option which should be set by NetworkManager. The right option instead is "primary", which will tell kernel to make the slave active, when it is ready. Deprecate the "active_slave" option and make it an alias for "primary". https://bugzilla.redhat.com/show_bug.cgi?id=1856640 --- libnm-core/nm-setting-bond.c | 15 +++++++- src/devices/nm-device-bond.c | 74 ++++++------------------------------ 2 files changed, 24 insertions(+), 65 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 3f8e5d0c1..d2640fe9c 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -111,9 +111,9 @@ _nm_assert_bond_meta (const OptionMeta *option_meta) })); return TRUE; case NM_BOND_OPTION_TYPE_IP: - case NM_BOND_OPTION_TYPE_IFNAME: nm_assert (option_meta->val); /* fall-through */ + case NM_BOND_OPTION_TYPE_IFNAME: case NM_BOND_OPTION_TYPE_MAC: nm_assert (!option_meta->list); nm_assert (option_meta->min == 0); @@ -151,7 +151,7 @@ NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE ( } }, { return NULL; }, - { NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, { "", NM_BOND_OPTION_TYPE_IFNAME } }, + { NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, { NULL, NM_BOND_OPTION_TYPE_IFNAME } }, { NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, { "65535", NM_BOND_OPTION_TYPE_INT, 1, 65535 } }, { NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, { NULL, NM_BOND_OPTION_TYPE_MAC } }, { NM_SETTING_BOND_OPTION_AD_SELECT, { "stable", NM_BOND_OPTION_TYPE_BOTH, 0, 2, _option_default_strv_ad_select } }, @@ -295,6 +295,17 @@ _bond_get_option_normalized (NMSettingBond* self, value = _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); if (!value) value = _bond_get_option (self, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); + } else if (NM_IN_STRSET (option, + NM_SETTING_BOND_OPTION_ACTIVE_SLAVE)) { + /* "active_slave" is deprecated, and an alias for "primary". The property + * itself always normalizes to %NULL. */ + value = NULL; + } else if (NM_IN_STRSET (option, + NM_SETTING_BOND_OPTION_PRIMARY)) { + /* "active_slave" is deprecated, and an alias for "primary". */ + value = _bond_get_option (self, NM_SETTING_BOND_OPTION_PRIMARY); + if (!value) + value = _bond_get_option (self, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); } else value = _bond_get_option (self, option); diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index d65c5f5ef..40bd185b8 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -181,12 +181,15 @@ update_connection (NMDevice *device, NMConnection *connection) } /* Read bond options from sysfs and update the Bond setting to match */ - options = nm_setting_bond_get_valid_options (s_bond); + options = nm_setting_bond_get_valid_options (NULL); for (; options[0]; options++) { const char *option = options[0]; gs_free char *value = NULL; char *p; + if (NM_IN_STRSET (option, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE)) + continue; + value = nm_platform_sysctl_master_get_option (nm_device_get_platform (device), ifindex, option); @@ -317,40 +320,6 @@ set_bond_attr_or_default (NMDevice *device, _set_bond_attr (device, opt, value); } -static void -set_bond_attr_active_slave (NMDevice *device, NMSettingBond *s_bond) -{ - NMDeviceBond *self = NM_DEVICE_BOND (device); - const NMPlatformLink *plink; - const char *value; - const char *error_reason; - int ifindex; - - value = nm_setting_bond_get_option_or_default (s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); - if (!value) - return; - - if (!nm_str_is_empty (value)) { - ifindex = nm_device_get_ifindex (device); - plink = nm_platform_link_get_by_ifname (nm_device_get_platform (device), value); - if (!plink) - error_reason = "does not exist"; - else if (plink->master != ifindex) - error_reason = "is not yet enslaved"; - else if (!NM_FLAGS_HAS (plink->n_ifi_flags, IFF_UP)) - error_reason = "is not up"; - else - error_reason = NULL; - - if (error_reason) { - _LOGT (LOGD_BOND, "bond option 'active_slave' not set as device \"%s\" %s", value, error_reason); - return; - } - } - - _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) { @@ -398,7 +367,6 @@ apply_bonding_config (NMDeviceBond *self) set_bond_attr_or_default (device, s_bond, NM_SETTING_BOND_OPTION_MODE); set_bond_arp_ip_targets (device, s_bond); - set_bond_attr_active_slave (device, s_bond); set_bond_attrs_or_default (device, s_bond, @@ -434,45 +402,26 @@ enslave_slave (NMDevice *device, gboolean configure) { NMDeviceBond *self = NM_DEVICE_BOND (device); - gboolean success = TRUE; - const char *slave_iface = nm_device_get_ip_iface (slave); - NMConnection *master_con; nm_device_master_check_slave_physical_port (device, slave, LOGD_BOND); if (configure) { + gboolean success; + nm_device_take_down (slave, TRUE); success = nm_platform_link_enslave (nm_device_get_platform (device), nm_device_get_ip_ifindex (device), nm_device_get_ip_ifindex (slave)); nm_device_bring_up (slave, TRUE, NULL); - if (!success) + if (!success) { + _LOGI (LOGD_BOND, "enslaved bond slave %s: failed", nm_device_get_ip_iface (slave)); return FALSE; - - _LOGI (LOGD_BOND, "enslaved bond slave %s", slave_iface); - - /* The active_slave option can be set only after the interface is enslaved */ - master_con = nm_device_get_applied_connection (device); - if (master_con) { - NMSettingBond *s_bond = nm_connection_get_setting_bond (master_con); - const char *active; - - if (s_bond) { - active = nm_setting_bond_get_option_or_default (s_bond, - NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); - if (nm_streq0 (active, nm_device_get_iface (slave))) { - nm_platform_sysctl_master_set_option (nm_device_get_platform (device), - nm_device_get_ifindex (device), - NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, - active); - _LOGD (LOGD_BOND, "setting slave %s as active one for master %s", - active, nm_device_get_iface (device)); - } - } } + + _LOGI (LOGD_BOND, "enslaved bond slave %s", nm_device_get_ip_iface (slave)); } else - _LOGI (LOGD_BOND, "bond slave %s was enslaved", slave_iface); + _LOGI (LOGD_BOND, "bond slave %s was enslaved", nm_device_get_ip_iface (slave)); return TRUE; } @@ -642,7 +591,6 @@ reapply_connection (NMDevice *device, NMConnection *con_old, NMConnection *con_n /* 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,