[asterisk-dev] [Code Review]: Preserve user defined CDR information after a Local channel is masqueraded

[Matt Jordan]

> On June 14, 2012, 10:24 a.m., rmudgett wrote:
> > /branches/1.8/channels/chan_local.c, lines 577-587
> > <https://reviewboard.asterisk.org/r/1985/diff/2/?file=28868#file28868line577>
> >
> >     You should check if the cdr ptr is not NULL.
> >     If the owner cdr is not null and the _bridge cdr is null, move the owner cdr to the _bridge cdr.
> >     If the owner cdr is NULL do nothing.
> 
> Matt Jordan wrote:
>     As we discussed, this can probably just be replaced with a swap of the channel's cdrs.

...And, after some further testing, I don't think I'm right that this can be replaced.

Swapping the CDR structures preserves more then just the user defined fields.  If you swap the entire CDR structure, then things such as the channel name that is being masqueraded away is also preserved, which wouldn't be desired.

For example, assume that SIP/phone_b is being masqueraded into Local/1 at default;1.  With a CDR swap, this will result in the user defined fields being preserved, but you'll also end up with something like the following:

"","1","2","default","1","Local/1 at default-5c20;1","SIP/phone_a-00000001","Dial","SIP/phone_a","2012-06-14 19:16:32","2012-06-14 19:16:35","2012-06-14 19:16:36",4,1,"ANSWERED","DOCUMENTATION","1339701392.0","foo"

In this case, Local/1 at default-5c20;1 should have been replaced with the name of the SIP channel that masqueraded into the Local channel, e.g., SIP/phone_b-00000001.

Going back to your finding, then, this does need:

1) If the owner cdr pointer is NULL, do nothing
2) If the owner cdr pointer is not NULL, copy over the user defined data, destroy the _bridge->cdr variables, and replace them with the owner->cdr variables


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1985/#review6458
-----------------------------------------------------------


On June 11, 2012, 4:14 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1985/
> -----------------------------------------------------------
> 
> (Updated June 11, 2012, 4:14 p.m.)
> 
> 
> Review request for Asterisk Developers, Terry Wilson, Tilghman Lesher, and rmudgett.
> 
> 
> Summary
> -------
> 
> This patch preserves CDR information that was set on a Local channel before it is masqueraded into by another channel.  This includes accountcode, peeraccount, userfield, amaflags, and any variables that were set.
> 
> Consider a scenario in which a Local channel bridges two SIP channels (possibly the result of a channel originate Local/foo at default extension bar at default).  This would look something like the following:
> 
> SIP/A <--> Local;1 <> Local;2 <--> SIP/B
> 
> In this scenario, currently, the only opportunity for setting CDR information on the resulting call occurs on the Local channels - both SIP channels are created as the result of outbound calls.  As a result, it is desirable to have the CDR information that was set on the Local channel, i.e., Local;2, persist through the masquerade.  At the same time, most of the CDR information, e.g., caller ID information, channel information, etc., should be swapped with the channel being masqueraded away.
> 
> This patch does this by copying over the information in local_fixup - this is only an improvement that should be done with respect to local channels.  This should prevent impacting other masquerade operations (such as on SIP transfers).
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_local.c 368780 
> 
> Diff: https://reviewboard.asterisk.org/r/1985/diff
> 
> 
> Testing
> -------
> 
> Tested the following scenario:
> 
> exten => 100,1,NoOp()
>      same => n,Set(CDR(userfield)=foo)
>      same => n,Dial(SIP/A)
> 
> exten => 101,1,NoOp()
>      same => n,Set(CDR(userfield)=bar)
>      same => n,Dial(SIP/B)
> 
> Performing "channel originate Local/100 at default extension 101 at default" prior to the patch would produce the following CDR record:
> 
> "","","100","default","","Local/100 at default-5160,2","SIP/A-00000000","Dial","SIP/A","2012-06-11 19:10:40","2012-06-11 19:10:42","2012-06-11 19:10:42",2,0,"ANSWERED","DOCUMENTATION","1339441840.1","foo"
> "","100","101","default","100","SIP/A-00000000","SIP/B-00000001","Dial","SIP/B","2012-06-11 19:10:42","2012-06-11 19:10:43","2012-06-11 19:10:45",3,2,"ANSWERED","DOCUMENTATION","1339441840.2",""
> 
> Performing the same post patch produces the following CDR:
> 
> "","","100","default","","Local/100 at default-5160,2","SIP/A-00000000","Dial","SIP/A","2012-06-11 19:10:40","2012-06-11 19:10:42","2012-06-11 19:10:42",2,0,"ANSWERED","DOCUMENTATION","1339441840.1","foo"
> "","100","101","default","100","SIP/A-00000000","SIP/B-00000001","Dial","SIP/B","2012-06-11 19:10:42","2012-06-11 19:10:43","2012-06-11 19:10:45",3,2,"ANSWERED","DOCUMENTATION","1339441840.2","bar"
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120614/726e3030/attachment.htm>