[asterisk-dev] [Code Review] Properly handle the linkedid for local channels and fix race condition for LINKEDID_END CEL event.

[rmudgett]

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



/branches/1.8/channels/chan_local.c
<https://reviewboard.asterisk.org/r/1895/#comment11232>

    The need for this change is subtle.  Please add a comment saying why the linkedid parameter is different.  When I first looked at the change, I wondered why it was needed from your description.  After digging into the ast_channel_alloc code the distinction is because linkedid could be NULL or an empty string.



/branches/1.8/main/cel.c
<https://reviewboard.asterisk.org/r/1895/#comment11235>

    ao2_ref returns with the ref count before it was called.  You could combine the two ao2_ref calls here and adjust the comparison count to 3.
    
    if (ao2_ref(lid, -1) == 3) {
    }



/branches/1.8/main/cel.c
<https://reviewboard.asterisk.org/r/1895/#comment11234>

    You need to check the ao2_link return for failure otherwise you have a ref leak since you are "keeping" the original ref.



/branches/1.8/main/cel.c
<https://reviewboard.asterisk.org/r/1895/#comment11233>

    Set appset to NULL like the other error exits.


- rmudgett


On May 2, 2012, 10:23 a.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1895/
> -----------------------------------------------------------
> 
> (Updated May 2, 2012, 10:23 a.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> While writing CEL tests, I noticed that if there is no requestor channel that the ;1 and ;2 sides of a local channel would have different linkedids when they should be the same. After fixing that issue, I found that Local channels were hung up so close together that the time between unlinking and destroying the channel would cause a race condition giving me 0, 1, or 2 LINKEDID_END events based on when they were unlinked.
> 
> This patch has the ;2 channel inherit the linkedid of the ;1 channel and fixes the race condition by no longer scanning the channel list for "other" channels with the same linkedid. Instead, cel.c has an ao2 container of linkedid strings and uses the refcount of the string as a counter of how many channels with the linkedid exist. Not only does this eliminate the race condition, but it also allows us to look up the linkedid by the hashed key instead of traversing the entire channel list.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_local.c 365004 
>   /branches/1.8/main/cel.c 365004 
> 
> Diff: https://reviewboard.asterisk.org/r/1895/diff
> 
> 
> Testing
> -------
> 
> Ran through the dialplan in the cdr/cdr_accountcode testsuite test and tested a single-channel call. Only one linkedid appeared, no refcount errors.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120502/0406fdcb/attachment-0001.htm>