Summary:	ASTERISK-01804: [post-1.0][patch] Adds the ability to copy a channel variable from any channel to the current channel
Reporter:	hwstar (hwstar)	Labels:
Date Opened:	2004-06-11 22:14:26	Date Closed:	2004-11-17 23:17:09.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) getchanvars.patch ( 1) new-var-functions.tar.gz
Description:	This patch allows any channel to get a copy of a channel variable from an arbitrary channel which is active. All channel variables are accessible including the builtin ones such as CALLERIDNUM. Source files affected: pbx.c The need for this was borne out of getting the caller ID information from a primary logical zap channel while the user was on the secondary logical zap channel. ****** ADDITIONAL INFORMATION ****** Example of use: GetChanVar(cid-of-last-call=CALLERIDNUM@Zap/1-1);
Comments:	By: Mark Spencer (markster) 2004-06-12 09:47:51 There should probably be a way to know if you were unable to find the referenced channel (e.g. exiting to n + 101 if it doesn't exist)? Also, if you're breaking things down into builtin_chanvars, why not just make a convenience routine that gives you any variable for a given channel rather than doing all that processing within the app itself?  Then that function might be used by things like AGI, so they have access to builtin vars like ${EXTEN}. By: Mark Spencer (markster) 2004-06-12 09:48:32 Actually, is there any reason we shouldn't just extend pbx_builtin_getvar_helper to have that functionality that occurs to you? By: hwstar (hwstar) 2004-06-12 13:01:44 Mark, I thought that I might break someone's dialplan with the special treatment of '@' if I included it in pbx_builtin_getvar_helper. If you think that the user's dialplans won't get screwed up using this nomenclature, then, it would be better to include this functionality in function useable by everyone else. Is there a better character to use than '@' in your opinion? As far as jumping to n + 101, I have comments from others (JT) that this is a bad idea from a user standpoint. If you want to go with the changes to pbx_builtin_getvar_helper, it would also be a bad idea to change how it works. It will be trivial to change the code to access the channel builtins from pbx_builtin_getvar_helper. Is this what you want to do? Steve. By: hwstar (hwstar) 2004-06-12 21:24:30 Also, should pbx_substitute_variables_temp be changed to provide '@' functionallity for extensions and expressions? Steve. By: hwstar (hwstar) 2004-06-13 18:17:44 After analyzing the code I found a shortcoming in the interface of pbx_builtin_getvar_helper. Ther way it's currently coded, it returns a pointer to an item in the varible list, which is ok when the data storage is pre-allocated. For the builtin channel variables, however, the values are built on the fly and there's no thread-safe way that I know of that will allow a pointer to be returned to a valid memory area, when the storage for th value is allocated in the called function. In other words, Everything goes out of scope when pbx_builtin_getvar_helper returns leaving you a pointer to undefined memory. What would have been a better interface? How about having the caller pass 2 additonal args:a pointer to some storage space, and the length of that storage space. It's probably too much hassle at this point to change the interface of pbx_builtin_getvar_helper, but a new helper function could be defined. Any thoughts on this? Steve. By: Mark Spencer (markster) 2004-06-13 18:24:05 My idea was to make getvar_helper know about builtin variables, not the "@" syntax.  The "@" syntax would have reentrancy issues potentially since getvar_helper is often called with chan->lock held, and will be trying to grab another chan->lock, potentially leading to a deadlock if we aren't careful. By: hwstar (hwstar) 2004-06-15 11:20:19 Mark, Even if the @ syntax is removed and I only implement the builtin variable feature, there still remains the problem of having a per-thread allocated storage area for the builtin's value. The current implementation pbx_builtin_getvar_helper function function just returns a pointer to the actual location where the non-builtin variable is stored in the varshead linked list. In the case of the builtin variables, the storage isn't pre-allocated, this means that either a per-channel linked list of builtin variables has to be created at initialization time, or the caller to pbx_builtin_getvar_helper has to supply a pointer and a length to a storage area for the return value. Which is best? Steve. By: Mark Spencer (markster) 2004-06-18 09:21:46 Hrm...  Good question.  I don't really like the name "pbx_builtin_getvar_helper" anyway.... Lets just create: /* Set a variable */ int ast_variable_set(struct ast_channel *chan, const char *variable, const char *value); /* Return variable, stored in buffer space provided */ char *ast_variable_get(struct ast_channel *chan, const char *variable, char *value, int maxlen); /* Return "strdup'd" version of value */ char *ast_variable_get_alloc(struct ast_channel *chan, const char *variable); And we'll leave the pbx_builtin_getvar_helper and pbx_bulitin_setvar_helper for the applications that truly need it.  The risk is that if you do pbx_builtin_getvar_helper on a channel you don't own, then there is a risk the channel will disappear before you look at the value. By: hwstar (hwstar) 2004-06-19 22:32:00 Mark, Sounds good to me. If we leave pbx_builtin_getvar_helper as is, it will not be able to return the builtin channel variables because of the problem I mentioned in my previous message. If this is not what you had in mind, add a bugnote. Otherwise, I'll make these changes, and hopefully upload a new patch in the next few days. Steve. By: Mark Spencer (markster) 2004-06-19 23:58:56 It's fine just as it is.  We'll have to find the places that are appropriate to change to the new functions. By: hwstar (hwstar) 2004-06-20 22:51:17 Mark, Do you think it's too dangerous to have ast_variable_set set builtin channel variables, or was this what you had in mind? Also, we ought to change the description of this bug to "[patch] Improved methods to set and get PBX variables" or something similar. Steve. By: Mark Spencer (markster) 2004-06-21 00:46:15 Yes, it's definitely not a good idea for variable_set to work on builtins. By: Mark Spencer (markster) 2004-06-26 17:14:27 How is it coming? By: hwstar (hwstar) 2004-06-28 11:01:32 Mark, I've been busy helping with app_rpt.c (Someone we both mutually know) Back to this bug: The ast_variable_get function currently has a problem  where it is returning garbage strings when builtin variables are requested. I'll need about week to sort it out. Steve. edited on: 06-28-04 10:47 By: hwstar (hwstar) 2004-07-03 15:57:58 Problem with ast_variable_get fixed.  New tar file uploaded with new patch and a test bench app to test out the new functions. The app should only be used for testing and not included when the patch is merged into CVS. By: Olle Johansson (oej) 2004-07-16 15:54:54 Housekeeping wonders if testing is successful? Where are we with this patch? Does it still apply to CVS? Do we need more testers? By: hwstar (hwstar) 2004-07-16 16:19:46 Your answers: aa. It tested out just fine on my system with the testbench included. bb. I'd like to see other confirm the test results on their systems. cc. Yes, I just applied the patch against a fresh checkout from the top-o-the-heap -- 7/16/04 14:00 dd. See bb. Steve. edited on: 07-16-04 16:04 By: Clod Patry (junky) 2004-08-27 00:43:47 I've just found something pretty interesting. we can overwrite our global variables in the extensions.conf. let me give ya a small example: [init_agi] exten => s,1,setVar(EPOCH=${EPOCH}); exten => s,2,DeadAgi(init_agi.agi); and into ur AGI, ya can now do a "GET VARIABLE EPOCH". cause i've just overwritten the ${EPOCH} glob.var to the ${EPOCH} chan.var. Maybe that would give an idea to someone to change something into the pbx_builtin_getvar_helper function. By: Clod Patry (junky) 2004-10-25 23:26:17 anyone working on this problem/patch? Thanks By: hwstar (hwstar) 2004-10-26 12:27:15 junky- Correct me if I'm wrong, but your observation doesn't have anything to do with the functions I wrote: ast_variable_set(), ast_variable_get() and ast_variable_get_alloc(). The issue you are having is with the setVar() CLI command. I'd like to suggest that we eiter a) break your issue out as a separate bug because if you follow the thread, the original intent has changed, or b) break out the additional functions as a separate bug to avoid confusion. Steve. By: Brian West (bkw918) 2004-11-08 09:32:11.000-0600 Junky EPOCH is already a var. bkw By: Mark Spencer (markster) 2004-11-16 23:53:07.000-0600 Added ImportVar app, should do it.