<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/+/11628">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11628/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/11628/1//COMMIT_MSG@7">Patch Set #1, Line 7:</a> <code style="font-family:monospace,monospace">mwi_subscribe_replaces_unsolicited:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Commit title should be the name of the module plus a short description of the fix.<br>Something like:<br>res_pjsip_mwi: Fix multiple NOTIFYs with mwi_subscribe_replaces_unsolicited</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11628/1//COMMIT_MSG@9">Patch Set #1, Line 9:</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;">Fixes an bug whereby multiple NOTIFYs were replied back to endpoint when a<br>REGISTER was received. This occurred when parameter<br>mwi_subscribe_replaces_unsolicited is set to yes in pjsip.conf.<br>The underlying problem here is that the<br>endpoint_receives_unsolicited_mwi_for_mailbox() function in res/res_pjsip_mwi.c<br>is sort of an overloaded function. When invoked from mwi_validate_for_aor() you<br>DO want to have the subscribe_replaces_unsolicited() logic occur whereby it<br>unsubscribes from stasis. When invoked from<br>create_mwi_subscriptions_for_endpoint you DO NOT want to have the<br>subscribe_replaces_unsolicited logic occur.The function should have a parameter<br>added to it which determines whether the logic is invoked or not.<br>The bug itself occurs because there are multiple mwi_subscription structures<br>for the mailbox, the logic which sends the NOTIFY when a REGISTER occurs goes<br>through all looking for mwi_subscription structures with the AOR. It finds<br>these multiple structures, calculates message count on them (despite having<br>no stasis subscriptions), and sends a NOTIFY.<br>Note: This fix does not restore back unsolicted NOTIFYs in the event<br>all subscriptions on a given AOR go away. It was decided to create<br>a new issue for that.<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Adding a few blank lines will make the message easier to read.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c">File res/res_pjsip_mwi.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/11628/1/res/res_pjsip_mwi.c@a501">Patch Set #1, Line 501:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why was this blank line removed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@a713">Patch Set #1, Line 713:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why was this blank line removed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@683">Patch Set #1, Line 683:</a> <code style="font-family:monospace,monospace"> if (sub->is_subscribe_replaces_unsolicited) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this actually needed? Wouldn't "is_solicited" be set on this subscription?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@757">Patch Set #1, Line 757:</a> <code style="font-family:monospace,monospace">invokedByValidate</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We don't use camel case identifiers. How about "handle_replace_unsolicited" since that's what it does?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@761">Patch Set #1, Line 761:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">whitespace</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11628/1/res/res_pjsip_mwi.c@787">Patch Set #1, Line 787:</a> <code style="font-family:monospace,monospace"> ao2_cleanup(mwi_stasis);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this needs to be outside the "if invoked" otherwise mwi_stasis will be leaked.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11628">change 11628</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/+/11628"/><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: I8f77c33b9b4d374d510aa5ecd4f700a77107d8d4 </div>
<div style="display:none"> Gerrit-Change-Number: 11628 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Christian Savinovich <csavinovich@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 26 Jul 2019 13:21:58 +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>