[asterisk-bugs] [Asterisk 0012642]: [patch] Variadic expansion in MONITOR_FILENAME

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=12642 
====================================================================== 
Reported By:                elbriga
Assigned To:                putnopvut
====================================================================== 
Project:                    Asterisk
Issue ID:                   12642
Category:                   Applications/app_queue
Reproducibility:            N/A
Severity:                   feature
Priority:                   normal
Status:                     assigned
Asterisk Version:           1.4.19 
SVN Branch (only for SVN checkouts, not tarball releases):  trunk 
SVN Revision (number only!): 116087 
Disclaimer on File?:        N/A 
Request Review:              
====================================================================== 
Date Submitted:             05-13-2008 18:39 CDT
Last Modified:              05-14-2008 12:58 CDT
====================================================================== 
Summary:                    [patch] Variadic expansion in MONITOR_FILENAME
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!
====================================================================== 

---------------------------------------------------------------------- 
 putnopvut - 05-14-08 12:58  
---------------------------------------------------------------------- 
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().

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. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
05-14-08 12:58  putnopvut      Note Added: 0086853                          
======================================================================