<p style="white-space: pre-wrap; word-wrap: break-word;">Added some questions/responses.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If possible can you look and provide some feedback?<br>Once I know the right direction, I will modify, retest and resubmit.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11216">View Change</a></p><p>9 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;">Why do you bump the reference count here on the channel and release it later?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">To prevent the channel from being deleted by somebody else. Wasn't sure the intricacies of the PJSIP implementation. I wasn't confident that it was safe to assume the channel would remain while executing the function.<br>If there is no need to bump the reference count, I will modify.</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;">These days we do the assignment and checks separately, minor detail though.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Would the following be more appropriate?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!(session->channel) {<br> return;<br>}<br>chan = ast_channel_ref(session->channel);<br>if (!chan) {<br> return;<br>}</pre></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@1919">Patch Set #1, Line 1919:</a> <code style="font-family:monospace,monospace"> ast_debug(3, "Transfer accepted\n");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This doesn't really provide any information or details about which transfer was accepted.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Would it be better to remove the debug entirely or change it to ...</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_debug(3, “Transfer accepted on channel %s\n”, ast_channel_name(chan));</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;">Will this not happen automatically?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This code was based on the xfer_client_on_evsub_state() function in third-party/pjproject/source/pjsip/src/pjsua-lib/pjsua_call.c, which includes the call to pjsip_evsub_terminate(). I think the justification is that if the subscription is suppressed, the far end will not terminate it, so it will remain active until it times out. Since we know it isn’t needed, explicitly terminating it seems like a good idea.</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@1970">Patch Set #1, Line 1970:</a> <code style="font-family:monospace,monospace"> /* If the subscription has terminated, return AST_TRANSFER_SUCCESS for 2XX. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">A multiline comment is like: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks, I will fix</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@1976">Patch Set #1, Line 1976:</a> <code style="font-family:monospace,monospace"> /* If the subscription is not terminated, terminate it now. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Specify why you terminate the subscription.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Would something like the following be sufficient?</p><p style="white-space: pre-wrap; word-wrap: break-word;">/* If subscription not terminated and subscription is finished (status code >= 200) terminate it */</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 think at the beginning of the function a comment detailing precisely how this works and the intera […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">For clarification, do you want a comment before the status = pjsip_evsub_initiate line?</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@1987">Patch Set #1, Line 1987:</a> <code style="font-family:monospace,monospace"> ast_debug(3, "Transfer completed: %d %.*s (%s)\n", </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This doesn't give any detail about what channel was transferred or anything like that.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks, I will modify.</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 is the reference counting like for this? Is it always guaranteed that session will be valid whe […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The callback occurs as a result of a received SIP message. While the message is being processed I don’t think the session can be deleted. The callback checks for a valid session before doing anything else, so I think I’ve taken care of this concern.</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: 1 </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: Mon, 13 May 2019 16:20:15 +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"> Gerrit-MessageType: comment </div>