[asterisk-dev] [Code Review] 4213: Stasis: Fix StasisStart and StasisEnd ordering and missing events

[Matt Jordan]

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



branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/4213/#comment24339>

    I'd rename this - adjusting subscriptions is a bit vague. Here, you're specifically unsubscribing the application from the specified channel - which is independent of it being related to the Stasis end message.
    
    stasis_app_unsubscribe_channel maybe?



branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/4213/#comment24343>

    I think we have an error here, in that we don't clean up the implicit application subscription to the channel here.



branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/4213/#comment24342>

    I'm confused about where the channel being masqueraded out has its StasisEnd raised. Are you guaranteed that the stolen callback will be raised for the channel being replaced? If not, where does the replaced channel get its StasisEnd and its subscription modified?



branches/12/res/stasis/app.h
<https://reviewboard.asterisk.org/r/4213/#comment24340>

    Given the length of the names of the parameters and the comments, place comments on the preceeding line:
    
    /*! Channel that is entering Stasis() */
    struct ast_channel_snapshot *channel;
    
    etc.



branches/12/res/stasis/app.c
<https://reviewboard.asterisk.org/r/4213/#comment24341>

    I'm not sure why this entire block was removed. It still feels like we would need to clean up the implicit application subscriptions on replacement. If not, the review summary does not explain why this code was no longer necessary.


- Matt Jordan


On Nov. 25, 2014, 2:29 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4213/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2014, 2:29 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Bugs: ASTERISK-24537
>     https://issues.asterisk.org/jira/browse/ASTERISK-24537
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This corrects several bugs that currently exist in the 12/13/trunk stasis application code.
> 
> * Masquerades now swap channel topics appropriately
> * StasisStart and StasisEnd publishing is now properly restricted to controlling apps via app topics
> * StasisEnd published because of replacement via masquerade-in is now published as a byproduct of the associated StasisStart to avoid race conditions
> * StasisEnds now appear in some cases in which they were missing involving masquerades and bridge swaps into and out of Stasis()
> 
> 
> Diffs
> -----
> 
>   branches/12/res/stasis/app.c 428504 
>   branches/12/res/stasis/app.h 428504 
>   branches/12/res/res_stasis.c 428504 
>   branches/12/main/channel_internal_api.c 428504 
>   branches/12/main/channel.c 428504 
>   branches/12/include/asterisk/channel.h 428504 
> 
> Diff: https://reviewboard.asterisk.org/r/4213/diff/
> 
> 
> Testing
> -------
> 
> Ran existing ARI tests and a few new ones that are in-progress along with the scenarios that originally found these issues.
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141202/b1e1e534/attachment-0001.html>