Index: pbx.c
===================================================================
--- pbx.c	(revision 7691)
+++ pbx.c	(working copy)
@@ -821,40 +827,33 @@
 /* Note that it's negative -- that's important later. */
 #define DONT_HAVE_LENGTH	0x80000000
 
+/*
+ * Returns 1 if there is a offset:length part, which is
+ * trimmed off (values go into variables)
+ */
 static int parse_variable_name(char *var, int *offset, int *length, int *isfunc)
 {
-	char *varchar, *offsetchar = NULL;
 	int parens=0;
 
 	*offset = 0;
 	*length = DONT_HAVE_LENGTH;
 	*isfunc = 0;
-	for (varchar = var; *varchar; varchar++) {
-		switch (*varchar) {
-		case '(':
+	for (; *var; var++) {
+		if (*var == '(') {
 			(*isfunc)++;
 			parens++;
-			break;
-		case ')':
+		} else if (*var == ')') {
 			parens--;
-			break;
-		case ':':
-			if (parens == 0) {
-				offsetchar = varchar + 1;
-				*varchar = '\0';
-				goto pvn_endfor;
-			}
+		} else if (*var == ':' && parens == 0) {
+			*var++ = '\0';
+			sscanf(var, "%d:%d", offset, length);
+			return 1; /* offset:length valid */
 		}
 	}
-pvn_endfor:
-	if (offsetchar) {
-		sscanf(offsetchar, "%d:%d", offset, length);
-		return 1;
-	} else {
-		return 0;
-	}
+	return 0;
 }
 
+/* takes a substring. It is ok to call with value == workspace. */
 static char *substring(char *value, int offset, int length, char *workspace, size_t workspace_len)
 {
 	char *ret = workspace;
@@ -899,127 +898,132 @@
   ---*/
 void pbx_retrieve_variable(struct ast_channel *c, const char *var, char **ret, char *workspace, int workspacelen, struct varshead *headp)
 {
-	char tmpvar[80];
-	time_t thistime;
-	struct tm brokentime;
-	int offset, offset2, isfunc;
-	struct ast_var_t *variables;
-	char *deprecated = NULL;
+	char tmpvar[80], *deprecated = NULL;
+	const char *s;	/* the result */
+	int offset, length;
+	int i, found = 1, need_substring;
+	struct varshead *places[3] = { headp, &globals, NULL};	/* list of places where we may look */
 
-	if (c) 
-		headp=&c->varshead;
-	*ret=NULL;
-	ast_copy_string(tmpvar, var, sizeof(tmpvar));
-	if (parse_variable_name(tmpvar, &offset, &offset2, &isfunc)) {
-		pbx_retrieve_variable(c, tmpvar, ret, workspace, workspacelen, headp);
-		if (!(*ret)) 
-			return;
-		*ret = substring(*ret, offset, offset2, workspace, workspacelen);
-	} else if (c && !strncmp(var, "CALL", 4)) {
+	if (c) {
+		places[0] = &c->varshead;
+#if 0 /* proposed modification */
+		/* XXX In the original code, if c is set, the headp passed
+		 * as an argument is overridden by c->varshead.
+		 * Do we really want this, or perhaps should also look there
+		 * before globals ? A proposed modification is below.
+		 */
+		places[1] = headp;
+		places[2] = globals;
+#endif
+	}
+	ast_copy_string(tmpvar, var, sizeof(tmpvar));	/* parse_variable_name modifies the string */
+	/* If called directly, we may need to run a substring() on the result */
+	need_substring = parse_variable_name(tmpvar, &offset, &length, &i /* ignored */);
+
+	/*
+	 * Look into predefined variables, then into variable lists.
+	 * 'found' is set to 0 if we did not match a name and need to look
+	 * into more places. If found=1, the result string in s is valid as follows:
+	 * s = NULL if the variable does not have a value;
+	 * s = workspace if the result has been assembled there;
+	 * s != workspace in case we have a string, that needs to be copied
+	 *	(so we have the ast_copy_string only in one place);
+	 */
+	found = 1;	/* default. set to 0 explicitly when we need */
+	s = NULL;	/* default value */
+  if (c) {	/* This group requires a valid channel */
+	/* Names with common parts are looked up a piece at a time using strncmp. */
+	if (!strncmp(var, "CALL", 4)) {
 		if (!strncmp(var + 4, "ER", 2)) {
 			if (!strncmp(var + 6, "ID", 2)) {
 				if (!var[8]) { 			/* CALLERID */
 					if (c->cid.cid_num) {
 						if (c->cid.cid_name) {
 							snprintf(workspace, workspacelen, "\"%s\" <%s>", c->cid.cid_name, c->cid.cid_num);
+							s = workspace;
 						} else {
-							ast_copy_string(workspace, c->cid.cid_num, workspacelen);
+							s = c->cid.cid_num;
 						}
-						*ret = workspace;
 					} else if (c->cid.cid_name) {
-						ast_copy_string(workspace, c->cid.cid_name, workspacelen);
-						*ret = workspace;
-					} else
-						*ret = NULL;
+						s = c->cid.cid_name;
+					}
 					deprecated = "CALLERID(all)";
-				} else if (!strcmp(var + 8, "NUM")) {
-					/* CALLERIDNUM */
-					if (c->cid.cid_num) {
-						ast_copy_string(workspace, c->cid.cid_num, workspacelen);
-						*ret = workspace;
-					} else
-						*ret = NULL;
+				} else if (!strcmp(var + 8, "NUM")) { /* CALLERIDNUM */
+					s = c->cid.cid_num;
 					deprecated = "CALLERID(num)";
-				} else if (!strcmp(var + 8, "NAME")) {
-					/* CALLERIDNAME */
-					if (c->cid.cid_name) {
-						ast_copy_string(workspace, c->cid.cid_name, workspacelen);
-						*ret = workspace;
-					} else
-						*ret = NULL;
+				} else if (!strcmp(var + 8, "NAME")) { /* CALLERIDNAME */
+					s = c->cid.cid_name;
 					deprecated = "CALLERID(name)";
 				} else
-					goto icky;
-			} else if (!strcmp(var + 6, "ANI")) {
-				/* CALLERANI */
-				if (c->cid.cid_ani) {
-					ast_copy_string(workspace, c->cid.cid_ani, workspacelen);
-					*ret = workspace;
-				} else
-					*ret = NULL;
+					found = 0;	/* continue the search */
+			} else if (!strcmp(var + 6, "ANI")) { /* CALLERANI */
+				s = c->cid.cid_ani;
 				deprecated = "CALLERID(ANI)";
 			} else
-				goto icky;
+				found = 0; /* continue the search */
 		} else if (!strncmp(var + 4, "ING", 3)) {
 			if (!strcmp(var + 7, "PRES")) {
 				/* CALLINGPRES */
 				snprintf(workspace, workspacelen, "%d", c->cid.cid_pres);
-				*ret = workspace;
+				s = workspace;
 			} else if (!strcmp(var + 7, "ANI2")) {
 				/* CALLINGANI2 */
 				snprintf(workspace, workspacelen, "%d", c->cid.cid_ani2);
-				*ret = workspace;
+				s = workspace;
 			} else if (!strcmp(var + 7, "TON")) {
 				/* CALLINGTON */
 				snprintf(workspace, workspacelen, "%d", c->cid.cid_ton);
-				*ret = workspace;
+				s = workspace;
 			} else if (!strcmp(var + 7, "TNS")) {
 				/* CALLINGTNS */
 				snprintf(workspace, workspacelen, "%d", c->cid.cid_tns);
-				*ret = workspace;
+				s = workspace;
 			} else
-				goto icky;
+				found = 0; /* continue the search */
 		} else
-			goto icky;
-	} else if (c && !strcmp(var, "DNID")) {
-		if (c->cid.cid_dnid) {
-			ast_copy_string(workspace, c->cid.cid_dnid, workspacelen);
-			*ret = workspace;
-		} else
-			*ret = NULL;
+			found = 0; /* continue the search */
+	} else if (!strcmp(var, "DNID")) {
+		s = c->cid.cid_dnid;
 		deprecated = "CALLERID(DNID)";
-	} else if (c && !strcmp(var, "HINT")) {
-		if (!ast_get_hint(workspace, workspacelen, NULL, 0, c, c->context, c->exten))
-			*ret = NULL;
-		else
-			*ret = workspace;
-	} else if (c && !strcmp(var, "HINTNAME")) {
-		if (!ast_get_hint(NULL, 0, workspace, workspacelen, c, c->context, c->exten))
-			*ret = NULL;
-		else
-			*ret = workspace;
-	} else if (c && !strcmp(var, "EXTEN")) {
-		ast_copy_string(workspace, c->exten, workspacelen);
-		*ret = workspace;
-	} else if (c && !strcmp(var, "RDNIS")) {
-		if (c->cid.cid_rdnis) {
-			ast_copy_string(workspace, c->cid.cid_rdnis, workspacelen);
-			*ret = workspace;
-		} else
-			*ret = NULL;
+	} else if (!strcmp(var, "HINT")) {
+		if (ast_get_hint(workspace, workspacelen, NULL, 0, c, c->context, c->exten))
+			s = workspace;
+	} else if (!strcmp(var, "HINTNAME")) {
+		if (ast_get_hint(NULL, 0, workspace, workspacelen, c, c->context, c->exten))
+			s = workspace;
+	} else if (!strcmp(var, "EXTEN")) {
+		s = c->exten;
+	} else if (!strcmp(var, "RDNIS")) {
+		s = c->cid.cid_rdnis;
 		deprecated = "CALLERID(RDNIS)";
-	} else if (c && !strcmp(var, "CONTEXT")) {
-		ast_copy_string(workspace, c->context, workspacelen);
-		*ret = workspace;
-	} else if (c && !strcmp(var, "PRIORITY")) {
+	} else if (!strcmp(var, "CONTEXT")) {
+		s = c->context;
+	} else if (!strcmp(var, "PRIORITY")) {
 		snprintf(workspace, workspacelen, "%d", c->priority);
-		*ret = workspace;
-	} else if (c && !strcmp(var, "CHANNEL")) {
-		ast_copy_string(workspace, c->name, workspacelen);
-		*ret = workspace;
-	} else if (!strcmp(var, "EPOCH")) {
+		s = workspace;
+	} else if (!strcmp(var, "CHANNEL")) {
+		s = c->name;
+	} else if (!strcmp(var, "UNIQUEID")) {
+		s = c->uniqueid;
+	} else if (!strcmp(var, "HANGUPCAUSE")) {
+		snprintf(workspace, workspacelen, "%d", c->hangupcause);
+		s = workspace;
+	} else if (!strcmp(var, "ACCOUNTCODE")) {
+		s = c->accountcode;
+		deprecated = "CDR(accountcode)";
+	} else if (!strcmp(var, "LANGUAGE")) {
+		s = c->language;
+		deprecated = "LANGUAGE()";
+	} else
+		found = 0; /* continue the search */
+  }
+  if (!found) { /* look for more */
+	time_t thistime;
+	struct tm brokentime;
+
+	if (!strcmp(var, "EPOCH")) {
 		snprintf(workspace, workspacelen, "%u",(int)time(NULL));
-		*ret = workspace;
+		s = workspace;
 	} else if (!strcmp(var, "DATETIME")) {
 		thistime=time(NULL);
 		localtime_r(&thistime, &brokentime);
@@ -1031,7 +1035,7 @@
 			brokentime.tm_min,
 			brokentime.tm_sec
 		);
-		*ret = workspace;
+		s = workspace;
 		deprecated = "STRFTIME(${EPOCH},,\%m\%d\%Y-\%H:\%M:\%S)";
 	} else if (!strcmp(var, "TIMESTAMP")) {
 		thistime=time(NULL);
@@ -1045,58 +1049,36 @@
 			brokentime.tm_min,
 			brokentime.tm_sec
 		);
-		*ret = workspace;
+		s = workspace;
 		deprecated = "STRFTIME(${EPOCH},,\%Y\%m\%d-\%H\%M\%S)";
-	} else if (c && !strcmp(var, "UNIQUEID")) {
-		snprintf(workspace, workspacelen, "%s", c->uniqueid);
-		*ret = workspace;
-	} else if (c && !strcmp(var, "HANGUPCAUSE")) {
-		snprintf(workspace, workspacelen, "%d", c->hangupcause);
-		*ret = workspace;
-	} else if (c && !strcmp(var, "ACCOUNTCODE")) {
-		ast_copy_string(workspace, c->accountcode, workspacelen);
-		*ret = workspace;
-		deprecated = "CDR(accountcode)";
-	} else if (c && !strcmp(var, "LANGUAGE")) {
-		ast_copy_string(workspace, c->language, workspacelen);
-		*ret = workspace;
-		deprecated = "LANGUAGE()";
-	} else {
-icky:
-		if (headp) {
-			AST_LIST_TRAVERSE(headp,variables,entries) {
-#if 0
-				ast_log(LOG_WARNING,"Comparing variable '%s' with '%s'\n",var,ast_var_name(variables));
-#endif
-				if (strcasecmp(ast_var_name(variables),var)==0) {
-					const char *s = ast_var_value(variables);
-					if (s) {
-						ast_copy_string(workspace, s, workspacelen);
-						*ret = workspace;
-					}
-					break;
-				}
+	} else
+		found = 0;
+  }
+	/* if not found, look into chanvars or global vars */
+	for (i = 0; !found && i < 3; i++) {
+		struct ast_var_t *variables;
+		if (!places[i])
+			continue;
+		AST_LIST_TRAVERSE(places[i], variables, entries) {
+			if (strcasecmp(ast_var_name(variables), var)==0) {
+				s = ast_var_value(variables);
+				found = 1;
+				break;
 			}
 		}
-		if (!(*ret)) {
-			/* Try globals */
-			AST_LIST_TRAVERSE(&globals,variables,entries) {
-#if 0
-				ast_log(LOG_WARNING,"Comparing variable '%s' with '%s'\n",var,ast_var_name(variables));
-#endif
-				if (strcasecmp(ast_var_name(variables),var)==0) {
-					const char *s = ast_var_value(variables);
-					if (s) {
-						ast_copy_string(workspace, s, workspacelen);
-						*ret = workspace;
-					}
-				}
-			}
-		}
 	}
-	if (deprecated) {
-		ast_log(LOG_WARNING, "${%s} is deprecated.  Please use ${%s} instead.\n", var, deprecated);
+	if (!s)
+		*ret = NULL;
+	else {
+		if (s != workspace)
+			ast_copy_string(workspace, s, workspacelen);
+		*ret = workspace;
+		if (need_substring)
+			*ret = substring(*ret, offset, length, workspace, workspacelen);
 	}
+		
+	if (deprecated)
+		ast_log(LOG_WARNING, "${%s} is deprecated.  Please use ${%s} instead.\n", var, deprecated);
 }
 
 /*! \brief CLI function to show installed custom functions 
