[Home]

Summary:ASTERISK-03012: [PATCH] don't clone app_groupcount variables in ast_do_masquerade
Reporter:Kevin P. Fleming (kpfleming)Labels:
Date Opened:2004-12-15 18:28:03.000-0600Date Closed:2008-01-15 15:19:22.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) masquerade_trim_groups_rev6.diff.txt
Description:I have been fighting a problem for a while on our systems, that goes like this:

When our SIP users accept or place calls, we assign groups to those channels (via SetGroup) to be able to track usage and also to limit incoming calls for users that want them. However, when a SIP user does a supervised transfer of a call, we found that the resulting channel (the replacement peer) ended up with _two_ groups assigned; the one they should have (as the target of a call), and the one that the transferer should have (as the originator of a call). This is wrong; once the transferer is out of the call, their group assignments should disappear. In addition, this problem causes multiple group assignments for the same category to be present, which is something that app_groupcount is not prepared for (since it will never do that on its own).

The attached patch fixes this behavior; when ast_do_masquerade is about to copy the variables from the clone channel into the original channel, it removes any app_groupcount-related variables from the original channel first. This has solved the problem for us; when the transfer is complete, the two remaining call legs have the proper group assignments.

I'm not really thrilled about making channel.c aware of app_groupcount-related internals like this, but I can't think of a simple solution that is any cleaner. If anyone can suggest one, I'm open to trying it out.

****** ADDITIONAL INFORMATION ******

Disclaimer is on file.
Comments:By: Kevin P. Fleming (kpfleming) 2004-12-16 20:22:11.000-0600

No comments? Maybe I shouldn't have marked this one as a "tweak" :-)
By: Clod Patry (junky) 2004-12-16 21:54:13.000-0600

the patch applied successfully for me.
Like im more playing around Zap, i didnt't test that patch yet.

And what do you think about adding comments for each new function created?
that would be a much easier for new developpers when they read all this code, what's your feeling about this?
By: Kevin P. Fleming (kpfleming) 2004-12-18 21:05:33.000-0600

rediffed against current CVS
added comments documenting new clone_variables function
converted remaining code in clone_variables to use list macros
add comment documenting locking assumption in ast_do_masquerade
By: Kevin P. Fleming (kpfleming) 2004-12-22 23:13:09.000-0600

This patch is currently broken, and needs some re-work. I probably will not get time to do that work until after the weekend. I will post a new patch when it works correctly and doesn't randomly segfault :-)
By: Brian West (bkw918) 2004-12-23 00:31:26.000-0600

Can you explain this a bit more since the GROUP is really just a channel var called "GROUP" how can you have TWO on one channel?  Unless each leg has its own group... if thats the case you could use chan_local to take care of this.

bkw
By: Kevin P. Fleming (kpfleming) 2004-12-23 08:53:04.000-0600

Groups can be set for categories, using

 SetGroup(foo@bar)

This creates a channel variable called GROUP_bar with a value of "foo".

And yes, each leg (every channel) can have its own group assignments; if you want to actually control your channel usage, it's the only way it can be done properly. It doesn't require Local channels at all; for example, take a look at the current Dial() app docs. If you set OUTBOUND_GROUP there, then that group will be assigned to the channels created by the dial app.
By: Kevin P. Fleming (kpfleming) 2004-12-23 13:40:53.000-0600

New version, rediffed against current CVS, but also needs the patches from bugs 3139 and 3140 to be applied first. No known problems left, it's safe to apply now :-)
By: Mark Spencer (markster) 2004-12-23 17:29:23.000-0600

You still have "XXX" things in there, are you really satisfied with this as-is?
By: Kevin P. Fleming (kpfleming) 2004-12-23 17:45:22.000-0600

Actually, the XXX thing is left over from the code I pulled out of ast_do_masquerade into clone_variables; I can't answer whether all this variable copying is actually needed or not, I can only say that copying the GROUP related variables is not needed.
By: Mark Spencer (markster) 2004-12-23 17:54:23.000-0600

Can you find me on IRC?
By: Kevin P. Fleming (kpfleming) 2004-12-24 09:51:29.000-0600

New version, relative to rev2 patch from bug 3140. No longer dependent on bug 3139, as that has been merged in CVS already.
By: Kevin P. Fleming (kpfleming) 2004-12-27 15:04:11.000-0600

Yet another rev... updated to current CVS, and relative to bug 3166 (which simplifies list macros).
By: Mark Spencer (markster) 2005-01-01 20:01:39.000-0600

Added to CVS, thanks!
By: Russell Bryant (russell) 2005-01-04 00:11:50.000-0600

Hm.  I would consider this a bug fix, but since it is dependent on other patches in head, it may just have to live as a known bug for 1.0.

Feel free to contact me on IRC if you have any thoughts ...
By: Digium Subversion (svnbot) 2008-01-15 15:19:22.000-0600

Repository: asterisk
Revision: 4633

U   trunk/channel.c

------------------------------------------------------------------------
r4633 | markster | 2008-01-15 15:19:22 -0600 (Tue, 15 Jan 2008) | 2 lines

Handle group memberships better with masquerade (bug ASTERISK-3012)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=4633