From dd1c453ff782dbc5d9b0c92c9e81ffdb7b3124b0 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 4 Jul 2016 16:25:39 +0200 Subject: [PATCH] bond: improve compatibility check of options and modes We print an error when the write of a bond options fails as this is considered an effect of a wrong configuration (or a bug in the checks done by NM) that the user should notice. But not all options are supported in all bonding modes and so we ignore some unsupported options for the current mode to avoid populating logs with useless errors. Improve the code there by using a more generic approach and synchronize the mode/option compatibility table with kernel (file drivers/net/bonding/bond_options.c). https://bugzilla.gnome.org/show_bug.cgi?id=767776 https://bugzilla.redhat.com/show_bug.cgi?id=1352131 --- libnm-core/nm-core-internal.h | 16 ++++ libnm-core/nm-setting-bond.c | 54 ++++++++++++++ src/devices/nm-device-bond.c | 133 ++++++++++++++++------------------ 3 files changed, 133 insertions(+), 70 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 956c910a87..d29eb23cb5 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -307,6 +307,22 @@ _nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name); /***********************************************************/ +typedef enum { + NM_BOND_MODE_UNKNOWN = 0, + NM_BOND_MODE_ROUNDROBIN, + NM_BOND_MODE_ACTIVEBACKUP, + NM_BOND_MODE_XOR, + NM_BOND_MODE_BROADCAST, + NM_BOND_MODE_8023AD, + NM_BOND_MODE_TLB, + NM_BOND_MODE_ALB, +} NMBondMode; + +NMBondMode _nm_setting_bond_mode_from_string (const char *str); +gboolean _nm_setting_bond_option_supported (const char *option, NMBondMode mode); + +/***********************************************************/ + gboolean _nm_utils_inet6_is_token (const struct in6_addr *in6addr); #endif diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index e008878db8..c6482da657 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -463,6 +463,60 @@ _nm_setting_bond_get_option_type (NMSettingBond *setting, const char *name) g_assert_not_reached (); } +NMBondMode +_nm_setting_bond_mode_from_string (const char *str) +{ + g_return_val_if_fail (str, NM_BOND_MODE_UNKNOWN); + + if (nm_streq (str, "balance-rr")) + return NM_BOND_MODE_ROUNDROBIN; + if (nm_streq (str, "active-backup")) + return NM_BOND_MODE_ACTIVEBACKUP; + if (nm_streq (str, "balance-xor")) + return NM_BOND_MODE_XOR; + if (nm_streq (str, "broadcast")) + return NM_BOND_MODE_BROADCAST; + if (nm_streq (str, "802.3ad")) + return NM_BOND_MODE_8023AD; + if (nm_streq (str, "balance-tlb")) + return NM_BOND_MODE_TLB; + if (nm_streq (str, "balance-alb")) + return NM_BOND_MODE_ALB; + + return NM_BOND_MODE_UNKNOWN; +} + +#define BIT(x) (1 << (x)) + +const static struct { + const char *option; + NMBondMode unsupp_modes; +} bond_unsupp_modes[] = { + { NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE, ~(BIT (NM_BOND_MODE_ROUNDROBIN)) }, + { NM_SETTING_BOND_OPTION_ARP_VALIDATE, BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB) }, + { NM_SETTING_BOND_OPTION_ARP_INTERVAL, BIT (NM_BOND_MODE_8023AD) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB) }, + { NM_SETTING_BOND_OPTION_LACP_RATE, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_PRIMARY, ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_ACTIVE_SLAVE, ~(BIT (NM_BOND_MODE_ACTIVEBACKUP) | BIT (NM_BOND_MODE_TLB) | BIT (NM_BOND_MODE_ALB)) }, + { NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB, ~(BIT (NM_BOND_MODE_TLB)) }, + { NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM, ~(BIT (NM_BOND_MODE_8023AD)) }, + { NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, ~(BIT (NM_BOND_MODE_8023AD)) }, +}; + +gboolean +_nm_setting_bond_option_supported (const char *option, NMBondMode mode) +{ + guint i; + + for (i = 0; i < G_N_ELEMENTS (bond_unsupp_modes); i++) { + if (nm_streq (option, bond_unsupp_modes[i].option)) + return !NM_FLAGS_HAS (bond_unsupp_modes[i].unsupp_modes, BIT (mode)); + } + + return TRUE; +} + static gboolean verify (NMSetting *setting, NMConnection *connection, GError **error) { diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 9c98c8ea84..0445ca2067 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -118,12 +118,15 @@ complete_connection (NMDevice *device, /******************************************************************/ static gboolean -set_bond_attr (NMDevice *device, const char *attr, const char *value) +set_bond_attr (NMDevice *device, NMBondMode mode, const char *attr, const char *value) { NMDeviceBond *self = NM_DEVICE_BOND (device); gboolean ret; int ifindex = nm_device_get_ifindex (device); + if (!_nm_setting_bond_option_supported (attr, mode)) + return FALSE; + ret = nm_platform_sysctl_master_set_option (NM_PLATFORM_GET, ifindex, attr, value); if (!ret) _LOGW (LOGD_HW, "failed to set bonding attribute '%s' to '%s'", attr, value); @@ -201,6 +204,7 @@ master_update_slave_connection (NMDevice *self, static void set_arp_targets (NMDevice *device, + NMBondMode mode, const char *value, const char *delim, const char *prefix) @@ -214,7 +218,7 @@ set_arp_targets (NMDevice *device, for (iter = items; iter && *iter; iter++) { if (*iter[0]) { tmp = g_strdup_printf ("%s%s", prefix, *iter); - set_bond_attr (device, "arp_ip_target", tmp); + set_bond_attr (device, mode, "arp_ip_target", tmp); g_free (tmp); } } @@ -223,6 +227,7 @@ set_arp_targets (NMDevice *device, static void set_simple_option (NMDevice *device, + NMBondMode mode, const char *attr, NMSettingBond *s_bond, const char *opt) @@ -232,18 +237,20 @@ set_simple_option (NMDevice *device, value = nm_setting_bond_get_option_by_name (s_bond, opt); if (!value) value = nm_setting_bond_get_option_default (s_bond, opt); - set_bond_attr (device, attr, value); + set_bond_attr (device, mode, attr, value); } static NMActStageReturn apply_bonding_config (NMDevice *device) { + NMDeviceBond *self = NM_DEVICE_BOND (device); NMConnection *connection; NMSettingBond *s_bond; int ifindex = nm_device_get_ifindex (device); - const char *mode, *value; + const char *mode_str, *value; char *contents; gboolean set_arp_interval = TRUE; + NMBondMode mode; /* Option restrictions: * @@ -263,98 +270,84 @@ apply_bonding_config (NMDevice *device) s_bond = nm_connection_get_setting_bond (connection); g_assert (s_bond); - mode = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE); - if (mode == NULL) - mode = "balance-rr"; + mode_str = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MODE); + if (!mode_str) + mode_str = "balance-rr"; + + mode = _nm_setting_bond_mode_from_string (mode_str); + if (mode == NM_BOND_MODE_UNKNOWN) { + _LOGW (LOGD_BOND, "unknown bond mode '%s'", mode_str); + return NM_ACT_STAGE_RETURN_FAILURE; + } + + /* Set mode first, as some other options (e.g. arp_interval) are valid + * only for certain modes. + */ + set_bond_attr (device, mode, "mode", mode_str); value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_MIIMON); if (value && atoi (value)) { /* clear arp interval */ - set_bond_attr (device, "arp_interval", "0"); + set_bond_attr (device, mode, "arp_interval", "0"); set_arp_interval = FALSE; - set_bond_attr (device, "miimon", value); - set_simple_option (device, "updelay", s_bond, NM_SETTING_BOND_OPTION_UPDELAY); - set_simple_option (device, "downdelay", s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY); + set_bond_attr (device, mode, "miimon", value); + set_simple_option (device, mode, "updelay", s_bond, NM_SETTING_BOND_OPTION_UPDELAY); + set_simple_option (device, mode, "downdelay", s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY); } else if (!value) { - /* If not given, and arp_interval is not given, default to 100 */ - long int val_int; - char *end; - + /* If not given, and arp_interval is not given or disabled, default to 100 */ value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - errno = 0; - val_int = strtol (value ? value : "0", &end, 10); - if (!value || (val_int == 0 && errno == 0 && *end == '\0')) - set_bond_attr (device, "miimon", "100"); + if (_nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT32, 0) == 0) + set_bond_attr (device, mode, "miimon", "100"); } - /* The stuff after 'mode' requires the given mode or doesn't care */ - set_bond_attr (device, "mode", mode); - - /* arp_interval not compatible with ALB, TLB */ - if (NM_IN_STRSET (mode, "balance-alb", "balance-tlb")) - set_arp_interval = FALSE; - if (set_arp_interval) { - set_simple_option (device, "arp_interval", s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); - + set_simple_option (device, mode, "arp_interval", s_bond, NM_SETTING_BOND_OPTION_ARP_INTERVAL); /* Just let miimon get cleared automatically; even setting miimon to * 0 (disabled) clears arp_interval. */ } + /* ARP validate: value > 0 only valid in active-backup mode */ value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_VALIDATE); - /* arp_validate > 0 only valid in active-backup mode */ if ( value && !nm_streq (value, "0") && !nm_streq (value, "none") - && nm_streq (mode, "active-backup")) - set_bond_attr (device, "arp_validate", value); + && mode == NM_BOND_MODE_ACTIVEBACKUP) + set_bond_attr (device, mode, "arp_validate", value); else - set_bond_attr (device, "arp_validate", "0"); + set_bond_attr (device, mode, "arp_validate", "0"); - if (NM_IN_STRSET (mode, "active-backup", "balance-alb", "balance-tlb")) { - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_PRIMARY); - set_bond_attr (device, "primary", value ? value : ""); - set_simple_option (device, "lp_interval", s_bond, NM_SETTING_BOND_OPTION_LP_INTERVAL); - } + /* Primary */ + value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_PRIMARY); + set_bond_attr (device, mode, "primary", value ? value : ""); - /* Clear ARP targets */ + /* ARP targets: clear and initialize the list */ contents = nm_platform_sysctl_master_get_option (NM_PLATFORM_GET, ifindex, "arp_ip_target"); - set_arp_targets (device, contents, " \n", "-"); + set_arp_targets (device, mode, contents, " \n", "-"); + value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); + set_arp_targets (device, mode, value, ",", "+"); g_free (contents); - /* Add new ARP targets */ - value = nm_setting_bond_get_option_by_name (s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET); - set_arp_targets (device, value, ",", "+"); - - set_simple_option (device, "primary_reselect", s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT); - set_simple_option (device, "fail_over_mac", s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC); - set_simple_option (device, "use_carrier", s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER); - set_simple_option (device, "ad_select", s_bond, NM_SETTING_BOND_OPTION_AD_SELECT); - set_simple_option (device, "xmit_hash_policy", s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY); - set_simple_option (device, "resend_igmp", s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP); - set_simple_option (device, "active_slave", s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); - set_simple_option (device, "all_slaves_active", s_bond, NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE); - set_simple_option (device, "num_grat_arp", s_bond, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); - set_simple_option (device, "num_unsol_na", s_bond, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); - - if (nm_streq (mode, "802.3ad")) { - set_simple_option (device, "lacp_rate", s_bond, NM_SETTING_BOND_OPTION_LACP_RATE); - set_simple_option (device, "ad_actor_sys_prio", s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO); - set_simple_option (device, "ad_actor_system", s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM); - set_simple_option (device, "ad_user_port_key", s_bond, NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY); - set_simple_option (device, "min_links", s_bond, NM_SETTING_BOND_OPTION_MIN_LINKS); - } - - if (nm_streq (mode, "active-backup")) - set_simple_option (device, "arp_all_targets", s_bond, NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS); - - if (nm_streq (mode, "balance-rr")) - set_simple_option (device, "packets_per_slave", s_bond, NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE); - - if (nm_streq (mode, "balance-tlb")) - set_simple_option (device, "tlb_dynamic_lb", s_bond, NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB); + set_simple_option (device, mode, "primary_reselect", s_bond, NM_SETTING_BOND_OPTION_PRIMARY_RESELECT); + set_simple_option (device, mode, "fail_over_mac", s_bond, NM_SETTING_BOND_OPTION_FAIL_OVER_MAC); + set_simple_option (device, mode, "use_carrier", s_bond, NM_SETTING_BOND_OPTION_USE_CARRIER); + set_simple_option (device, mode, "ad_select", s_bond, NM_SETTING_BOND_OPTION_AD_SELECT); + set_simple_option (device, mode, "xmit_hash_policy", s_bond, NM_SETTING_BOND_OPTION_XMIT_HASH_POLICY); + set_simple_option (device, mode, "resend_igmp", s_bond, NM_SETTING_BOND_OPTION_RESEND_IGMP); + set_simple_option (device, mode, "active_slave", s_bond, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); + set_simple_option (device, mode, "all_slaves_active", s_bond, NM_SETTING_BOND_OPTION_ALL_SLAVES_ACTIVE); + set_simple_option (device, mode, "num_grat_arp", s_bond, NM_SETTING_BOND_OPTION_NUM_GRAT_ARP); + set_simple_option (device, mode, "num_unsol_na", s_bond, NM_SETTING_BOND_OPTION_NUM_UNSOL_NA); + set_simple_option (device, mode, "lacp_rate", s_bond, NM_SETTING_BOND_OPTION_LACP_RATE); + set_simple_option (device, mode, "ad_actor_sys_prio", s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYS_PRIO); + set_simple_option (device, mode, "ad_actor_system", s_bond, NM_SETTING_BOND_OPTION_AD_ACTOR_SYSTEM); + set_simple_option (device, mode, "ad_user_port_key", s_bond, NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY); + set_simple_option (device, mode, "min_links", s_bond, NM_SETTING_BOND_OPTION_MIN_LINKS); + set_simple_option (device, mode, "arp_all_targets", s_bond, NM_SETTING_BOND_OPTION_ARP_ALL_TARGETS); + set_simple_option (device, mode, "packets_per_slave", s_bond, NM_SETTING_BOND_OPTION_PACKETS_PER_SLAVE); + set_simple_option (device, mode, "tlb_dynamic_lb", s_bond, NM_SETTING_BOND_OPTION_TLB_DYNAMIC_LB); + set_simple_option (device, mode, "lp_interval", s_bond, NM_SETTING_BOND_OPTION_LP_INTERVAL); return NM_ACT_STAGE_RETURN_SUCCESS; }