From 2e2e2f92df5f5b855adfc5cc9c2481a409f58528 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 10 Sep 2020 09:27:59 +0200 Subject: [PATCH] cli: normalize profile when setting bond options "active_slave" or "primary" "active_slave" is by now deprecated and became an alias for "primary". If a profile specifies both properties, only "primary" is honored, without failing validation (to not break existing behavior). Maybe we should introduce a normalization for such cases. But normalize might not do the right thing, if a profile currently has "primary" set, and the user modifies it to set "active_slave" to a different value, normalize would not know which setting was set first and remove "active_slave" again. In the past, nm_setting_bond_add_option() performed some simple normalization, but this was dropped, because (such incompatible) settings can also be created via the GObject property. Our C accessor function should not be less flexible than other ways of creating a profile. In the end, whenever a user (or a tool) creates a profile, the tool must be aware of the semantics. E.g. setting an IP route without a suitable IP address is unlike to make sense, the tool must understand what it's doing. The same is true for the bond options. When a tool (or user) sets the "active_slave" property, then it must clear out the redundant information from the "primary" setting. There is no alternative to this problem than having tools smart enough to understand what they are doing. --- clients/common/nm-meta-setting-desc.c | 35 ++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c index 44fa9e6c11..5386632e7a 100644 --- a/clients/common/nm-meta-setting-desc.c +++ b/clients/common/nm-meta-setting-desc.c @@ -2376,12 +2376,14 @@ _nm_meta_setting_bond_add_option (NMSetting *setting, const char *value, GError **error) { + NMSettingBond *s_bond = NM_SETTING_BOND (setting); gs_free char *tmp_value = NULL; + const char *val2; char *p; if ( !value || !value[0]) { - if (!nm_setting_bond_remove_option (NM_SETTING_BOND (setting), name)) { + if (!nm_setting_bond_remove_option (s_bond, name)) { nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, _("failed to unset bond option \"%s\""), name); @@ -2401,7 +2403,7 @@ _nm_meta_setting_bond_add_option (NMSetting *setting, *p = ','; } - if (!nm_setting_bond_add_option (NM_SETTING_BOND (setting), name, value)) { + if (!nm_setting_bond_add_option (s_bond, name, value)) { nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, _("failed to set bond option \"%s\""), name); @@ -2410,10 +2412,35 @@ _nm_meta_setting_bond_add_option (NMSetting *setting, if (nm_streq (name, NM_SETTING_BOND_OPTION_ARP_INTERVAL)) { if (_nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXINT, 0) > 0) - _nm_setting_bond_remove_options_miimon (NM_SETTING_BOND (setting)); + _nm_setting_bond_remove_options_miimon (s_bond); } else if (nm_streq (name, NM_SETTING_BOND_OPTION_MIIMON)) { if (_nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXINT, 0) > 0) - _nm_setting_bond_remove_options_arp_interval (NM_SETTING_BOND (setting)); + _nm_setting_bond_remove_options_arp_interval (s_bond); + } else if (nm_streq (name, NM_SETTING_BOND_OPTION_PRIMARY)) { + if ( (val2 = nm_setting_bond_get_option_by_name (s_bond, + NM_SETTING_BOND_OPTION_ACTIVE_SLAVE)) + && !nm_streq (val2, value)) { + /* "active_slave" option is deprecated and an alias for "primary". When + * setting "primary" to a different value, remove the deprecated "active_slave" + * setting. + * + * If we wouldn't do this, then the profile would work as requested, but ignoring + * the (redundant, differing) "active_slave" option. That is confusing, thus clean + * it up. */ + nm_setting_bond_remove_option (s_bond, + NM_SETTING_BOND_OPTION_ACTIVE_SLAVE); + } + } else if (nm_streq (name, NM_SETTING_BOND_OPTION_ACTIVE_SLAVE)) { + if ( (val2 = nm_setting_bond_get_option_by_name (s_bond, + NM_SETTING_BOND_OPTION_PRIMARY)) + && !nm_streq (val2, value)) { + /* "active_slave" is a deprecated alias for "primary". NetworkManager will ignore + * "active_slave" if "primary" is set, we thus need to coerce the primary option + * too. */ + nm_setting_bond_add_option (s_bond, + NM_SETTING_BOND_OPTION_PRIMARY, + value); + } } return TRUE;