Summary:	ASTERISK-12014: [patch] Variadic expansion in MONITOR_FILENAME
Reporter:	Gabriel Ortiz Lour (elbriga)	Labels:
Date Opened:	2008-05-13 18:39:05	Date Closed:	2011-06-07 14:03:12
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_queue_v2.diff ( 1) app_queue.diff
Description:	patch for apps/app_queue.c This patch will allow the use of "^{AGENTID}" or "^{SIPID}" on the channel var "MONITOR_FILENAME", so it will store the monitored call with a filename that can include the Agent ID or SIP account that answered that particular call on a queue. By the way... this functionality has a bounty! Hope I can still claim it!
Comments:	By: Gabriel Ortiz Lour (elbriga) 2008-05-13 19:05:58 Oh, I forgot to mention that I'm getting the info from the chan->cdr structure, and I would like to know if that is the right place or I should get it from somewhere else By: Gabriel Ortiz Lour (elbriga) 2008-05-14 07:17:48 I was thinking that would be better to have only "^{AGENTID}" that would be translated to -> "Agent-11" or "SIP-test", simply changing the "/" for "-" on the channel's name. I'm working on it right now, and will send a new patch. By: Gabriel Ortiz Lour (elbriga) 2008-05-14 08:51:14 Duh! Now getting the name of the channel using.. duh! peer->name! Hahaha! I removed the idea of SIPID that sounded dumb... Now (the new diff file) it will create de channel var AGENTID, that is the name of the channel that awnsered the queue, changing "/" for "-", so it can be used on filenames. By: Mark Michelson (mmichelson) 2008-05-14 12:58:50 Thanks for the submission. I agree in principle to the idea, but the execution in the patch is a bit off. Here are some reasons. 1. Use of strcpy is discouraged. In this case, I know that the member's channel name has a maximum of 80 characters and that the vars variable has space for 2048, but this use depends heavily on knowing that information beforehand, and if either of those capacities were to change in a way that made this use of strcpy unsafe, this would be troublesome. Instead of using strcpy, I would recommend using either ast_copy_string() or ast_strdupa(). *** EDIT: Actually, on closer inspection, I was wrong about the channel name being limited to 80 characters. If I read the code correctly, the channel name length is not limited and so the use of strcpy() is just plain unsafe *** 2. Instead of iterating through the string searching for the '/' character, it is preferred that you use the strchr() function. 3. I don't like the fact that you are using the "vars" variable to hold the AGENTID value since that variable's purpose is for storing channel variables after calling vars2manager. I don't *think* this could cause a noticeable problem, but I think it's a bad coding practice to use the same variable for multiple purposes within a function since it makes the code more susceptible to bugs due to potential future changes. 4. The peer->name includes more than just "SIP/test" in it. It also includes the pointer address of the channel (e.g. "SIP/test-8eead45"). If your patch were applied, the AGENTID would be "SIP-test-8eead45" in the example I just gave. This may have been done on purpose, but your second note on this issue seems to suggest that it was not. Instead of using peer->name as the basis for creating the AGENTID, you could instead use member->interface, which is the same as the channel name, except that it does not have the pointer address appended (i.e. it would be "SIP/test" instead of "SIP/test-8eead45"). 5. Another qualm I have is with the name AGENTID and the fact that it is set unconditionally. There is a setting in queues.conf called "setinterfacevar" which controls whether queue member-related channel variables are set. I think the AGENTID should only be set if setinterfacevar is set for the queue. The reason I don't particularly like the name AGENTID is that it implies that the variable has something to do with the Agent channel type, when it actually is just the channel name of the queue member that answered. Unfortunately, I don't have a good alternative for the name. The best I can come up with is MEMBERINTERFACE_NOSLASH since setinterfacevar already sets a variable called MEMBERINTERFACE. The difference between the two is that the _NOSLASH version would have the / replaced with - as you have done in this patch. 6. Any new features need to have their patches created against SVN trunk, not the 1.4 branch. Please update your patch to be against trunk. By: Mark Michelson (mmichelson) 2008-06-17 10:52:41 There hasn't been any response in over a month, so I'm suspending this.