<p>Patch set 8:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10617">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/8/include/asterisk/res_pjsip_session.h">File include/asterisk/res_pjsip_session.h:</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/10617/8/include/asterisk/res_pjsip_session.h@262">Patch Set #8, Line 262:</a> <code style="font-family:monospace,monospace"> struct ast_module *module;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This breaks the ABI of modules which provide session supplements. Not an issue for 'master' but if we want this in 13/16 we'll probably need to figure out how to deal with this.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10617/8/include/asterisk/res_pjsip_session.h@591">Patch Set #8, Line 591:</a> <code style="font-family:monospace,monospace"> ast_sip_session_register_supplement_with_module(ast_module_info->self, supplement)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should use AST_MODULE_SELF in 16+. ast_module_info->self is only available from the main module source file, AST_MODULE_SELF can be used by any source.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/include/res_pjsip_private.h">File res/res_pjsip/include/res_pjsip_private.h:</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/10617/8/res/res_pjsip/include/res_pjsip_private.h@224">Patch Set #8, Line 224:</a> <code style="font-family:monospace,monospace">int ast_res_pjsip_init_message_filter(const struct ast_module_info *module);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If we find a way to cherry-pick this to 13 I'd prefer this pass 'struct ast_module *module'. For 16+ it's unneeded as the function can just use the regular macro that will use AST_MODULE_SELF.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/pjsip_session.c">File res/res_pjsip/pjsip_session.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/10617/8/res/res_pjsip/pjsip_session.c@41">Patch Set #8, Line 41:</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(!supplement) {<br> ast_assert(0);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should simply use `ast_assert(supplement != NULL);`.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/pjsip_session.c@104">Patch Set #8, Line 104:</a> <code style="font-family:monospace,monospace"> if (copy->module) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_module_ref checks that the module is not NULL so no need to also check here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip_session.c">File res/res_pjsip_session.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/10617/8/res/res_pjsip_session.c@3331">Patch Set #8, Line 3331:</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) {<br> return;<br> }<br><br> ast_log(LOG_DEBUG, "Fired handle_session_destroy.\n");<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's impossible to reach this point with a NULL session so please remove the NULL check. Also I think this debug message should be removed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip_session.c@3347">Patch Set #8, Line 3347:</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) {<br> return;<br> }<br><br> ast_log(LOG_DEBUG, "Fired handle_session_end.\n");<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10617">change 10617</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/10617"/><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: I5b82be3a75d702cf1933d8d1417f44aa10ad1029 </div>
<div style="display:none"> Gerrit-Change-Number: 10617 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 19 Nov 2018 11:55:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>