From 649968632ea065d7b4cbf213ec1cc04a0736bc33 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 18 Mar 2019 11:08:41 +0100 Subject: [PATCH] cli: merge remove-property, reset-property and set-property It's fundamentally wrong to have separate "remove_fcn" and "set_fcn" implementations. Set, reset, add, and remove are all similar, and should be implemented in a similar manner. Merge the implementations all in set-property, which now can: - reset the value (value == NULL && modifier == '\0') - set a value (value != NULL && modifier == '\0') - add a value (value != NULL && modifier == '+') - remove a value (value != NULL && modifier == '-') The real problem is that remove_fcn() behaves fundamentally different from set_fcn(). You can do "+setting.property value1,value2" but you cannot remove them the same way. That is because most remove_fcn() implementations don't expect a list of values. But it's also because of the unnatural split between set_fcn() and remove_fcn(). The next commit will merge set_fcn(), remove_fcn() and reset property. This commit just merges them all in nmc_setting_set_property(). --- clients/cli/connections.c | 129 +++++++++++------------------ clients/cli/settings.c | 166 +++++++++++++++----------------------- clients/cli/settings.h | 17 +--- 3 files changed, 115 insertions(+), 197 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index fe8aff39cf..0235de8132 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -3959,11 +3959,12 @@ set_property (NMClient *client, char modifier, GError **error) { - gs_free char *property_name = NULL, *value_free = NULL; + gs_free char *property_name = NULL; + gs_free_error GError *local = NULL; NMSetting *setting; - GError *local = NULL; - g_assert (setting_name && setting_name[0]); + nm_assert (setting_name && setting_name[0]); + nm_assert (NM_IN_SET (modifier, '\0', '+', '-')); setting = nm_connection_get_setting_by_name (connection, setting_name); if (!setting) { @@ -3977,43 +3978,26 @@ set_property (NMClient *client, g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, _("Error: invalid property '%s': %s."), property, local->message); - g_clear_error (&local); return FALSE; } - if (modifier != '-') { - /* Set/add value */ - if (modifier != '+') { - /* We allow the existing property value to be passed as parameter, - * so make a copy if we are going to free it. - */ - value = value_free = g_strdup (value); - nmc_setting_reset_property (client, setting, property_name, NULL); - } - if (!nmc_setting_set_property (client, setting, property_name, value, &local)) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("Error: failed to modify %s.%s: %s."), - setting_name, property, local->message); - g_clear_error (&local); - return FALSE; - } - } else { - /* Remove value - * - either empty: remove whole value - * - or specified by index <0-n>: remove item at the index - * - or option name: remove item with the option name - */ - if (value) { - nmc_setting_remove_property_option (setting, property_name, value, &local); - if (local) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("Error: failed to remove a value from %s.%s: %s."), - setting_name, property, local->message); - g_clear_error (&local); - return FALSE; - } - } else - nmc_setting_reset_property (client, setting, property_name, NULL); + if (!nmc_setting_set_property (client, + setting, + property_name, + ( (modifier == '-' && !value) + ? '\0' + : modifier), + value, + &local)) { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("Error: failed to %s %s.%s: %s."), + ( modifier != '-' + ? "modify" + : "remove a value from"), + setting_name, + property, + local->message); + return FALSE; } /* Don't ask for this property in interactive mode. */ @@ -6899,7 +6883,6 @@ property_edit_submenu (NmCli *nmc, gs_free char *cmd_property_user = NULL; gs_free char *cmd_property_arg = NULL; gs_free char *prop_val_user = NULL; - nm_auto_unset_gvalue GValue prop_g_value = G_VALUE_INIT; gboolean removed; gboolean dirty; @@ -6949,24 +6932,17 @@ property_edit_submenu (NmCli *nmc, } else prop_val_user = g_strdup (cmd_property_arg); - /* nmc_setting_set_property() only adds new value, thus we have to - * remove the original value and save it for error cases. - */ - if (cmdsub == NMC_EDITOR_SUB_CMD_SET) { - nmc_property_get_gvalue (curr_setting, prop_name, &prop_g_value); - nmc_setting_reset_property (nmc->client, curr_setting, prop_name, NULL); - } - - set_result = nmc_setting_set_property (nmc->client, curr_setting, prop_name, prop_val_user, &tmp_err); + set_result = nmc_setting_set_property (nmc->client, + curr_setting, + prop_name, + (cmdsub == NMC_EDITOR_SUB_CMD_SET) + ? '\0' + : '+', + prop_val_user, + &tmp_err); if (!set_result) { g_print (_("Error: failed to set '%s' property: %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); - if (cmdsub == NMC_EDITOR_SUB_CMD_SET) { - /* Block change signals and restore original value */ - g_signal_handlers_block_matched (curr_setting, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, NULL); - nmc_property_set_gvalue (curr_setting, prop_name, &prop_g_value); - g_signal_handlers_unblock_matched (curr_setting, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, NULL); - } } break; @@ -6977,33 +6953,23 @@ property_edit_submenu (NmCli *nmc, _("Edit '%s' value: "), prop_name); - nmc_property_get_gvalue (curr_setting, prop_name, &prop_g_value); - nmc_setting_reset_property (nmc->client, curr_setting, prop_name, NULL); - - if (!nmc_setting_set_property (nmc->client, curr_setting, prop_name, prop_val_user, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, curr_setting, prop_name, '\0', 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); - g_signal_handlers_block_matched (curr_setting, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, NULL); - nmc_property_set_gvalue (curr_setting, prop_name, &prop_g_value); - g_signal_handlers_unblock_matched (curr_setting, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, NULL); } break; case NMC_EDITOR_SUB_CMD_REMOVE: - if (cmd_property_arg) { - if (!nmc_setting_remove_property_option (curr_setting, - prop_name, - cmd_property_arg, - &tmp_err)) { - g_print (_("Error: %s\n"), tmp_err->message); - g_clear_error (&tmp_err); - } - } else { - if (!nmc_setting_reset_property (nmc->client, curr_setting, prop_name, &tmp_err)) { - g_print (_("Error: failed to remove value of '%s': %s\n"), prop_name, - tmp_err->message); - g_clear_error (&tmp_err); - } + if (!nmc_setting_set_property (nmc->client, + curr_setting, + prop_name, + ( cmd_property_arg + ? '-' + : '\0'), + cmd_property_arg, + &tmp_err)) { + g_print (_("Error: %s\n"), tmp_err->message); + g_clear_error (&tmp_err); } break; @@ -7354,8 +7320,7 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t _("Enter '%s' value: "), prop_name); - /* Set property value */ - 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, '+', 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); } @@ -7415,8 +7380,12 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t prop_name); } - /* Set property value */ - if (!nmc_setting_set_property (nmc->client, ss, prop_name, cmd_arg_v, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, + ss, + prop_name, + cmd_arg_v ? '+' : '\0', + cmd_arg_v, + &tmp_err)) { g_print (_("Error: failed to set '%s' property: %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); @@ -7513,7 +7482,7 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t if (!prop_name) break; - if (!nmc_setting_reset_property (nmc->client, menu_ctx.curr_setting, prop_name, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, menu_ctx.curr_setting, prop_name, '\0', NULL, &tmp_err)) { g_print (_("Error: failed to remove value of '%s': %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); @@ -7563,7 +7532,7 @@ 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_reset_property (nmc->client, ss, prop_name, &tmp_err)) { + if (!nmc_setting_set_property (nmc->client, ss, prop_name, '\0', NULL, &tmp_err)) { g_print (_("Error: failed to remove value of '%s': %s\n"), prop_name, tmp_err->message); g_clear_error (&tmp_err); diff --git a/clients/cli/settings.c b/clients/cli/settings.c index 34fb03704a..416f5a0c12 100644 --- a/clients/cli/settings.c +++ b/clients/cli/settings.c @@ -509,46 +509,35 @@ nmc_setting_get_property_parsable (NMSetting *setting, const char *prop, GError return get_property_val (setting, prop, NM_META_ACCESSOR_GET_TYPE_PARSABLE, TRUE, error); } -static gboolean -_set_fcn_call (const NMMetaPropertyInfo *property_info, - NMSetting *setting, - const char *value, - GError **error) -{ - return property_info->property_type->set_fcn (property_info, - nmc_meta_environment, - nmc_meta_environment_arg, - setting, - value, - error); -} - -/* - * Generic function for setting property value. - * - * Sets property=value in setting by calling specialized functions. - * If value is NULL then default property value is set. - * - * Returns: TRUE on success; FALSE on failure and sets error - */ gboolean -nmc_setting_set_property (NMClient *client, NMSetting *setting, const char *prop, const char *value, GError **error) +nmc_setting_set_property (NMClient *client, + NMSetting *setting, + const char *prop, + char modifier, + const char *value, + GError **error) { + nm_auto_unset_gvalue GValue gvalue_old = G_VALUE_INIT; const NMMetaPropertyInfo *property_info; + gs_free char *value_to_free = NULL; + /* FIXME: any mentioning of GParamSpec only works for GObject base properties. That is + * wrong, the property meta-data must handle all properties. */ + GParamSpec *param_spec = NULL; 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); 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 (!value) { nm_auto_unset_gvalue GValue gvalue = G_VALUE_INIT; - GParamSpec *param_spec; - /* No value argument sets default value */ + g_return_val_if_fail (modifier == '\0', TRUE); - /* FIXME: this only works with GObject based properties. */ param_spec = g_object_class_find_property (G_OBJECT_GET_CLASS (G_OBJECT (setting)), prop); if (param_spec) { g_value_init (&gvalue, G_PARAM_SPEC_VALUE_TYPE (param_spec)); @@ -558,46 +547,7 @@ nmc_setting_set_property (NMClient *client, NMSetting *setting, const char *prop return TRUE; } - if (property_info->property_type->set_fcn) { - gs_free char *value_to_free = NULL; - - switch (property_info->setting_info->general->meta_type) { - case NM_META_SETTING_TYPE_CONNECTION: - if (nm_streq (property_info->property_name, NM_SETTING_CONNECTION_SECONDARIES)) { - if (!_set_fcn_precheck_connection_secondaries (client, value, &value_to_free, error)) - return FALSE; - if (value_to_free) - value = value_to_free; - } - break; - default: - break; - } - - return _set_fcn_call (property_info, - setting, - value, - error); - } - -out_fail_read_only: - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, _("the property can't be changed")); - return FALSE; -} - -gboolean -nmc_setting_remove_property_option (NMSetting *setting, - const char *prop, - const char *value, - GError **error) -{ - const NMMetaPropertyInfo *property_info; - - g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); - g_return_val_if_fail (!error || !*error, FALSE); - g_return_val_if_fail (value, FALSE); - - if ((property_info = nm_meta_property_info_find_by_setting (setting, prop))) { + if (modifier == '-') { if (property_info->property_type->remove_fcn) { return property_info->property_type->remove_fcn (property_info, nmc_meta_environment, @@ -606,9 +556,58 @@ nmc_setting_remove_property_option (NMSetting *setting, value, error); } + return TRUE; + } + + switch (property_info->setting_info->general->meta_type) { + case NM_META_SETTING_TYPE_CONNECTION: + if (nm_streq (property_info->property_name, NM_SETTING_CONNECTION_SECONDARIES)) { + if (!_set_fcn_precheck_connection_secondaries (client, value, &value_to_free, error)) + return FALSE; + if (value_to_free) + value = value_to_free; + } + break; + default: + break; + } + + if (modifier == '\0') { + /* FIXME: reset the value. By default, "set_fcn" adds values (don't ask). */ + param_spec = g_object_class_find_property (G_OBJECT_GET_CLASS (G_OBJECT (setting)), prop); + if (param_spec) { + nm_auto_unset_gvalue GValue gvalue = G_VALUE_INIT; + + /* get the current value, to restore it on failure below. */ + g_value_init (&gvalue_old, G_PARAM_SPEC_VALUE_TYPE (param_spec)); + g_object_get_property (G_OBJECT (setting), prop, &gvalue_old); + + g_value_init (&gvalue, G_PARAM_SPEC_VALUE_TYPE (param_spec)); + g_param_value_set_default (param_spec, &gvalue); + g_object_set_property (G_OBJECT (setting), prop, &gvalue); + } + } + + if (!property_info->property_type->set_fcn (property_info, + nmc_meta_environment, + nmc_meta_environment_arg, + setting, + value, + error)) { + if ( modifier == '\0' + && param_spec) { + /* restore the previous value. */ + g_object_set_property (G_OBJECT (setting), prop, &gvalue_old); + } + + return FALSE; } return TRUE; + +out_fail_read_only: + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, _("the property can't be changed")); + return FALSE; } /* @@ -706,41 +705,6 @@ nmc_setting_get_property_desc (NMSetting *setting, const char *prop) nmcli_desc ?: ""); } -/* - * Gets setting:prop property value and returns it in 'value'. - * Caller is responsible for freeing the GValue resources using g_value_unset() - */ -gboolean -nmc_property_get_gvalue (NMSetting *setting, const char *prop, GValue *value) -{ - GParamSpec *param_spec; - - param_spec = g_object_class_find_property (G_OBJECT_GET_CLASS (G_OBJECT (setting)), prop); - if (param_spec) { - memset (value, 0, sizeof (GValue)); - g_value_init (value, G_PARAM_SPEC_VALUE_TYPE (param_spec)); - g_object_get_property (G_OBJECT (setting), prop, value); - return TRUE; - } - return FALSE; -} - -/* - * Sets setting:prop property value from 'value'. - */ -gboolean -nmc_property_set_gvalue (NMSetting *setting, const char *prop, GValue *value) -{ - GParamSpec *param_spec; - - param_spec = g_object_class_find_property (G_OBJECT_GET_CLASS (G_OBJECT (setting)), prop); - if (param_spec && G_VALUE_TYPE (value) == G_PARAM_SPEC_VALUE_TYPE (param_spec)) { - g_object_set_property (G_OBJECT (setting), prop, value); - return TRUE; - } - return FALSE; -} - /*****************************************************************************/ gboolean diff --git a/clients/cli/settings.h b/clients/cli/settings.h index 931ecdfe08..1ff936859c 100644 --- a/clients/cli/settings.h +++ b/clients/cli/settings.h @@ -45,24 +45,9 @@ char *nmc_setting_get_property_parsable (NMSetting *setting, gboolean nmc_setting_set_property (NMClient *client, NMSetting *setting, const char *prop, + char modifier, const char *val, GError **error); -static inline gboolean -nmc_setting_reset_property (NMClient *client, - NMSetting *setting, - const char *prop, - GError **error) -{ - return nmc_setting_set_property (client, setting, prop, NULL, error); -} - -gboolean nmc_setting_remove_property_option (NMSetting *setting, - const char *prop, - const char *value, - GError **error); - -gboolean nmc_property_get_gvalue (NMSetting *setting, const char *prop, GValue *value); -gboolean nmc_property_set_gvalue (NMSetting *setting, const char *prop, GValue *value); gboolean setting_details (const NmcConfig *nmc_config, NMSetting *setting, const char *one_prop);