<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13501">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/chan_pjsip.c@2944">Patch Set #4, Line 2944:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "Unable to register PJSIP_ASYMMETRIC_RTP_CODEC dialplan function\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/PJSIP_ASYMMETRIC_RTP_CODEC/PJSIP_SESSION_ENDPOINT</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c">File channels/pjsip/dialplan_functions.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@85">Patch Set #4, Line 85:</a> <code style="font-family:monospace,monospace"> Set PJSIP endpoint options flag on a session.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove the word "flag" here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@89">Patch Set #4, Line 89:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <description><br> <para>When written, sets the endpoint option flag of a session. </para><br> <example title="To control asymmetric_rtp_codec"><br> exten => s,n,Set(PJSIP_SESSION_ENDPOINT(asymmetric_rtp_codec)=yes)<br> </example><br> </description><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make a note this should only be used for outgoing calls, used in a pre-dial handler.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@1354">Patch Set #4, Line 1354:</a> <code style="font-family:monospace,monospace"> data->session->endpoint = endpoint_copy;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How safe is this? Before this happens are there any instances of the endpoint being used, or pointed to by other things? If so it might be possible for those items to still point to the old endpoint object.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Worse case they didn't bump the ref when that happened, so potential for a crash. Best case they will be pointing to a "stale" object.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@1389">Patch Set #4, Line 1389:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updating a session's endpoint information only makes sense for outgoing calls, so I'd put a check in to make sure this is for outgoing only.</p><p style="white-space: pre-wrap; word-wrap: break-word;">You should be able to check the role (PJSIP_ROLE_UAC vs PJSIP_ROLE_UAS) on the session's pjsip session:</p><p style="white-space: pre-wrap; word-wrap: break-word;">session->inv_session->role;</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@1390">Patch Set #4, Line 1390:</a> <code style="font-family:monospace,monospace"> return ast_sip_push_task_wait_serializer(channel->session->serializer, session_endpoint_write_cb, &edata);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Before calling the write function you also might want to create a list of configuration options that shouldn't be updated (ones used in ast_sip_session_create_outgoing for instance), then check to make sure those are not being set.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/13501">change 13501</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/13501"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-Change-Id: I600d5ac1c6c2b47a7a0da6425aea559f1aa61c7c </div>
<div style="display:none"> Gerrit-Change-Number: 13501 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Salah Ahmed <txrubel@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 14 Jan 2020 19:14:40 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>