<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7989">View Change</a></p><p>Patch set 4:</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'll address the res_pjsip_session requirements soon (or maybe in a separate commit). For now I don't really want to rebase everything else. This review only depends on the first review (7873) so once that is merged this can be rebased onto master instead of being in the series.</p><p>(3 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/apps/app_confbridge.c">File apps/app_confbridge.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7989/4/apps/app_confbridge.c@2389">Patch Set #4, Line 2389:</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 (ast_test_flag(&user.u_profile, USER_OPT_JITTERBUFFER)) {<br> ast_func_write(chan, "JITTERBUFFER(adaptive)", "default");<br> }<br><br> if (ast_test_flag(&user.u_profile, USER_OPT_DENOISE)) {<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These checks are still needed. They are not to prevent loading because a d</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This change was intentional, if the admin enables jitterbuffer or denoise and does not load the required modules we should print a warning.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I see this situation no differently than putting 'same => n,Set(JITTERBUFFER(adaptive)=default)' into dialplan. That doesn't cause a warning at pbx_config load, it produces a warning each time it is run until the admin loads func_jitterbuffer or changes the dialplan.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_hep_pjsip.c">File res/res_hep_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_hep_pjsip.c@235">Patch Set #4, Line 235:</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 (!hepv3_is_loaded()) {<br> ast_log(AST_LOG_WARNING, "res_hep is disabled; declining module load\n");<br> return AST_MODULE_LOAD_DECLINE;<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This shouldn't be needed if res_hep declines to load when it is disabled.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Personally I don't like the current way this is done but I made these minimum changes (removing the ast_module_check's).</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we should stop declining load and instead conditionally register/unregister logging_module. Also somehow monitor the enabled/disabled status of res_hep and manage logging_module based on that. I'm not sure if that's something that should be master only or all branches, but it seems like something that should be in a separate commit.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_diversion.c">File res/res_pjsip_diversion.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_diversion.c@429">Patch Set #4, Line 429:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Include res_pjsip_session too since this module used CHECK_PJSIP_SESSION_MO</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_sip_session_register_supplement and ast_sip_session_unregister_supplement are both in res_pjsip.so so my symbol usage scan did not find these dependencies (script attached to ASTERISK-26245).</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is one of the few exceptions where a module with global symbols is required even though we do not link to it.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7989">change 7989</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/7989"/><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: I332a6e8383d4c72c8e89d988a184ab8320c4872e </div>
<div style="display:none"> Gerrit-Change-Number: 7989 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </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-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 21 Jan 2018 18:34:41 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>