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

@@ -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 != '-') {
+	if (!nmc_setting_set_property (client,
-		/* Set/add value */
+	                               setting,
-		if (modifier != '+') {
+	                               property_name,
-			/* We allow the existing property value to be passed as parameter,
+	                               (  (modifier == '-' && !value)
-			 * so make a copy if we are going to free it.
+	                                ? '\0'
-			 */
+	                                : modifier),
-			value = value_free = g_strdup (value);
+	                               value,
-			nmc_setting_reset_property (client, setting, property_name, NULL);
+	                               &local)) {
-		}
+		g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT,
-		if (!nmc_setting_set_property (client, setting, property_name, value, &local)) {
+		             _("Error: failed to %s %s.%s: %s."),
-			g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT,
+		             (  modifier != '-'
-			             _("Error: failed to modify %s.%s: %s."),
+		              ? "modify"
-			             setting_name, property, local->message);
+		              : "remove a value from"),
-			g_clear_error (&local);
+		             setting_name,
-			return FALSE;
+		             property,
-		}
+		             local->message);
-	} else {
+		return FALSE;
-		/* 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);
 	}
 
 	/* 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
+			set_result = nmc_setting_set_property (nmc->client,
-			 * remove the original value and save it for error cases.
+			                                       curr_setting,
-			 */
+			                                       prop_name,
-			if (cmdsub == NMC_EDITOR_SUB_CMD_SET) {
+			                                         (cmdsub == NMC_EDITOR_SUB_CMD_SET)
-				nmc_property_get_gvalue (curr_setting, prop_name, &prop_g_value);
+			                                       ? '\0'
-				nmc_setting_reset_property (nmc->client, curr_setting, prop_name, NULL);
+			                                       : '+',
-			}
+			                                       prop_val_user,
-
+			                                       &tmp_err);
-			set_result = nmc_setting_set_property (nmc->client, curr_setting, prop_name, 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);
+			if (!nmc_setting_set_property (nmc->client, curr_setting, prop_name, '\0', prop_val_user, &tmp_err)) {
-			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)) {
 				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_set_property (nmc->client,
-				if (!nmc_setting_remove_property_option (curr_setting,
+			                               curr_setting,
-				                                         prop_name,
+			                               prop_name,
-				                                         cmd_property_arg,
+			                               (  cmd_property_arg
-				                                         &tmp_err)) {
+			                                ? '-'
-					g_print (_("Error: %s\n"), tmp_err->message);
+			                                : '\0'),
-					g_clear_error (&tmp_err);
+			                               cmd_property_arg,
-				}
+			                               &tmp_err)) {
-			} else {
+				g_print (_("Error: %s\n"), tmp_err->message);
-				if (!nmc_setting_reset_property (nmc->client, curr_setting, prop_name, &tmp_err)) {
+				g_clear_error (&tmp_err);
-					g_print (_("Error: failed to remove value of '%s': %s\n"), prop_name,
-					         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,
-				if (!nmc_setting_set_property (nmc->client, ss, prop_name, cmd_arg_v, &tmp_err)) {
+				                               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);

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) {
+	if (modifier == '-') {
-		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 (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

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);