[asterisk-dev] [Code Review]: If a CDR variable is set during a bridged call, make sure the values get copied over to the bridge CDR

[Terry Wilson]

> On Feb. 8, 2012, 2:31 p.m., Tilghman Lesher wrote:
> > If it were me, I'd add some extra checks to verify that the bridge CDR variables I'm copying into are blank.  Just that, blank, not "or unmodified from the original CDR".  Like you said, this violates the fundamental rule of never modifying CDR code, so you need to be extra-vigilant to ensure that nobody was counting upon values set _prior_ to the bridge being posted.
> > 
> > One efficient way to do this would be to create another version of copy_vars that appends to the _tail_ of the list, instead of the _head_.  That ensures that if there were any variables already set, they stay their original values, and you won't be breaking anybody's dialplan.

The bridge_cdr is never referenced inside the bridge loop, and is never attached to anything so it *can't* be modified anywhere else. It is either dup'd from the chan_cdr, or created if the chan_cdr doesn't exist (with a comment saying that it not existing should be rare/impossible). In any case, if there exists a chan_cdr, which is modifiable, then it seems like those values should just be copied over.


- Terry


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


On Feb. 7, 2012, 6:34 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1721/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2012, 6:34 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This completely violates my normal rule of "Don't touch CDR code, ever. No, really, I mean it. Never." So, with that said, I will be more than happy if someone tells me that this is a horrible idea and we shouldn't implement it.
> 
> Now, with *that* said, this is something that really should work. The problem is that before entering the bridge loop, we duplicate the channel cdr as the bridge cdr. When the CDR() application modifies the CDR during the bridge, it writes to the old channel cdr. This patch just copies the writable "read only" CDR variables (don't get me started) amaflags, accountcode, and userfield  and other CDR variables from the channel cdr to the bridge cdr before posting the bridge cdr.
> 
> I await someone who is smarter than I am to tell me why this is a bad idea, because I know that modifying CDR code is always a bad idea.
> 
> 
> This addresses bug ASTERISK-16990.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16990
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/features.c 354347 
> 
> Diff: https://reviewboard.asterisk.org/r/1721/diff
> 
> 
> Testing
> -------
> 
> I made a dynamic feature that calls Set(CDR(userfield)=stuff) and verified that "stuff" is actually written to the CDR userfield when it is used.
> 
> 
> Thanks,
> 
> Terry
> 
>

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