----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3038/#review10299 -----------------------------------------------------------
I have a general thread-safety concern with this addition. In general, the ast_sip_session, pjsip_dialog, and pjsip_inv_session structures are only accessed from SIP servant threads. With this set of changes, you access session data from whichever thread is executing the CHANNEL() function. To ensure thread safety, I recommend pushing the entirety of the processing into the session's serializer using ast_sip_push_task_synchronous(). /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19678> These descriptions are inaccurate when used on outgoing channels. /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19679> These descriptions are inaccurate when used on outbound channels. /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19675> In channel_read_pjsip(), you check that the tech_pvt (called "channel" here) is non-NULL before proceeding. You should perform the same check here before assigning the value of pvt to channel->pvt. This change should be made in both channel_read_rtp() and channel_read_rtcp() /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19676> This warning should be refined to indicate that there is no RTP session for the type of media specified. ast_log(AST_LOG_WARNING, "Channel %s has no '%s' RTP session", ast_channel_name(chan), S_OR(field, "audio")); The same critique applies to a similar log message in channel_read_rtcp() /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19677> All of these snprintfs should just be ast_copy_string() /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19672> The preferred method of printing URIs is not to access their vptr directly but instead use pjsip_uri_print() This can be applied several places throughout this file. /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19673> Just use ast_copy_string() /branches/12/channels/pjsip/dialplan_functions.c <https://reviewboard.asterisk.org/r/3038/#comment19674> Print a warning about an unrecognized option here. - Mark Michelson On Dec. 1, 2013, 2:08 p.m., Matt Jordan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3038/ > ----------------------------------------------------------- > > (Updated Dec. 1, 2013, 2:08 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > This patch adds CHANNEL function support to chan_pjsip. Since things were > getting a bit large, all dialplan functions that were in chan_pjsip have also > been moved into their own file (dialplan_functions). > > Information that can be retrieved: > * rtp,type,[media_type] - Get RTP information, including media > source/destination addresses, whether or not the media is secure, etc. > * rtcp,statistic,[media_type] - Get RTCP statistic information > * endpoint - Get the name of the endpoint associated with this channel. Use > PJSIP_ENDPOINT to get more info. > * pjsip,type - Get signalling related information, including > source/destination addresses, URIs in the INVITE request, whether or not the > signalling is using a secure transport, etc. > > Note that after this patch is committed, we should go back through the > CHANNEL function documentation and move all of the channel technology > specific information into <info/> blocks, so that the documentation is > co-located with the channel drivers themselves. > > > Diffs > ----- > > /branches/12/res/res_pjsip_t38.c 403254 > /branches/12/include/asterisk/res_pjsip_session.h 403254 > /branches/12/funcs/func_channel.c 403254 > /branches/12/channels/pjsip/include/dialplan_functions.h PRE-CREATION > /branches/12/channels/pjsip/include/chan_pjsip.h PRE-CREATION > /branches/12/channels/pjsip/dialplan_functions.c PRE-CREATION > /branches/12/channels/chan_pjsip.c 403254 > /branches/12/channels/Makefile 403254 > > Diff: https://reviewboard.asterisk.org/r/3038/diff/ > > > Testing > ------- > > See https://reviewboard.asterisk.org/r/3037 > > > Thanks, > > Matt Jordan > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
