<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7461">View Change</a></p><p>Patch set 2:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Not technically required on master but I'd prefer to see a test added for this new feature.  The testsuite already has a couple existing SIPnotify tests [1].</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://github.com/asterisk/testsuite/tree/master/tests/channels/SIP/ami/sip_notify</p><p>(7 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c">File channels/chan_sip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15593">Patch Set #2, Line 15593:</a> <code style="font-family:monospace,monospace">     struct ast_variable *vars = astman_get_variables_order(m, ORDER_NATURAL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"vars" is poorly handled in this function (even prior to your patch).</p><ul><li>At the beginning of the function we should check for vars == NULL, error out if found.</li><li>ast_variables_destroy(vars); should be called before every return, none of the error conditions have it.</li></ul></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15607">Patch Set #2, Line 15607:</a> <code style="font-family:monospace,monospace">        // check if Call-ID variable is set</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">/* Please use this style comments. */</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15608">Patch Set #2, Line 15608:</a> <code style="font-family:monospace,monospace">     for (var = vars; var; var = var->next) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Instead of the loop you should use astman_get_header to retrieve a specific variable.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15614">Patch Set #2, Line 15614:</a> <code style="font-family:monospace,monospace">                     p = ao2_find(dialogs, &tmp_dialog, OBJ_POINTER);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This would leak if someone sent multiple "Call-ID" headers.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15650">Patch Set #2, Line 15650:</a> <code style="font-family:monospace,monospace">      p->notify->headers = header = ast_variable_new("Subscription-State", "terminated", "");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Tab indent.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15661">Patch Set #2, Line 15661:</a> <code style="font-family:monospace,monospace">                     // do nothing here</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">/**/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7461/2/channels/chan_sip.c@15679">Patch Set #2, Line 15679:</a> <code style="font-family:monospace,monospace">               transmit_invite(p, SIP_NOTIFY, 0, 1, NULL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This branch leaks p.  Definitely need a dialog_unref, probably sip_scheddestroy.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7461">change 7461</a>. To unsubscribe, 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/7461"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5797ded4752acd966db6b13971284db684cc5ab4 </div>
<div style="display:none"> Gerrit-Change-Number: 7461 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Yasuhiko Kamata <yasuhiko.kamata@nxtg.co.jp> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 07 Dec 2017 04:13:44 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>