<p>Patch set 1:<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/+/15494">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15494/1//COMMIT_MSG">Commit Message:</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/+/15494/1//COMMIT_MSG@20">Patch Set #1, Line 20:</a> <code style="font-family:monospace,monospace">  the serializer but did already held the dialog lock</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/held/hold/</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15494/1/res/res_pjsip_refer.c">File res/res_pjsip_refer.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/+/15494/1/res/res_pjsip_refer.c@141">Patch Set #1, Line 141:</a> <code style="font-family:monospace,monospace">       /* Send a deferred initial 100 Trying SIP frag NOTIFY if we haven't already. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we have to move the unlock to before this comment.  I do not think we should be sending the NOTIFY message with the lock held.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The critical part of this function is determining if the subscription is still there.  It is also the critical part of the other function.  Thus the lock is the common mutex mechanism of the two.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Could do below in this function:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">lock dialog<br>if (!sub) {<br>   ast_debug()<br>   unlock dialog<br>   return 0<br>}</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (notification->state == terminated) {<br>   ast_debug()<br>   pjsip_evsub_set_mod_data(sub, refer_progress_module.id, NULL)<br>   /* This is for dropping the reference on the subscription *?<br>   ao2_cleanup(notification->progress)<br>   notification->progress->sub = NULL<br>}<br>unlock dialog</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15494/1/res/res_pjsip_refer.c@317">Patch Set #1, Line 317:</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;">      /* Since it was unlocked it is possible for this to have been removed already, so check again */<br>              if (pjsip_evsub_get_mod_data(sub, refer_progress_module.id)) {<br>                        pjsip_evsub_set_mod_data(sub, refer_progress_module.id, NULL);<br>                        ao2_cleanup(progress);<br>                }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since we are unlinking the subscription and did not release the lock when doing it, we do not need to test.  We can just do.</p><p style="white-space: pre-wrap; word-wrap: break-word;">progress->sub = NULL;<br>pjsip_evsub_set_mod_data(sub, refer_progress_module.id, NULL);<br>ao2_cleanup(progress);</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15494">change 15494</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/+/15494"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I97a8bb01771a3c85345649b8124507f7622a8480 </div>
<div style="display:none"> Gerrit-Change-Number: 15494 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 23 Feb 2021 22:23:21 +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>