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

[jrose]

> On May 18, 2012, 11:07 a.m., Mark Michelson wrote:
> > 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.

Somehow while working on this I got into my head that we wouldn't be able to enter ast_rtp_instance_bridge without first having checked the channel techs were the same.  Looking this over again today I'm not sure how I ever thought that, so yeah... it sort of messed with my way of thinking from here.


> On May 18, 2012, 11:07 a.m., Mark Michelson wrote:
> > /trunk/main/rtp_engine.c, lines 1336-1340
> > <https://reviewboard.asterisk.org/r/1924/diff/1/?file=28013#file28013line1336>
> >
> >     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.

Alright, but this is going to mean that the vinstance arguments coming from ast_rtp_instance_bridge are going to need to be cleared when going into the multi functions.


> On May 18, 2012, 11:07 a.m., Mark Michelson wrote:
> > /trunk/main/rtp_engine.c, lines 1342-1344
> > <https://reviewboard.asterisk.org/r/1924/diff/1/?file=28013#file28013line1342>
> >
> >     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

Alllllllllrighty then.  New patch incoming.


- jrose


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


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/83216892/attachment.htm>