<p style="white-space: pre-wrap; word-wrap: break-word;">No problem! Just fixed it. :)</p><p><a href="https://gerrit.asterisk.org/10617">View Change</a></p><p>14 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This breaks the ABI of modules which provide session supplements. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thank you for reviewing. Btw, I don't get it what it suppose to be.. May I get some reference about what you saying it?</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_SELF, supplement)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should use AST_MODULE_SELF in 16+. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/7/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/7/include/asterisk/res_pjsip_session.h@585">Patch Set #7, Line 585:</a> <code style="font-family:monospace,monospace"> * \param module Referenced module(NULL safe)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Document that it is safe for it to be NULL</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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(void);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If we find a way to cherry-pick this to 13 I'd prefer this pass 'struct ast_module *module'. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/9/res/res_pjsip/pjsip_message_filter.c">File res/res_pjsip/pjsip_message_filter.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/9/res/res_pjsip/pjsip_message_filter.c@529">Patch Set #9, Line 529:</a> <code style="font-family:monospace,monospace">       ast_sip_session_register_supplement(&filter_session_supplement);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In Asterisk 16+ we can just use ast_sip_session_register_supplement(&filter_session_supplement) from […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ah! Now I've got it! No problem. :)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/7/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/7/res/res_pjsip/pjsip_session.c@41">Patch Set #7, 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;">     ast_assert(supplement != NULL);<br><br>     s<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should be an assert if you do want to add a check in.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip/pjsip_session.c@45">Patch Set #7, Line 45:</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->response_priority) {<br>              supplement->response_priority = AST_SIP_SESSION_BEFORE_MEDIA;<br>      }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">There's no need for this if, it can always be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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;">      ast_assert(supplement != NULL);<br><br>     s<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should simply use `ast_assert(supplement != NULL);`.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ast_module_ref checks that the module is not NULL so no need to also check here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/5/res/res_pjsip_caller_id.c">File res/res_pjsip_caller_id.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/5/res/res_pjsip_caller_id.c@782">Patch Set #5, Line 782:</a> <code style="font-family:monospace,monospace">   ast_sip_session_register_supplement(&caller_id_supplement);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Instead of having to do this for every module the ast_sip_session_register_supplement should become  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Agree. I've tried that at the beginning like this.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static struct ast_sip_session_supplement caller_id_supplement = {<br>      .module = ast_module_info->self,<br>   .method = "INVITE,UPDATE",<br>  .priority = AST_SIP_SUPPLEMENT_PRIORITY_CHANNEL - 1000,<br>       .incoming_request = caller_id_incoming_request,<br>       .incoming_response = caller_id_incoming_response,<br>     .outgoing_request = caller_id_outgoing_request,<br>       .outgoing_response = caller_id_outgoing_response,<br>};</pre><p style="white-space: pre-wrap; word-wrap: break-word;">But it caused compile error. Which is ast_module_info is not constant. How can I solve this..? Any idea?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Ah, MACRO! I've got it! You already mentioned it. Thanks! :)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10617/7/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/7/res/res_pjsip_session.c@2249">Patch Set #7, Line 2249:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Remove added red</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip_session.c@3320">Patch Set #7, Line 3320:</a> <code style="font-family:monospace,monospace">        AST_LIST_TRAVERSE(&session->supplements, iter, next) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think these debug messages would be useful after implementing this change.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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;">     AST_LIST_TRAVERSE(&session->supplements, iter, next) {<br>         if (iter->session_destroy) {<br>                       iter->session_destroy(session);<br>            }<br>     }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It's impossible to reach this point with a NULL session so please remove the NULL check. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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;">        }<br>}<br><br>static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata,<br>           enum ast_sip_session_response_priority response_p<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: 10 </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: Tue, 20 Nov 2018 23:42:37 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>