<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7989">View Change</a></p><p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(12 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 style="white-space: pre-wrap; word-wrap: break-word;">These checks are still needed. They are not to prevent loading because a dependency is missing. They are to see if the optional functionality is available to use.</p><p style="white-space: pre-wrap; word-wrap: break-word;">On the other hand, ast_func_write() logs an error if you are trying to use the dialplan functions but didn't install the modules. Not loading the functionality but wanting to use it is more of a configuration error anyway.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We have certainly done our best now to ensure that they are loaded and running before app_confbridge starts and that they will remain available while app_confbridge is running.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/apps/app_meetme.c">File apps/app_meetme.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_meetme.c@3591">Patch Set #4, Line 3591:</a> <code style="font-family:monospace,monospace"> if (!ast_test_flag64(confflags, CONFFLAG_DONT_DENOISE)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here as in app_confbridge.</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 style="white-space: pre-wrap; word-wrap: break-word;">This shouldn't be needed if res_hep declines to load when it is disabled.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In res_hep.c:load_module()</p><p style="white-space: pre-wrap; word-wrap: break-word;">Replace:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (aco_process_config(&cfg_info, 0) == ACO_PROCESS_ERROR) {<br> goto error;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;"><br>With:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (aco_process_config(&cfg_info, 0) == ACO_PROCESS_ERROR<br> || !hepv3_is_loaded()) {<br> goto error;<br>}</pre></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_hep_rtcp.c">File res/res_hep_rtcp.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_rtcp.c@160">Patch Set #4, Line 160:</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 style="white-space: pre-wrap; word-wrap: break-word;">Same as res_hep_pjsip.c</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 style="white-space: pre-wrap; word-wrap: break-word;">Include res_pjsip_session too since this module used CHECK_PJSIP_SESSION_MODULE_LOADED(). Unless you are assuming the res_pjsip_session module has already been merged into res_pjsip.so.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_dtmf_info.c">File res/res_pjsip_dtmf_info.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_dtmf_info.c@178">Patch Set #4, Line 178:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_empty_info.c">File res/res_pjsip_empty_info.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_empty_info.c@84">Patch Set #4, Line 84:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_messaging.c">File res/res_pjsip_messaging.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_messaging.c@850">Patch Set #4, Line 850:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_nat.c">File res/res_pjsip_nat.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_nat.c@375">Patch Set #4, Line 375:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_one_touch_record_info.c">File res/res_pjsip_one_touch_record_info.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_one_touch_record_info.c@126">Patch Set #4, Line 126:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_path.c">File res/res_pjsip_path.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_path.c@259">Patch Set #4, Line 259:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_rfc3326.c">File res/res_pjsip_rfc3326.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_rfc3326.c@156">Patch Set #4, Line 156:</a> <code style="font-family:monospace,monospace"> .requires = "res_pjsip",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about res_pjsip_session as in res_pjsip_diversion.</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 17:28:25 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>