[asterisk-dev] [Code Review] chan_sip: Fix directmedia's use of ACL to limit remotebridging to certain host addresses for trunk

[Mark Michelson]

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


A couple of general comments

1. There has been no logic added to ast_rtp_instance_bridge() to ensure that get_rtp_info_multi is only called for channels of the same technology.
2. I don't see the value in calling get_rtp_info_multi() for each channel. Just call it once and do the necessary operations in both directions in that one callback.


/trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/1924/#comment11621>

    I would prefer this be approached differently. Rather than calling the multi version in place of the non-multi version, call the non-multi version first, then call the multi-version afterwards if applicable.
    
    I have two main reasons for this.
    
    1. Consider the case where a SIP channel and a non-SIP, but RTP-using, channel are being bridged. glue0 would have a multi function defined, but glue1 may not. Since both channels are not SIP channels, the multi function is not suitable to be called. However, the non-multi function should still be called in order to make sure that settings for that individual SIP peer allow for direct media.
    
    2. It allows for easier and more clear assignment of duties to the callbacks. This also saves you the copying and pasting you've done in chan_sip.



/trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/1924/#comment11618>

    No need for the ternary logic since the if already checks that glue0->get_vrtp_info_multi is non-NULL.
    
    The same goes for another usage below with video_glue1_res


- Mark


On May 18, 2012, 9:38 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1924/
> -----------------------------------------------------------
> 
> (Updated May 18, 2012, 9:38 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Related to https://reviewboard.asterisk.org/r/1899/ which has some locking changes that need to be made still...
> 
> This patch takes Mark's suggested approach for adding callbacks for the rtp_bridge function to be able to supply two channels for determining rtp glue stuff in cases where a channel driver needs data about both peers in order to determine what types of bridging are permissible.
> 
> 
> This addresses bug AST-876.
>     https://issues.asterisk.org/jira/browse/AST-876
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 366591 
>   /trunk/include/asterisk/rtp_engine.h 366591 
>   /trunk/main/rtp_engine.c 366591 
> 
> Diff: https://reviewboard.asterisk.org/r/1924/diff
> 
> 
> Testing
> -------
> 
> Similar to the testing done for the review 1899 version.  Calls were tested with and without directmediapermit/deny on both sides of a call with calls being started from both directions. Specific things tested include whether or not the host address lists were copied properly (because that was a rather substantial problem earlier on) and the results of ast_apply_ha in each case.
> 
> 
> Thanks,
> 
> jrose
> 
>

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