libnm: merge branch 'th/bond-add-option-relax'

https://bugzilla.redhat.com/show_bug.cgi?id=1887523

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/653

11 files changed, 151 insertions, 89 deletions

diff --git a/NEWS b/NEWS
index f2bbfc05fd..83990f01ff 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,10 @@ The API is subject to change and not guaranteed to be compatible
 with the later release.
 USE AT YOUR OWN RISK.  NOT RECOMMENDED FOR PRODUCTION USE!
 
+* libnm: nm_setting_bond_add_option() no longer validates the
+  option that is set. Instead, use nm_connection_verify() to validate
+  the profile.
+
 =============================================
 NetworkManager-1.28
 Overview of changes since NetworkManager-1.26
diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c
index cdb3084a07..115f65591a 100644
--- a/clients/common/nm-meta-setting-desc.c
+++ b/clients/common/nm-meta-setting-desc.c
@@ -2400,13 +2400,9 @@ _nm_meta_setting_bond_add_option(NMSetting * setting,
     char *         p;
 
     if (!value || !value[0]) {
-        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);
-            return FALSE;
-        }
+        /* This call only fails if name is currently not tracked. It's not an
+         * error to remove an option that is not set. */
+        nm_setting_bond_remove_option(s_bond, name);
         return TRUE;
     }
 
@@ -2421,7 +2417,7 @@ _nm_meta_setting_bond_add_option(NMSetting * setting,
                 *p = ',';
     }
 
-    if (!nm_setting_bond_add_option(s_bond, name, value)) {
+    if (!nm_setting_bond_validate_option(name, value)) {
         nm_utils_error_set(error,
                            NM_UTILS_ERROR_INVALID_ARGUMENT,
                            _("failed to set bond option \"%s\""),
@@ -2429,6 +2425,8 @@ _nm_meta_setting_bond_add_option(NMSetting * setting,
         return FALSE;
     }
 
+    nm_setting_bond_add_option(s_bond, name, value);
+
     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(s_bond);
diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c
index 10bfe7b839..84a0b34835 100644
--- a/libnm-core/nm-connection.c
+++ b/libnm-core/nm-connection.c
@@ -1065,7 +1065,8 @@ _normalize_bond_mode(NMConnection *self)
 
         if (mode_int != -1) {
             const char *mode_new = nm_utils_bond_mode_int_to_string(mode_int);
-            if (g_strcmp0(mode_new, mode) != 0) {
+
+            if (!nm_streq0(mode_new, mode)) {
                 nm_setting_bond_add_option(s_bond, NM_SETTING_BOND_OPTION_MODE, mode_new);
                 return TRUE;
             }
diff --git a/libnm-core/nm-keyfile/nm-keyfile.c b/libnm-core/nm-keyfile/nm-keyfile.c
index 395af1db09..a8aefe6349 100644
--- a/libnm-core/nm-keyfile/nm-keyfile.c
+++ b/libnm-core/nm-keyfile/nm-keyfile.c
@@ -1141,7 +1141,10 @@ mac_address_parser_INFINIBAND(KeyfileReaderInfo *info, NMSetting *setting, const
 }
 
 static void
-read_hash_of_string(GKeyFile *file, NMSetting *setting, const char *key)
+read_hash_of_string(KeyfileReaderInfo *info,
+                    GKeyFile *         file,
+                    NMSetting *        setting,
+                    const char *       kf_group)
 {
     gs_strfreev char **keys = NULL;
     const char *const *iter;
@@ -1149,10 +1152,10 @@ read_hash_of_string(GKeyFile *file, NMSetting *setting, const char *key)
     gboolean           is_vpn;
     gsize              n_keys;
 
-    nm_assert((NM_IS_SETTING_VPN(setting) && nm_streq(key, NM_SETTING_VPN_DATA))
-              || (NM_IS_SETTING_VPN(setting) && nm_streq(key, NM_SETTING_VPN_SECRETS))
-              || (NM_IS_SETTING_BOND(setting) && nm_streq(key, NM_SETTING_BOND_OPTIONS))
-              || (NM_IS_SETTING_USER(setting) && nm_streq(key, NM_SETTING_USER_DATA)));
+    nm_assert((NM_IS_SETTING_VPN(setting) && nm_streq(kf_group, NM_SETTING_VPN_DATA))
+              || (NM_IS_SETTING_VPN(setting) && nm_streq(kf_group, NM_SETTING_VPN_SECRETS))
+              || (NM_IS_SETTING_BOND(setting) && nm_streq(kf_group, NM_SETTING_BOND_OPTIONS))
+              || (NM_IS_SETTING_USER(setting) && nm_streq(kf_group, NM_SETTING_USER_DATA)));
 
     keys = nm_keyfile_plugin_kf_get_keys(file, setting_name, &n_keys, NULL);
     if (n_keys == 0)
@@ -1160,23 +1163,38 @@ read_hash_of_string(GKeyFile *file, NMSetting *setting, const char *key)
 
     if ((is_vpn = NM_IS_SETTING_VPN(setting)) || NM_IS_SETTING_BOND(setting)) {
         for (iter = (const char *const *) keys; *iter; iter++) {
+            const char *  kf_key  = *iter;
             gs_free char *to_free = NULL;
             gs_free char *value   = NULL;
             const char *  name;
 
-            value = nm_keyfile_plugin_kf_get_string(file, setting_name, *iter, NULL);
+            value = nm_keyfile_plugin_kf_get_string(file, setting_name, kf_key, NULL);
             if (!value)
                 continue;
 
-            name = nm_keyfile_key_decode(*iter, &to_free);
+            name = nm_keyfile_key_decode(kf_key, &to_free);
 
             if (is_vpn) {
                 /* Add any item that's not a class property to the data hash */
                 if (!g_object_class_find_property(G_OBJECT_GET_CLASS(setting), name))
                     nm_setting_vpn_add_data_item(NM_SETTING_VPN(setting), name, value);
             } else {
-                if (!nm_streq(name, "interface-name"))
-                    nm_setting_bond_add_option(NM_SETTING_BOND(setting), name, value);
+                if (!nm_streq(name, "interface-name")) {
+                    gs_free_error GError *error = NULL;
+
+                    if (!_nm_setting_bond_validate_option(name, value, &error)) {
+                        if (!handle_warn(info,
+                                         kf_key,
+                                         name,
+                                         NM_KEYFILE_WARN_SEVERITY_WARN,
+                                         _("ignoring invalid bond option %s%s%s = %s%s%s: %s"),
+                                         NM_PRINT_FMT_QUOTE_STRING(name),
+                                         NM_PRINT_FMT_QUOTE_STRING(value),
+                                         error->message))
+                            return;
+                    } else
+                        nm_setting_bond_add_option(NM_SETTING_BOND(setting), name, value);
+                }
             }
         }
         openconnect_fix_secret_flags(setting);
@@ -3170,7 +3188,7 @@ read_one_setting_value(KeyfileReaderInfo *       info,
                                                   NULL);
         g_object_set(setting, key, sa, NULL);
     } else if (type == G_TYPE_HASH_TABLE) {
-        read_hash_of_string(keyfile, setting, key);
+        read_hash_of_string(info, keyfile, setting, key);
     } else if (type == G_TYPE_ARRAY) {
         read_array_of_uint(keyfile, setting, key);
     } else if (G_TYPE_IS_FLAGS(type)) {
diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c
index 65cd8ba5d7..1d24d8fee8 100644
--- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c
+++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c
@@ -36,18 +36,19 @@ _nm_setting_bond_remove_options_arp_interval(NMSettingBond *s_bond)
     nm_setting_bond_remove_option(s_bond, NM_SETTING_BOND_OPTION_ARP_IP_TARGET);
 }
 
+/*****************************************************************************/
+
 NM_UTILS_STRING_TABLE_LOOKUP_DEFINE(
     _nm_setting_bond_mode_from_string,
     NMBondMode,
-    { g_return_val_if_fail(name, NM_BOND_MODE_UNKNOWN); },
+    {
+        G_STATIC_ASSERT_EXPR(_NM_BOND_MODE_NUM <= 9);
+
+        if (name && name[0] < '0' + _NM_BOND_MODE_NUM && name[0] >= '0' && name[1] == '\0') {
+            return name[0] - '0';
+        }
+    },
     { return NM_BOND_MODE_UNKNOWN; },
-    {"0", NM_BOND_MODE_ROUNDROBIN},
-    {"1", NM_BOND_MODE_ACTIVEBACKUP},
-    {"2", NM_BOND_MODE_XOR},
-    {"3", NM_BOND_MODE_BROADCAST},
-    {"4", NM_BOND_MODE_8023AD},
-    {"5", NM_BOND_MODE_TLB},
-    {"6", NM_BOND_MODE_ALB},
     {"802.3ad", NM_BOND_MODE_8023AD},
     {"active-backup", NM_BOND_MODE_ACTIVEBACKUP},
     {"balance-alb", NM_BOND_MODE_ALB},
@@ -56,6 +57,47 @@ NM_UTILS_STRING_TABLE_LOOKUP_DEFINE(
     {"balance-xor", NM_BOND_MODE_XOR},
     {"broadcast", NM_BOND_MODE_BROADCAST}, );
 
+const char *
+_nm_setting_bond_mode_to_string(int mode)
+{
+    static const char *const modes[] = {
+        [NM_BOND_MODE_8023AD]       = "802.3ad",
+        [NM_BOND_MODE_ACTIVEBACKUP] = "active-backup",
+        [NM_BOND_MODE_ALB]          = "balance-alb",
+        [NM_BOND_MODE_BROADCAST]    = "broadcast",
+        [NM_BOND_MODE_ROUNDROBIN]   = "balance-rr",
+        [NM_BOND_MODE_TLB]          = "balance-tlb",
+        [NM_BOND_MODE_XOR]          = "balance-xor",
+    };
+
+    G_STATIC_ASSERT(G_N_ELEMENTS(modes) == _NM_BOND_MODE_NUM);
+
+    if (NM_MORE_ASSERT_ONCE(5)) {
+        char       sbuf[100];
+        int        i;
+        NMBondMode m;
+
+        for (i = 0; i < (int) G_N_ELEMENTS(modes); i++) {
+            nm_assert(modes[i]);
+            nm_assert(i == _nm_setting_bond_mode_from_string(modes[i]));
+            nm_assert(i == _nm_setting_bond_mode_from_string(nm_sprintf_buf(sbuf, "%d", i)));
+        }
+        nm_assert(NM_BOND_MODE_UNKNOWN == _nm_setting_bond_mode_from_string(NULL));
+        nm_assert(NM_BOND_MODE_UNKNOWN == _nm_setting_bond_mode_from_string(""));
+        for (i = -2; i < ((int) G_N_ELEMENTS(modes)) + 20; i++) {
+            if (i < 0 || i >= G_N_ELEMENTS(modes))
+                m = NM_BOND_MODE_UNKNOWN;
+            else
+                m = i;
+            nm_assert(m == _nm_setting_bond_mode_from_string(nm_sprintf_buf(sbuf, "%d", i)));
+        }
+    }
+
+    if (mode >= 0 && mode < (int) G_N_ELEMENTS(modes))
+        return modes[mode];
+    return NULL;
+}
+
 /*****************************************************************************/
 
 gboolean
diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
index 5d672fedb3..ca29eade24 100644
--- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
+++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
@@ -65,11 +65,16 @@ typedef enum {
     NM_BOND_MODE_8023AD       = 4,
     NM_BOND_MODE_TLB          = 5,
     NM_BOND_MODE_ALB          = 6,
+
     _NM_BOND_MODE_NUM,
 } NMBondMode;
 
 NMBondMode _nm_setting_bond_mode_from_string(const char *str);
 
+const char *_nm_setting_bond_mode_to_string(int mode);
+
+gboolean _nm_setting_bond_validate_option(const char *name, const char *value, GError **error);
+
 /*****************************************************************************/
 
 static inline guint32
diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c
index f355734bfa..2155815e06 100644
--- a/libnm-core/nm-setting-bond.c
+++ b/libnm-core/nm-setting-bond.c
@@ -515,8 +515,8 @@ validate_ifname(const char *name, const char *value)
     return nm_utils_ifname_valid_kernel(value, NULL);
 }
 
-static gboolean
-_setting_bond_validate_option(const char *name, const char *value, GError **error)
+gboolean
+_nm_setting_bond_validate_option(const char *name, const char *value, GError **error)
 {
     const OptionMeta *option_meta;
     gboolean          success;
@@ -589,7 +589,7 @@ handle_error:
 gboolean
 nm_setting_bond_validate_option(const char *name, const char *value)
 {
-    return _setting_bond_validate_option(name, value, NULL);
+    return _nm_setting_bond_validate_option(name, value, NULL);
 }
 
 /**
@@ -608,9 +608,6 @@ nm_setting_bond_get_option_by_name(NMSettingBond *setting, const char *name)
 {
     g_return_val_if_fail(NM_IS_SETTING_BOND(setting), NULL);
 
-    if (!nm_setting_bond_validate_option(name, NULL))
-        return NULL;
-
     return _bond_get_option(setting, name);
 }
 
@@ -620,16 +617,18 @@ nm_setting_bond_get_option_by_name(NMSettingBond *setting, const char *name)
  * @name: name for the option
  * @value: value for the option
  *
- * Add an option to the table.  The option is compared to an internal list
- * of allowed options.  Option names may contain only alphanumeric characters
- * (ie [a-zA-Z0-9]).  Adding a new name replaces any existing name/value pair
+ * Add an option to the table. Adding a new name replaces any existing name/value pair
  * that may already exist.
  *
- * The order of how to set several options is relevant because there are options
- * that conflict with each other.
+ * Returns: returns %FALSE if either @name or @value is %NULL, in that case
+ * the option is not set. Otherwise, the function does not fail and does not validate
+ * the arguments. All validation happens via nm_connection_verify() or do basic validation
+ * yourself with nm_setting_bond_validate_option().
  *
- * Returns: %TRUE if the option was valid and was added to the internal option
- * list, %FALSE if it was not.
+ * Note: Before 1.30, libnm would perform basic validation of the name and the value
+ * via nm_setting_bond_validate_option() and reject the request by returning FALSE.
+ * Since 1.30, libnm no longer rejects any values as the setter is not supposed
+ * to perform validation.
  **/
 gboolean
 nm_setting_bond_add_option(NMSettingBond *setting, const char *name, const char *value)
@@ -638,16 +637,16 @@ nm_setting_bond_add_option(NMSettingBond *setting, const char *name, const char
 
     g_return_val_if_fail(NM_IS_SETTING_BOND(setting), FALSE);
 
-    if (!value || !nm_setting_bond_validate_option(name, value))
+    if (!name)
+        return FALSE;
+    if (!value)
         return FALSE;
 
     priv = NM_SETTING_BOND_GET_PRIVATE(setting);
 
     nm_clear_g_free(&priv->options_idx_cache);
     g_hash_table_insert(priv->options, g_strdup(name), g_strdup(value));
-
     _notify(setting, PROP_OPTIONS);
-
     return TRUE;
 }
 
@@ -666,20 +665,17 @@ gboolean
 nm_setting_bond_remove_option(NMSettingBond *setting, const char *name)
 {
     NMSettingBondPrivate *priv;
-    gboolean              found;
 
     g_return_val_if_fail(NM_IS_SETTING_BOND(setting), FALSE);
 
-    if (!nm_setting_bond_validate_option(name, NULL))
-        return FALSE;
-
     priv = NM_SETTING_BOND_GET_PRIVATE(setting);
 
+    if (!g_hash_table_remove(priv->options, name))
+        return FALSE;
+
     nm_clear_g_free(&priv->options_idx_cache);
-    found = g_hash_table_remove(priv->options, name);
-    if (found)
-        _notify(setting, PROP_OPTIONS);
-    return found;
+    _notify(setting, PROP_OPTIONS);
+    return TRUE;
 }
 
 /**
@@ -783,7 +779,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error)
         for (i = 0; priv->options_idx_cache[i].name; i++) {
             n = &priv->options_idx_cache[i];
 
-            if (!n->value_str || !_setting_bond_validate_option(n->name, n->value_str, error)) {
+            if (!n->value_str || !_nm_setting_bond_validate_option(n->name, n->value_str, error)) {
                 g_prefix_error(error,
                                "%s.%s: ",
                                NM_SETTING_BOND_SETTING_NAME,
diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 3aa4b4f90a..cc39be25bd 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -4942,20 +4942,7 @@ nm_utils_check_virtual_device_compatibility(GType virtual_type, GType other_type
     }
 }
 
-typedef struct {
-    const char *str;
-    const char *num;
-} BondMode;
-
-static const BondMode bond_mode_table[] = {
-    [0] = {"balance-rr", "0"},
-    [1] = {"active-backup", "1"},
-    [2] = {"balance-xor", "2"},
-    [3] = {"broadcast", "3"},
-    [4] = {"802.3ad", "4"},
-    [5] = {"balance-tlb", "5"},
-    [6] = {"balance-alb", "6"},
-};
+/*****************************************************************************/
 
 /**
  * nm_utils_bond_mode_int_to_string:
@@ -4972,9 +4959,7 @@ static const BondMode bond_mode_table[] = {
 const char *
 nm_utils_bond_mode_int_to_string(int mode)
 {
-    if (mode >= 0 && mode < G_N_ELEMENTS(bond_mode_table))
-        return bond_mode_table[mode].str;
-    return NULL;
+    return _nm_setting_bond_mode_to_string(mode);
 }
 
 /**
@@ -4993,16 +4978,7 @@ nm_utils_bond_mode_int_to_string(int mode)
 int
 nm_utils_bond_mode_string_to_int(const char *mode)
 {
-    int i;
-
-    if (!mode || !*mode)
-        return -1;
-
-    for (i = 0; i < G_N_ELEMENTS(bond_mode_table); i++) {
-        if (NM_IN_STRSET(mode, bond_mode_table[i].str, bond_mode_table[i].num))
-            return i;
-    }
-    return -1;
+    return _nm_setting_bond_mode_from_string(mode);
 }
 
 /*****************************************************************************/
diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c
index a54c9b47c4..43f2a92ef4 100644
--- a/src/devices/nm-device-bond.c
+++ b/src/devices/nm-device-bond.c
@@ -149,6 +149,7 @@ ignore_option(NMSettingBond *s_bond, const char *option, const char *value)
 static void
 update_connection(NMDevice *device, NMConnection *connection)
 {
+    NMDeviceBond * self    = NM_DEVICE_BOND(device);
     NMSettingBond *s_bond  = nm_connection_get_setting_bond(connection);
     int            ifindex = nm_device_get_ifindex(device);
     NMBondMode     mode    = NM_BOND_MODE_UNKNOWN;
@@ -197,7 +198,10 @@ update_connection(NMDevice *device, NMConnection *connection)
                 }
             }
 
-            nm_setting_bond_add_option(s_bond, option, value);
+            if (!_nm_setting_bond_validate_option(option, value, NULL))
+                _LOGT(LOGD_BOND, "cannot set invalid bond option '%s' = '%s'", option, value);
+            else
+                nm_setting_bond_add_option(s_bond, option, value);
         }
     }
 }
diff --git a/src/initrd/nmi-cmdline-reader.c b/src/initrd/nmi-cmdline-reader.c
index b9432d4a62..ead9053683 100644
--- a/src/initrd/nmi-cmdline-reader.c
+++ b/src/initrd/nmi-cmdline-reader.c
@@ -682,8 +682,6 @@ reader_parse_master(Reader *reader, char *argument, const char *type_name, const
     char *               slaves;
     const char *         slave;
     char *               opts;
-    char *               opt;
-    const char *         opt_name;
     const char *         mtu = NULL;
 
     master = get_word(&argument, ':');
@@ -705,8 +703,21 @@ reader_parse_master(Reader *reader, char *argument, const char *type_name, const
 
         opts = get_word(&argument, ':');
         while (opts && *opts) {
+            gs_free_error GError *error = NULL;
+            char *                opt;
+            const char *          opt_name;
+
             opt      = get_word(&opts, ',');
             opt_name = get_word(&opt, '=');
+
+            if (!_nm_setting_bond_validate_option(opt_name, opt, &error)) {
+                _LOGW(LOGD_CORE,
+                      "Ignoring invalid bond option: %s%s%s = %s%s%s: %s",
+                      NM_PRINT_FMT_QUOTE_STRING(opt_name),
+                      NM_PRINT_FMT_QUOTE_STRING(opt),
+                      error->message);
+                continue;
+            }
             nm_setting_bond_add_option(s_bond, opt_name, opt);
         }
 
diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
index 5a021a8c37..3389b2a0aa 100644
--- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
+++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
@@ -5339,24 +5339,31 @@ infiniband_connection_from_ifcfg(const char *file, shvarFile *ifcfg, GError **er
 static void
 handle_bond_option(NMSettingBond *s_bond, const char *key, const char *value)
 {
-    char *      sanitized = NULL, *j;
-    const char *p         = value;
+    gs_free char *sanitized = NULL;
+    const char *  p         = value;
 
     /* Remove any quotes or +/- from arp_ip_target */
-    if (!g_strcmp0(key, NM_SETTING_BOND_OPTION_ARP_IP_TARGET) && value && value[0]) {
+    if (nm_streq0(key, NM_SETTING_BOND_OPTION_ARP_IP_TARGET) && value && value[0]) {
+        char *j;
+
         if (*p == '\'' || *p == '"')
             p++;
-        j = sanitized = g_malloc0(strlen(p) + 1);
+        j = sanitized = g_malloc(strlen(p) + 1);
         while (*p) {
             if (*p != '+' && *p != '-' && *p != '\'' && *p != '"')
                 *j++ = *p;
             p++;
         }
+        *j++  = '\0';
+        value = sanitized;
+    }
+
+    if (!_nm_setting_bond_validate_option(key, value, NULL)) {
+        PARSE_WARNING("invalid bonding option '%s' = %s", key, value);
+        return;
     }
 
-    if (!nm_setting_bond_add_option(s_bond, key, sanitized ?: value))
-        PARSE_WARNING("invalid bonding option '%s' = %s", key, sanitized ?: value);
-    g_free(sanitized);
+    nm_setting_bond_add_option(s_bond, key, value);
 }
 
 static NMSetting *