<p><a href="https://gerrit.asterisk.org/c/asterisk/+/11216">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/11216/1/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/11216/1/channels/chan_pjsip.c@1909">Patch Set #1, Line 1909:</a> <code style="font-family:monospace,monospace"> if (!(session->channel) || !(chan = ast_channel_ref(session->channel))) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Generally everything in regards to a session is expected to be queued up and executed on that sessio […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">After doing some more research, this callback function is serialized via the session's serializer. Therefore, there should be no need for additional locking or additional serialization. The associated code has been removed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1906">Patch Set #1, Line 1906:</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;"> if (!(session = (struct ast_sip_session *)pjsip_evsub_get_mod_data(sub, refer_callback_module.id))) {<br> return;<br> }<br> if (!(session->channel) || !(chan = ast_channel_ref(session->channel))) {<br> return;<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Yes, except if (!session->channel) is fine</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_channel_ref() call has been removed (see my note above)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1933">Patch Set #1, Line 1933:</a> <code style="font-family:monospace,monospace"> pjsip_evsub_terminate(sub, PJ_TRUE);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'd update the comment to make this clear.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated comment.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1980">Patch Set #1, Line 1980:</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;"> status = pjsip_evsub_initiate(sub, pjsip_get_subscribe_method(), 0, &tdata);<br> if (status == PJ_SUCCESS) {<br> pjsip_evsub_send_request(sub, tdata);<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I believe I was referring to the comment before the xfer_client_on_evsub_state function itself.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I beefed up the comment somewhat.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@2011">Patch Set #1, Line 2011:</a> <code style="font-family:monospace,monospace"> xfer_cb.on_evsub_state = &xfer_client_on_evsub_state;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Thinking about this some more - I think this now adds a hard dependency on res_pjsip_pubsub. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You are correct.<br>Added a dependency to the module's .requires.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@2020">Patch Set #1, Line 2020:</a> <code style="font-family:monospace,monospace"> pjsip_evsub_set_mod_data(sub, refer_callback_module.id, session);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What stops the session from going away in that case?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Addressed in my comments regarding serialization.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11216">change 11216</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/+/11216"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: If6c27c757c66f71e8b75e3fe49da53ebe62395dc </div>
<div style="display:none"> Gerrit-Change-Number: 11216 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Dan Cropp <dan@amtelco.com> </div>
<div style="display:none"> Gerrit-Reviewer: Dan Cropp <dan@amtelco.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: mattf <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 15 May 2019 16:59:17 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Comment-In-Reply-To: Dan Cropp <dan@amtelco.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>