<p>Patch set 2:<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/10377">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10377/2/include/asterisk/module.h">File include/asterisk/module.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/10377/2/include/asterisk/module.h@75">Patch Set #2, Line 75:</a> <code style="font-family:monospace,monospace"> * system-wide stability. Declined modules will prevent other any</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/other any/any other/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10377/2/include/asterisk/module.h@91">Patch Set #2, Line 91:</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> * \brief Module load is successful but disabled.<br> *<br> * Returning this value has the same effect as calling<br> * ast_module_set_disabled(1) then returning AST_MODULE_LOAD_SUCCESS.<br> * It is important that a module not unload if returning LOAD_DISABLED.<br> * load_module will not be called again. It is expected that a disabled<br> * module can be enabled by modifying and reload the module's configuration.<br> *<br> * See \ref ast_module_set_disabled<br> */<br> AST_MODULE_LOAD_DISABLED = 4,<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What is the benefit of returning this at all?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Modules can (and already do) set themselves up in their own disabled state and return SUCCESS without this patch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">After further examination, this patch just does status reporting with things like the CLI "module show [like]" command. However, for the status reporting to be accurate it would require changing all modules to return DISABLED (or call ast_module_set_disabled()) when the user has configured them to be disabled. This is doable but will require diligence to prevent new modules from not reporting.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It would be more useful if the loader kept track of which dependencies were missing to report that in the CLI "module show [like]" command.</p><ul><li>Running - module is ready for use</li><li>Declined - Module itself declined to load</li><li>Missing dependencies - Module is missing dependencies or they are declined (may want to add which dependencies are preventing loading)</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">With the missing dependencies information we could make it so the dependent modules could attempt loading when the user fixes the dependent module's declined status.</p><p style="white-space: pre-wrap; word-wrap: break-word;">res_smdi - declined<br>app_voicemail - missing dependency (res_smdi)</p><p style="white-space: pre-wrap; word-wrap: break-word;">The user does a "module load res_smdi.so" after fixing the config error.<br>app_voicemail then automatically tries to load again if res_smdi's status becomes running.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10377/2/main/loader.c">File main/loader.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/10377/2/main/loader.c@1427">Patch Set #2, Line 1427:</a> <code style="font-family:monospace,monospace"> int oldValue;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">old_value<br>not camel case</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10377/2/main/loader.c@2339">Patch Set #2, Line 2339:</a> <code style="font-family:monospace,monospace"> return "Run Declined";</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just "Declined"</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10377">change 10377</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/10377"/><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: Ic44810ca6ce3deca30bceeb4eaa306d94f6b90e8 </div>
<div style="display:none"> Gerrit-Change-Number: 10377 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 04 Oct 2018 19:27:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>