[Asterisk-code-review] Stasis: Use custom structure when setting variables. (asterisk[13])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: Stasis: Use custom structure when setting variables.
......................................................................


Stasis: Use custom structure when setting variables.

A recent change to queue channel variable setting to the Stasis control
queue caused a regression. When setting channel variables, it is
possible to give a NULL channel variable value in order to unset the
variable (i.e. remove it from the channel variable list). The change
introduced a call to ast_variable_new(), which is not tolerant of NULL
channel variable values.

This new change switches from using ast_variable to using a custom
channel variable struct that is lighter weight and NULL value-tolerant.

Change-Id: I784d7beaaa3c036ea936d103e7caf0bb1562162d
---
M res/stasis/control.c
1 file changed, 45 insertions(+), 4 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/stasis/control.c b/res/stasis/control.c
index 57b5e99..ebb7e01 100644
--- a/res/stasis/control.c
+++ b/res/stasis/control.c
@@ -623,10 +623,36 @@
 	return 0;
 }
 
+/*!
+ * \brief structure for queuing ARI channel variable setting
+ *
+ * It may seem weird to define this custom structure given that we already have
+ * ast_var_t and ast_variable defined elsewhere. The problem with those is that
+ * they are not tolerant of NULL channel variable value pointers. In fact, in both
+ * cases, the best they could do is to have a zero-length variable value. However,
+ * when un-setting a channel variable, it is important to pass a NULL value, not
+ * a zero-length string.
+ */
+struct chanvar {
+	/*! Name of variable to set/unset */
+	char *name;
+	/*! Value of variable to set. If unsetting, this will be NULL */
+	char *value;
+};
+
+static void free_chanvar(void *data)
+{
+	struct chanvar *var = data;
+
+	ast_free(var->name);
+	ast_free(var->value);
+	ast_free(var);
+}
+
 static int app_control_set_channel_var(struct stasis_app_control *control,
 	struct ast_channel *chan, void *data)
 {
-	struct ast_variable *var = data;
+	struct chanvar *var = data;
 
 	pbx_builtin_setvar_helper(control->channel, var->name, var->value);
 
@@ -635,14 +661,29 @@
 
 int stasis_app_control_set_channel_var(struct stasis_app_control *control, const char *variable, const char *value)
 {
-	struct ast_variable *var;
+	struct chanvar *var;
 
-	var = ast_variable_new(variable, value, "ARI");
+	var = ast_calloc(1, sizeof(*var));
 	if (!var) {
 		return -1;
 	}
 
-	stasis_app_send_command_async(control, app_control_set_channel_var, var, ast_free_ptr);
+	var->name = ast_strdup(variable);
+	if (!var->name) {
+		free_chanvar(var);
+		return -1;
+	}
+
+	/* It's kosher for value to be NULL. It means the variable is being unset */
+	if (value) {
+		var->value = ast_strdup(value);
+		if (!var->value) {
+			free_chanvar(var);
+			return -1;
+		}
+	}
+
+	stasis_app_send_command_async(control, app_control_set_channel_var, var, free_chanvar);
 
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/2076
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I784d7beaaa3c036ea936d103e7caf0bb1562162d
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>