[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/#review6269
-----------------------------------------------------------


It's getting closer. Not too much left to fix.

The one major thing to comment on is the parameters to the get_xxx_info_multi() functions. It seems odd to pass two ast_rtp_glue_results as output parameters since the result can be summed up in a single return. Rather than modifying the glue0_res and glue1_res from ast_rtp_instance_bridge(), have the function return a single ast_rtp_glue_result independent of the other results (glue2_res, perhaps). After returning, make sure that glue2_res isn't AST_RTP_GLUE_RESULT_FORBID. After that, all you would have to do is to insert glue2_res into the logical or statements where glue0_res and glue1_res are used, and this should work properly.


/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1924/#comment11656>

    This function essentially performs the same operation twice. You can factor the single operation into a function and just call that function twice with different arguments.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1924/#comment11646>

    Since directmediaha1 is actually p2's directmedia ACL, I'd suggest naming it something like p2_directmediaha to be more clear with it. I had thought you were applying the wrong ACL below until I looked more closely.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1924/#comment11642>

    This should be done in the RTP bridging code instead of farming it out to the callback like this. And for that matter, we could also optimize a bit by not doing any of this if either glue0_res or glue1_res is AST_RTP_GLUE_RESULT_LOCAL.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1924/#comment11649>

    As the function is currently written, you may end up overriding a previous AST_RTP_GLUE_RESULT_LOCAL with AST_RTP_GLUE_RESULT_REMOTE here. My suggestion at the top of the review will help with this.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1924/#comment11651>

    I was thinking about this a bit, and I think that if a directmedia ACL forbids direct media between two endpoints, then the result should be AST_RTP_GLUE_RESULT_LOCAL since a local RTP bridge should still be possible.


- Mark


On May 18, 2012, 12:21 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1924/
> -----------------------------------------------------------
> 
> (Updated May 18, 2012, 12:21 p.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 366775 
>   /trunk/include/asterisk/rtp_engine.h 366775 
>   /trunk/main/rtp_engine.c 366775 
> 
> 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/986a96c9/attachment.htm>