[asterisk-dev] [Code Review] 2544: Refactor CEL channel events on top of Stasis-Core

[Matt Jordan]

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



trunk/include/asterisk/stasis_channels.h
<https://reviewboard.asterisk.org/r/2544/#comment16900>

    We can't put bridge peer related information in channel snapshots.
    
    It is not up to a channel to know who it is bridged to. If you need that information, you need to store it yourself based on information from the bridging topic. You need to maintain who is in the bridges yourself, and provide that information accordingly. There is no guarantee that a channel is bridged with one other channel: it is equally plausible for the channel to be bridged with many other parties.
    
    This may make it 'easier' for CEL, but putting this information in an object that is shared by all consumers of Stasis-Core means this information should be useful to all consumers. And consumers of Stasis-Core should not be assuming that a channel has a single channel that is bridged to.



trunk/include/asterisk/stasis_channels.h
<https://reviewboard.asterisk.org/r/2544/#comment16901>

    This information should also not be part of a channel snapshot. You are provided this information when a dial end occurs.
    
    While I'm not a fan of having a separate list of variables for manager, I'm against have separate lists or fields of information for different consumers.



trunk/main/cel.c
<https://reviewboard.asterisk.org/r/2544/#comment16902>

    If you haven't already, I'd go ahead and make this a 'cel_state_router', create a topic for yourself, and forward the channel topic to it.
    
    You're going to need the bridging topic eventually.



trunk/main/cel.c
<https://reviewboard.asterisk.org/r/2544/#comment16903>

    Yuck.
    
    First, if you were going to keep this, use a scoped ao2 lock.
    
    Second, don't keep this. This is sadness.
    
    I'd go ahead and bite the bullet and turn CEL configuration over to the config framework. It isn't large - 4 variables? And then you no longer need this ugly reload lock shenaniganry.



trunk/main/cel.c
<https://reviewboard.asterisk.org/r/2544/#comment16904>

    If CEL isn't enabled, bail. Then you don't have to check it again.



trunk/main/cel.c
<https://reviewboard.asterisk.org/r/2544/#comment16905>

    I'd just use S_OR in the creation of the event, i.e.,
    
    EVENT_IE_CEL_USEREVENT_NAME, AST_EVENT_IE_PLTYPE_STR, S_OR(userdefevname, ""), ...



trunk/main/stasis_channels.c
<https://reviewboard.asterisk.org/r/2544/#comment16906>

    Remove all of this



trunk/main/stasis_channels.c
<https://reviewboard.asterisk.org/r/2544/#comment16907>

    Just as mentioning the usage of optimizing Local channels during internal transfer operations that does not involve dialplan execution will cause me to rage, so too does reaching across a bridge to grab the bridged channel cause Richard to burst into righteous anger.
    
    You don't know that you have a single bridged channel. This function is doomed to die.



trunk/main/stasis_channels.c
<https://reviewboard.asterisk.org/r/2544/#comment16908>

    Nope - there's a dial end message for a reason. Subscribe to it and use that for knowing what happens during a dial operation.


- Matt Jordan


On May 15, 2013, 9:01 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2544/
> -----------------------------------------------------------
> 
> (Updated May 15, 2013, 9:01 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21563
>     https://issues.asterisk.org/jira/browse/ASTERISK-21563
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This uses the channel state change events from Stasis-Core to determine when channel-related CEL events should be raised. Those refactored in this patch are:
> * AST_CEL_CHANNEL_START
> * AST_CEL_ANSWER
> * AST_CEL_APP_START
> * AST_CEL_APP_END
> * AST_CEL_HANGUP
> * AST_CEL_CHANNEL_END
> 
> Retirement of Linked IDs is also refactored.
> 
> 
> Diffs
> -----
> 
>   trunk/include/asterisk/stasis_channels.h 388883 
>   trunk/main/cel.c 388883 
>   trunk/main/channel.c 388883 
>   trunk/main/pbx.c 388883 
>   trunk/main/stasis_channels.c 388883 
> 
> Diff: https://reviewboard.asterisk.org/r/2544/diff/
> 
> 
> Testing
> -------
> 
> Ran these changes through all integration tests that check CEL records.
> 
> 
> Thanks,
> 
> opticron
> 
>

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