From c1e40a4f39da03ff2a398830bbf6d2c3d59b24e3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Aug 2019 08:53:05 +0200 Subject: [PATCH 1/7] shared: use nm_auto_unref_gtypeclass in _nm_utils_enum_from_str_full() --- shared/nm-glib-aux/nm-enum-utils.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/shared/nm-glib-aux/nm-enum-utils.c b/shared/nm-glib-aux/nm-enum-utils.c index b16267a5e1..19ce9a41e6 100644 --- a/shared/nm-glib-aux/nm-enum-utils.c +++ b/shared/nm-glib-aux/nm-enum-utils.c @@ -225,7 +225,7 @@ _nm_utils_enum_from_str_full (GType type, char **err_token, const NMUtilsEnumValueInfo *value_infos) { - GTypeClass *klass; + nm_auto_unref_gtypeclass GTypeClass *klass = NULL; gboolean ret = FALSE; int value = 0; gs_free char *str_clone = NULL; @@ -317,7 +317,6 @@ _nm_utils_enum_from_str_full (GType type, NM_SET_OUT (err_token, !ret && s[0] ? g_strdup (s) : NULL); NM_SET_OUT (out_value, ret ? value : 0); - g_type_class_unref (klass); return ret; } From 4e51e844d9ac4a2a7f556891c24170a2808eb71e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Aug 2019 08:52:45 +0200 Subject: [PATCH 2/7] libnm: fix NMSetting8021xAuthFlags to be a flags type This is an API break, but probably not too bad. A lot of things when using the type will work as before. --- libnm-core/nm-setting-8021x.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libnm-core/nm-setting-8021x.h b/libnm-core/nm-setting-8021x.h index 5a5ae65042..f811c60ec7 100644 --- a/libnm-core/nm-setting-8021x.h +++ b/libnm-core/nm-setting-8021x.h @@ -84,9 +84,12 @@ typedef enum { /*< underscore_name=nm_setting_802_1x_ck_scheme >*/ * #NMSetting8021xAuthFlags values indicate which authentication settings * should be used. * + * Before 1.22, this was wrongly marked as a enum and not as a flags + * type. + * * Since: 1.8 */ -typedef enum { /*< underscore_name=nm_setting_802_1x_auth_flags >*/ +typedef enum { /*< flags, underscore_name=nm_setting_802_1x_auth_flags >*/ NM_SETTING_802_1X_AUTH_FLAGS_NONE = 0, NM_SETTING_802_1X_AUTH_FLAGS_TLS_1_0_DISABLE = 0x1, NM_SETTING_802_1X_AUTH_FLAGS_TLS_1_1_DISABLE = 0x2, From 036b793797edd48c51ba1e0b700157217aa051cc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Aug 2019 08:55:44 +0200 Subject: [PATCH 3/7] cli: support +/- modifiers for flags properties --- clients/common/nm-meta-setting-desc.c | 38 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c index d3f5efaffb..d5014441cb 100644 --- a/clients/common/nm-meta-setting-desc.c +++ b/clients/common/nm-meta-setting-desc.c @@ -1458,11 +1458,12 @@ _set_fcn_gobject_enum (ARGS_SET_FCN) GType gtype_prop; gboolean has_gtype = FALSE; nm_auto_unset_gvalue GValue gval = G_VALUE_INIT; + nm_auto_unref_gtypeclass GTypeClass *gtype_prop_class = NULL; nm_auto_unref_gtypeclass GTypeClass *gtype_class = NULL; gboolean is_flags; int v; - if (_SET_FCN_DO_RESET_DEFAULT (property_info, modifier, value)) + if (_SET_FCN_DO_RESET_DEFAULT_WITH_SUPPORTS_REMOVE (property_info, modifier, value)) return _gobject_property_reset_default (setting, property_info->property_name); if (property_info->property_typ_data) { @@ -1479,15 +1480,15 @@ _set_fcn_gobject_enum (ARGS_SET_FCN) G_TYPE_INT, G_TYPE_UINT) && G_TYPE_IS_CLASSED (gtype) - && (gtype_class = g_type_class_ref (gtype)) - && ( (is_flags = G_IS_FLAGS_CLASS (gtype_class)) - || G_IS_ENUM_CLASS (gtype_class))) { + && (gtype_prop_class = g_type_class_ref (gtype)) + && ( (is_flags = G_IS_FLAGS_CLASS (gtype_prop_class)) + || G_IS_ENUM_CLASS (gtype_prop_class))) { /* valid */ } else if ( !has_gtype && G_TYPE_IS_CLASSED (gtype_prop) - && (gtype_class = g_type_class_ref (gtype_prop)) - && ( (is_flags = G_IS_FLAGS_CLASS (gtype_class)) - || G_IS_ENUM_CLASS (gtype_class))) { + && (gtype_prop_class = g_type_class_ref (gtype_prop)) + && ( (is_flags = G_IS_FLAGS_CLASS (gtype_prop_class)) + || G_IS_ENUM_CLASS (gtype_prop_class))) { gtype = gtype_prop; } else g_return_val_if_reached (FALSE); @@ -1507,16 +1508,33 @@ _set_fcn_gobject_enum (ARGS_SET_FCN) v); } + gtype_class = g_type_class_ref (gtype); + + if ( G_IS_FLAGS_CLASS (gtype_class) + && !_SET_FCN_DO_SET_ALL (modifier, value)) { + nm_auto_unset_gvalue GValue int_value = { }; + guint v_flag; + + g_value_init (&int_value, G_TYPE_UINT); + g_object_get_property (G_OBJECT (setting), property_info->property_name, &int_value); + v_flag = g_value_get_uint (&int_value); + + if (_SET_FCN_DO_REMOVE (modifier, value)) + v = (int) (v_flag & ~((guint) v)); + else + v = (int) (v_flag | ((guint) v)); + } + g_value_init (&gval, gtype_prop); if (gtype_prop == G_TYPE_INT) g_value_set_int (&gval, v); else if (gtype_prop == G_TYPE_UINT) g_value_set_uint (&gval, v); else if (is_flags) { - nm_assert (G_IS_FLAGS_CLASS (gtype_class)); + nm_assert (G_IS_FLAGS_CLASS (gtype_prop_class)); g_value_set_flags (&gval, v); } else { - nm_assert (G_IS_ENUM_CLASS (gtype_class)); + nm_assert (G_IS_ENUM_CLASS (gtype_prop_class)); g_value_set_enum (&gval, v); } @@ -4339,12 +4357,14 @@ static const NMMetaPropertyType _pt_gobject_secret_flags = { .get_fcn = _get_fcn_gobject_secret_flags, .set_fcn = _set_fcn_gobject_enum, .values_fcn = _values_fcn_gobject_enum, + .set_supports_remove = TRUE, }; static const NMMetaPropertyType _pt_gobject_enum = { .get_fcn = _get_fcn_gobject_enum, .set_fcn = _set_fcn_gobject_enum, .values_fcn = _values_fcn_gobject_enum, + .set_supports_remove = TRUE, }; static const NMMetaPropertyType _pt_gobject_devices = { From de40eb04030b2a53fc0bdaa7885abc260c7fedfe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Aug 2019 09:21:13 +0200 Subject: [PATCH 4/7] cli: reorder checks in nmc_setting_set_property() for modifier type No notable change in behavior, but makes more sense this way. --- clients/cli/settings.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clients/cli/settings.c b/clients/cli/settings.c index 2446cb0856..91aaf28e38 100644 --- a/clients/cli/settings.c +++ b/clients/cli/settings.c @@ -549,12 +549,6 @@ nmc_setting_set_property (NMClient *client, if (!property_info->property_type->set_fcn) goto out_fail_read_only; - if ( NM_IN_SET (modifier, '+', '-') - && !value) { - /* nothing to do. */ - return TRUE; - } - if ( modifier == '-' && !property_info->property_type->set_supports_remove) { /* The property is a plain property. It does not support '-'. @@ -578,6 +572,13 @@ nmc_setting_set_property (NMClient *client, } } + if ( NM_IN_SET (modifier, '+', '-') + && ( !value + || !value[0])) { + /* nothing to do. */ + return TRUE; + } + g_object_freeze_notify (G_OBJECT (setting)); success = property_info->property_type->set_fcn (property_info, nmc_meta_environment, From 0825ec34fde7fc35db54aff110d678ebce2dfb52 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Aug 2019 09:43:15 +0200 Subject: [PATCH 5/7] cli: add NMMetaAccessorModifier enum instead of using "char" type The enum values are unique throughout the source code so they can easier be searched (e.g. with grep), compared to '\0'. It is often interesting where a certain modifier is used, so searching the source code is important to give relevant results. Also, the modifier is really an enum and we shouldn't misuse char type. If that would be a good idea in general, we wouldn't need any enums at all. But we use them for good reasons. --- clients/cli/connections.c | 196 ++++++++++++++++++-------- clients/cli/settings.c | 8 +- clients/cli/settings.h | 2 +- clients/common/nm-meta-setting-desc.c | 35 +++-- clients/common/nm-meta-setting-desc.h | 8 +- 5 files changed, 175 insertions(+), 74 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index b8905cc78a..260844faec 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -3321,6 +3321,21 @@ get_valid_settings_array (const char *con_type) return NULL; } +static char * +_construct_property_name (const char *setting_name, + const char *property_name, + NMMetaAccessorModifier modifier) +{ + return g_strdup_printf ("%s%s.%s\n", + ( modifier == NM_META_ACCESSOR_MODIFIER_ADD + ? "+" + : ( modifier == NM_META_ACCESSOR_MODIFIER_DEL + ? "-" + : "")), + setting_name, + property_name); +} + /* get_valid_properties_string: * @array: base properties for the current connection type * @array_slv: slave properties (or ipv4/ipv6 ones) for the current connection type @@ -3338,7 +3353,7 @@ get_valid_settings_array (const char *con_type) static char * get_valid_properties_string (const NMMetaSettingValidPartItem *const*array, const NMMetaSettingValidPartItem *const*array_slv, - char modifier, + NMMetaAccessorModifier modifier, const char *prefix, const char *postfix) { @@ -3380,32 +3395,26 @@ get_valid_properties_string (const NMMetaSettingValidPartItem *const*array, /* Search the array with the arguments of the current property */ for (j = 0; j < setting_info->properties_num; j++) { - char *new; + gs_free char *ss1 = NULL; const char *arg_name; arg_name = setting_info->properties[j]->property_name; /* If required, expand the alias too */ - if (!postfix && setting_info->alias) { - if (modifier) - g_string_append_c (str, modifier); - new = g_strdup_printf ("%s.%s\n", - setting_info->alias, - arg_name); - g_string_append (str, new); - g_free (new); + if ( !postfix + && setting_info->alias) { + gs_free char *ss2 = NULL; + + ss2 = _construct_property_name (setting_info->alias, arg_name, modifier); + g_string_append (str, ss2); } - if (postfix && !g_str_has_prefix (arg_name, postfix)) + if ( postfix + && !g_str_has_prefix (arg_name, postfix)) continue; - if (modifier) - g_string_append_c (str, modifier); - new = g_strdup_printf ("%s.%s\n", - prop_name, - arg_name); - g_string_append (str, new); - g_free (new); + ss1 = _construct_property_name (prop_name, arg_name, modifier); + g_string_append (str, ss1); } } } @@ -4005,7 +4014,7 @@ set_property (NMClient *client, const char *setting_name, const char *property, const char *value, - char modifier, + NMMetaAccessorModifier modifier, GError **error) { gs_free char *property_name = NULL; @@ -4013,7 +4022,9 @@ set_property (NMClient *client, NMSetting *setting; nm_assert (setting_name && setting_name[0]); - nm_assert (NM_IN_SET (modifier, '\0', '+', '-')); + nm_assert (NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_SET, + NM_META_ACCESSOR_MODIFIER_ADD, + NM_META_ACCESSOR_MODIFIER_DEL)); setting = nm_connection_get_setting_by_name (connection, setting_name); if (!setting) { @@ -4033,14 +4044,15 @@ set_property (NMClient *client, if (!nmc_setting_set_property (client, setting, property_name, - ( (modifier == '-' && !value) - ? '\0' + ( ( modifier == NM_META_ACCESSOR_MODIFIER_DEL + && !value) + ? NM_META_ACCESSOR_MODIFIER_SET : modifier), value, &local)) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, _("Error: failed to %s %s.%s: %s."), - ( modifier != '-' + ( modifier != NM_META_ACCESSOR_MODIFIER_DEL ? "modify" : "remove a value from"), setting_name, @@ -4070,8 +4082,15 @@ set_option (NmCli *nmc, NMConnection *connection, const NMMetaAbstractInfo *abst if (option && option->check_and_set) { return option->check_and_set (nmc, connection, option, value, error); } else if (value) { - return set_property (nmc->client, connection, setting_name, property_name, - value, inf_flags & NM_META_PROPERTY_INF_FLAG_MULTI ? '+' : '\0', error); + return set_property (nmc->client, + connection, + setting_name, + property_name, + value, + inf_flags & NM_META_PROPERTY_INF_FLAG_MULTI + ? NM_META_ACCESSOR_MODIFIER_ADD + : NM_META_ACCESSOR_MODIFIER_SET, + error); } else if (inf_flags & NM_META_PROPERTY_INF_FLAG_REQD) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, _("Error: '%s' is mandatory."), option_name); @@ -4192,9 +4211,13 @@ set_connection_type (NmCli *nmc, NMConnection *con, const OptionInfo *option, co } if (slave_type) { - if (!set_property (nmc->client, con, NM_SETTING_CONNECTION_SETTING_NAME, - NM_SETTING_CONNECTION_SLAVE_TYPE, slave_type, - '\0', error)) { + if (!set_property (nmc->client, + con, + NM_SETTING_CONNECTION_SETTING_NAME, + NM_SETTING_CONNECTION_SLAVE_TYPE, + slave_type, + NM_META_ACCESSOR_MODIFIER_SET, + error)) { return FALSE; } enable_options (NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_MASTER, master); @@ -4209,7 +4232,13 @@ set_connection_type (NmCli *nmc, NMConnection *con, const OptionInfo *option, co NM_SETTING_CONNECTION_INTERFACE_NAME); } - if (!set_property (nmc->client, con, option->setting_info->general->setting_name, option->property, value, '\0', error)) + if (!set_property (nmc->client, + con, + option->setting_info->general->setting_name, + option->property, + value, + NM_META_ACCESSOR_MODIFIER_SET, + error)) return FALSE; if (!con_settings (con, &type_settings, &slv_settings, error)) @@ -4238,7 +4267,13 @@ set_connection_iface (NmCli *nmc, NMConnection *con, const OptionInfo *option, c } } - return set_property (nmc->client, con, option->setting_info->general->setting_name, option->property, value, '\0', error); + return set_property (nmc->client, + con, + option->setting_info->general->setting_name, + option->property, + value, + NM_META_ACCESSOR_MODIFIER_SET, + error); } static gboolean @@ -4261,13 +4296,23 @@ set_connection_master (NmCli *nmc, NMConnection *con, const OptionInfo *option, connections = nm_client_get_connections (nmc->client); value = normalized_master_for_slave (connections, value, slave_type, &slave_type); - if (!set_property (nmc->client, con, NM_SETTING_CONNECTION_SETTING_NAME, - NM_SETTING_CONNECTION_SLAVE_TYPE, slave_type, - '\0', error)) { + if (!set_property (nmc->client, + con, + NM_SETTING_CONNECTION_SETTING_NAME, + NM_SETTING_CONNECTION_SLAVE_TYPE, + slave_type, + NM_META_ACCESSOR_MODIFIER_SET, + error)) { return FALSE; } - return set_property (nmc->client, con, option->setting_info->general->setting_name, option->property, value, '\0', error); + return set_property (nmc->client, + con, + option->setting_info->general->setting_name, + option->property, + value, + NM_META_ACCESSOR_MODIFIER_SET, + error); } static gboolean @@ -4381,7 +4426,13 @@ set_bluetooth_type (NmCli *nmc, NMConnection *con, const OptionInfo *option, con return FALSE; } - return set_property (nmc->client, con, option->setting_info->general->setting_name, option->property, value, '\0', error); + return set_property (nmc->client, + con, + option->setting_info->general->setting_name, + option->property, + value, + NM_META_ACCESSOR_MODIFIER_SET, + error); } static gboolean @@ -4400,8 +4451,13 @@ set_ip4_address (NmCli *nmc, NMConnection *con, const OptionInfo *option, const NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_MANUAL, NULL); } - return set_property (nmc->client, con, option->setting_info->general->setting_name, option->property, value, - '+', error); + return set_property (nmc->client, + con, + option->setting_info->general->setting_name, + option->property, + value, + NM_META_ACCESSOR_MODIFIER_ADD, + error); } static gboolean @@ -4420,8 +4476,13 @@ set_ip6_address (NmCli *nmc, NMConnection *con, const OptionInfo *option, const NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_MANUAL, NULL); } - return set_property (nmc->client, con, option->setting_info->general->setting_name, option->property, value, - '+', error); + return set_property (nmc->client, + con, + option->setting_info->general->setting_name, + option->property, + value, + NM_META_ACCESSOR_MODIFIER_ADD, + error); } /*****************************************************************************/ @@ -4493,7 +4554,7 @@ option_relevant (NMConnection *connection, const NMMetaAbstractInfo *abstract_in static void complete_property_name (NmCli *nmc, NMConnection *connection, - char modifier, + NMMetaAccessorModifier modifier, const char *prefix, const char *postfix) { @@ -4516,7 +4577,7 @@ complete_property_name (NmCli *nmc, NMConnection *connection, if (word_list) g_print ("%s", word_list); - if (modifier != '\0') + if (modifier != NM_META_ACCESSOR_MODIFIER_SET) return; for (s = 0; s < _NM_META_SETTING_TYPE_NUM; s++) { @@ -4658,7 +4719,7 @@ nmc_read_connection_properties (NmCli *nmc, gs_strfreev char **strv = NULL; const NMMetaSettingValidPartItem *const*type_settings; const NMMetaSettingValidPartItem *const*slv_settings; - char modifier = '\0'; + NMMetaAccessorModifier modifier; if (!con_settings (connection, &type_settings, &slv_settings, error)) return FALSE; @@ -4673,8 +4734,11 @@ nmc_read_connection_properties (NmCli *nmc, return FALSE; } - if (option[0] == '+' || option[0] == '-') - modifier = *option; + switch (option[0]) { + case '+': modifier = NM_META_ACCESSOR_MODIFIER_ADD; break; + case '-': modifier = NM_META_ACCESSOR_MODIFIER_DEL; break; + default: modifier = NM_META_ACCESSOR_MODIFIER_SET; break; + } strv = g_strsplit (option, ".", 2); if (g_strv_length (strv) == 2) { @@ -4683,7 +4747,7 @@ nmc_read_connection_properties (NmCli *nmc, char *setting = strv[0]; const char *setting_name; - if (modifier) + if (modifier != NM_META_ACCESSOR_MODIFIER_SET) setting++; if (*argc == 1 && nmc->complete) @@ -4769,7 +4833,7 @@ nmc_read_connection_properties (NmCli *nmc, } if (!chosen) { - if (modifier) + if (modifier != NM_META_ACCESSOR_MODIFIER_SET) option++; if (*argc == 1 && nmc->complete) complete_property_name (nmc, connection, modifier, option, NULL); @@ -6981,8 +7045,8 @@ property_edit_submenu (NmCli *nmc, curr_setting, prop_name, (cmdsub == NMC_EDITOR_SUB_CMD_SET) - ? '\0' - : '+', + ? NM_META_ACCESSOR_MODIFIER_SET + : NM_META_ACCESSOR_MODIFIER_ADD, prop_val_user, &tmp_err); if (!set_result) { @@ -6998,7 +7062,12 @@ property_edit_submenu (NmCli *nmc, _("Edit '%s' value: "), prop_name); - if (!nmc_setting_set_property (nmc->client, curr_setting, prop_name, '\0', prop_val_user, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, + curr_setting, + prop_name, + NM_META_ACCESSOR_MODIFIER_SET, + prop_val_user, + &tmp_err)) { g_print (_("Error: failed to set '%s' property: %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); } @@ -7009,8 +7078,8 @@ property_edit_submenu (NmCli *nmc, curr_setting, prop_name, ( cmd_property_arg - ? '-' - : '\0'), + ? NM_META_ACCESSOR_MODIFIER_DEL + : NM_META_ACCESSOR_MODIFIER_SET), cmd_property_arg, &tmp_err)) { g_print (_("Error: %s\n"), tmp_err->message); @@ -7364,7 +7433,12 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t _("Enter '%s' value: "), prop_name); - if (!nmc_setting_set_property (nmc->client, menu_ctx.curr_setting, prop_name, '+', prop_val_user, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, + menu_ctx.curr_setting, + prop_name, + NM_META_ACCESSOR_MODIFIER_ADD, + prop_val_user, + &tmp_err)) { g_print (_("Error: failed to set '%s' property: %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); } @@ -7428,7 +7502,9 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t if (!nmc_setting_set_property (nmc->client, ss, prop_name, - cmd_arg_v ? '+' : '\0', + cmd_arg_v + ? NM_META_ACCESSOR_MODIFIER_ADD + : NM_META_ACCESSOR_MODIFIER_SET, cmd_arg_v, &tmp_err)) { g_print (_("Error: failed to set '%s' property: %s\n"), @@ -7527,7 +7603,12 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t if (!prop_name) break; - if (!nmc_setting_set_property (nmc->client, menu_ctx.curr_setting, prop_name, '\0', NULL, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, + menu_ctx.curr_setting, + prop_name, + NM_META_ACCESSOR_MODIFIER_SET, + NULL, + &tmp_err)) { g_print (_("Error: failed to remove value of '%s': %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); @@ -7577,7 +7658,12 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t prop_name = is_property_valid (ss, cmd_arg_p, &tmp_err); if (prop_name) { - if (!nmc_setting_set_property (nmc->client, ss, prop_name, '\0', NULL, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, + ss, + prop_name, + NM_META_ACCESSOR_MODIFIER_SET, + NULL, + &tmp_err)) { g_print (_("Error: failed to remove value of '%s': %s\n"), prop_name, tmp_err->message); diff --git a/clients/cli/settings.c b/clients/cli/settings.c index 91aaf28e38..fec36a89fd 100644 --- a/clients/cli/settings.c +++ b/clients/cli/settings.c @@ -532,7 +532,7 @@ gboolean nmc_setting_set_property (NMClient *client, NMSetting *setting, const char *prop, - char modifier, + NMMetaAccessorModifier modifier, const char *value, GError **error) { @@ -542,14 +542,14 @@ nmc_setting_set_property (NMClient *client, g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - g_return_val_if_fail (NM_IN_SET (modifier, '\0', '-', '+'), FALSE); + g_return_val_if_fail (NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_SET, NM_META_ACCESSOR_MODIFIER_DEL, NM_META_ACCESSOR_MODIFIER_ADD), FALSE); if (!(property_info = nm_meta_property_info_find_by_setting (setting, prop))) goto out_fail_read_only; if (!property_info->property_type->set_fcn) goto out_fail_read_only; - if ( modifier == '-' + if ( modifier == NM_META_ACCESSOR_MODIFIER_DEL && !property_info->property_type->set_supports_remove) { /* The property is a plain property. It does not support '-'. * @@ -572,7 +572,7 @@ nmc_setting_set_property (NMClient *client, } } - if ( NM_IN_SET (modifier, '+', '-') + if ( NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_ADD, NM_META_ACCESSOR_MODIFIER_DEL) && ( !value || !value[0])) { /* nothing to do. */ diff --git a/clients/cli/settings.h b/clients/cli/settings.h index 1ff936859c..62e8546616 100644 --- a/clients/cli/settings.h +++ b/clients/cli/settings.h @@ -45,7 +45,7 @@ char *nmc_setting_get_property_parsable (NMSetting *setting, gboolean nmc_setting_set_property (NMClient *client, NMSetting *setting, const char *prop, - char modifier, + NMMetaAccessorModifier modifier, const char *val, GError **error); diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c index d5014441cb..bdeb2026e8 100644 --- a/clients/common/nm-meta-setting-desc.c +++ b/clients/common/nm-meta-setting-desc.c @@ -638,7 +638,7 @@ _env_warn_fcn (const NMMetaEnvironment *environment, const NMMetaPropertyInfo *property_info, const NMMetaEnvironment *environment, gpointer environment_user_data, NMSetting *setting, NMMetaAccessorGetType get_type, NMMetaAccessorGetFlags get_flags, NMMetaAccessorGetOutFlags *out_flags, gboolean *out_is_default, gpointer *out_to_free #define ARGS_SET_FCN \ - const NMMetaPropertyInfo *property_info, const NMMetaEnvironment *environment, gpointer environment_user_data, NMSetting *setting, char modifier, const char *value, GError **error + const NMMetaPropertyInfo *property_info, const NMMetaEnvironment *environment, gpointer environment_user_data, NMSetting *setting, NMMetaAccessorModifier modifier, const char *value, GError **error #define ARGS_REMOVE_FCN \ const NMMetaPropertyInfo *property_info, const NMMetaEnvironment *environment, gpointer environment_user_data, NMSetting *setting, const char *value, GError **error @@ -653,43 +653,52 @@ _env_warn_fcn (const NMMetaEnvironment *environment, const NMMetaSettingInfoEditor *setting_info, NMSetting *setting, NMMetaAccessorSettingInitType init_type static gboolean -_SET_FCN_DO_RESET_DEFAULT (const NMMetaPropertyInfo *property_info, char modifier, const char *value) +_SET_FCN_DO_RESET_DEFAULT (const NMMetaPropertyInfo *property_info, NMMetaAccessorModifier modifier, const char *value) { nm_assert (property_info); nm_assert (!property_info->property_type->set_supports_remove); - nm_assert (NM_IN_SET (modifier, '\0', '+')); - nm_assert (value || modifier == '\0'); + nm_assert (NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_SET, + NM_META_ACCESSOR_MODIFIER_ADD)); + nm_assert ( value + || modifier == NM_META_ACCESSOR_MODIFIER_SET); return value == NULL; } static gboolean -_SET_FCN_DO_RESET_DEFAULT_WITH_SUPPORTS_REMOVE (const NMMetaPropertyInfo *property_info, char modifier, const char *value) +_SET_FCN_DO_RESET_DEFAULT_WITH_SUPPORTS_REMOVE (const NMMetaPropertyInfo *property_info, NMMetaAccessorModifier modifier, const char *value) { nm_assert (property_info); nm_assert (property_info->property_type->set_supports_remove); - nm_assert (NM_IN_SET (modifier, '\0', '+', '-')); - nm_assert (value || modifier == '\0'); + nm_assert (NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_SET, + NM_META_ACCESSOR_MODIFIER_ADD, + NM_META_ACCESSOR_MODIFIER_DEL)); + nm_assert ( value + || modifier == NM_META_ACCESSOR_MODIFIER_SET); return value == NULL; } static gboolean -_SET_FCN_DO_SET_ALL (char modifier, const char *value) +_SET_FCN_DO_SET_ALL (NMMetaAccessorModifier modifier, const char *value) { - nm_assert (NM_IN_SET (modifier, '\0', '+', '-')); + nm_assert (NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_SET, + NM_META_ACCESSOR_MODIFIER_ADD, + NM_META_ACCESSOR_MODIFIER_DEL)); nm_assert (value); - return modifier == '\0'; + return modifier == NM_META_ACCESSOR_MODIFIER_SET; } static gboolean -_SET_FCN_DO_REMOVE (char modifier, const char *value) +_SET_FCN_DO_REMOVE (NMMetaAccessorModifier modifier, const char *value) { - nm_assert (NM_IN_SET (modifier, '\0', '+', '-')); + nm_assert (NM_IN_SET (modifier, NM_META_ACCESSOR_MODIFIER_SET, + NM_META_ACCESSOR_MODIFIER_ADD, + NM_META_ACCESSOR_MODIFIER_DEL)); nm_assert (value); - return modifier == '-'; + return modifier == NM_META_ACCESSOR_MODIFIER_DEL; } #define RETURN_UNSUPPORTED_GET_TYPE() \ diff --git a/clients/common/nm-meta-setting-desc.h b/clients/common/nm-meta-setting-desc.h index 27ae4254b1..cb4fe4b259 100644 --- a/clients/common/nm-meta-setting-desc.h +++ b/clients/common/nm-meta-setting-desc.h @@ -129,6 +129,12 @@ typedef enum { _NM_META_COLOR_NUM } NMMetaColor; +typedef enum { + NM_META_ACCESSOR_MODIFIER_SET, + NM_META_ACCESSOR_MODIFIER_ADD, + NM_META_ACCESSOR_MODIFIER_DEL, +} NMMetaAccessorModifier; + typedef enum { NM_META_ACCESSOR_GET_TYPE_PRETTY, NM_META_ACCESSOR_GET_TYPE_PARSABLE, @@ -210,7 +216,7 @@ struct _NMMetaPropertyType { const NMMetaEnvironment *environment, gpointer environment_user_data, NMSetting *setting, - char modifier, + NMMetaAccessorModifier modifier, const char *value, GError **error); From b789ce01e90f84f5259aa001f2056e4cf897655d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Aug 2019 11:29:08 +0200 Subject: [PATCH 6/7] cli: fix handling modifier in nmc_read_connection_properties() for aliases Various cleanups: - after detecting the modifier, remove it from the string right away. It's redundant and confusing to do it later. - rename variables and move to inner scope. - don't use g_str_split() to split the property name at the first dot. strchr() is sufficient. Also, now that we strip the modifier from option early, they start also working for aliases. There is no need to not support (or behave differently) w.r.t. whether aliases support modifiers or not. This fixes: $ nmcli connection modify r +ip4 192.168.5.2/24 Error: invalid . 'ip4'. --- clients/cli/connections.c | 59 +++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index 260844faec..4ac295e6f2 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -4706,20 +4706,18 @@ nmc_read_connection_properties (NmCli *nmc, char ***argv, GError **error) { - const char *option; - const char *value = NULL; - GError *local = NULL; - /* First check if we have a slave-type, as this would mean we will not * have ip properties but possibly others, slave-type specific. */ /* Go through arguments and set properties */ do { - const NMMetaAbstractInfo *chosen = NULL; - gs_strfreev char **strv = NULL; const NMMetaSettingValidPartItem *const*type_settings; const NMMetaSettingValidPartItem *const*slv_settings; NMMetaAccessorModifier modifier; + const char *option_orig; + const char *option; + const char *value = NULL; + const char *tmp; if (!con_settings (connection, &type_settings, &slv_settings, error)) return FALSE; @@ -4727,57 +4725,58 @@ nmc_read_connection_properties (NmCli *nmc, ensure_settings (connection, slv_settings); ensure_settings (connection, type_settings); - option = **argv; - if (!option) { + option_orig = **argv; + if (!option_orig) { g_set_error_literal (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, _("Error: . argument is missing.")); return FALSE; } - switch (option[0]) { - case '+': modifier = NM_META_ACCESSOR_MODIFIER_ADD; break; - case '-': modifier = NM_META_ACCESSOR_MODIFIER_DEL; break; - default: modifier = NM_META_ACCESSOR_MODIFIER_SET; break; + switch (option_orig[0]) { + case '+': modifier = NM_META_ACCESSOR_MODIFIER_ADD; option = &option_orig[1]; break; + case '-': modifier = NM_META_ACCESSOR_MODIFIER_DEL; option = &option_orig[1]; break; + default: modifier = NM_META_ACCESSOR_MODIFIER_SET; option = option_orig; break; } - strv = g_strsplit (option, ".", 2); - if (g_strv_length (strv) == 2) { + if ((tmp = strchr (option, '.'))) { + gs_free char *option_sett = g_strndup (option, tmp - option); + const char *option_prop = &tmp[1]; + const char *option_sett_expanded; + GError *local = NULL; + /* This seems like a . (such as "connection.id" or "bond.mode"), * optionally prefixed with "+| or "-". */ - char *setting = strv[0]; - const char *setting_name; - if (modifier != NM_META_ACCESSOR_MODIFIER_SET) - setting++; + if ( *argc == 1 + && nmc->complete) + complete_property_name (nmc, connection, modifier, option_sett, option_prop); - if (*argc == 1 && nmc->complete) - complete_property_name (nmc, connection, modifier, setting, strv[1]); - - setting_name = check_valid_name (setting, type_settings, slv_settings, &local); - if (!setting_name) { + option_sett_expanded = check_valid_name (option_sett, type_settings, slv_settings, &local); + if (!option_sett_expanded) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, _("Error: invalid or not allowed setting '%s': %s."), - setting, local->message); + option_sett, local->message); g_clear_error (&local); return FALSE; } (*argc)--; (*argv)++; - if (!get_value (&value, argc, argv, option, error)) + if (!get_value (&value, argc, argv, option_orig, error)) return FALSE; if (!*argc && nmc->complete) { - complete_property (nmc, setting, strv[1], value ?: "", connection); + complete_property (nmc, option_sett, option_prop, value ?: "", connection); return TRUE; } - if (!set_property (nmc->client, connection, setting_name, strv[1], value, modifier, error)) + if (!set_property (nmc->client, connection, option_sett_expanded, option_prop, value, modifier, error)) return FALSE; } else { - NMMetaSettingType s; + const NMMetaAbstractInfo *chosen = NULL; const char *chosen_setting_name = NULL; const char *chosen_option = NULL; + NMMetaSettingType s; /* Let's see if this is an property alias (such as "id", "mode", "type" or "con-name")*/ for (s = 0; s < _NM_META_SETTING_TYPE_NUM; s++) { @@ -4833,8 +4832,6 @@ nmc_read_connection_properties (NmCli *nmc, } if (!chosen) { - if (modifier != NM_META_ACCESSOR_MODIFIER_SET) - option++; if (*argc == 1 && nmc->complete) complete_property_name (nmc, connection, modifier, option, NULL); g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, @@ -4847,7 +4844,7 @@ nmc_read_connection_properties (NmCli *nmc, (*argc)--; (*argv)++; - if (!get_value (&value, argc, argv, option, error)) + if (!get_value (&value, argc, argv, option_orig, error)) return FALSE; if (!*argc && nmc->complete) From cec39d76bdad0c9ad4281cae54c98b89b55fa2b8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Aug 2019 15:57:14 +0200 Subject: [PATCH 7/7] man/cli: better explain modifying properties regarding +/- modifiers --- man/nmcli.xml | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/man/nmcli.xml b/man/nmcli.xml index 43d469c4ca..e9517456f3 100644 --- a/man/nmcli.xml +++ b/man/nmcli.xml @@ -876,21 +876,23 @@ Add, modify or remove properties in the connection profile. To set the property just specify the property name followed by the - value. An empty value ("") removes the property value. + value. An empty value ("") resets the property value to + the default. In addition to the properties, you can also use short names for some of the properties. Consult the section for details. - If you want to append an item to the existing value, use - + prefix for the property name. If you want to remove just - one item from container-type property, use - prefix for - the property name and specify a value or an zero-based index of the item to - remove (or option name for properties with named options) as - value. The + and - - modifies only have a real effect for multi-value (container) - properties like ipv4.dns, ipv4.addresses, - bond.options, etc. + If you want to append an item or a flag to the existing value, use + + prefix for the property name or alias. If you want to + remove items from a container-type or flag property, use - prefix. + For certain properties you can also remove elements by specifying the zero-based + index(es). + The + and - modifiers + only have a real effect for properties that support them. + These are for example multi-value (container) properties or flags like ipv4.dns, + ip4, ipv4.addresses, bond.options, + 802-1x.phase1-auth-flags etc. See nm-settings5 for complete reference of setting and property names, their descriptions