<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8166">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>(9 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c">File main/loader.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@98">Patch Set #4, Line 98:</a> <code style="font-family:monospace,monospace"> <para>The numeric status code denoting the success or failure</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"The result of the load request."</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@104">Patch Set #4, Line 104:</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;"> <enum name="Skip"><para>Module was skipped for some reason</para></enum><br> <enum name="Priority"><para>Module is not loaded yet, but is added to priority list</para></enum><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are these values even possible outside of startup?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@119">Patch Set #4, Line 119:</a> <code style="font-family:monospace,monospace"> <para>The numeric status code denoting the success or failure</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"The result of the unload request."</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@206">Patch Set #4, Line 206:</a> <code style="font-family:monospace,monospace"> const char *desc;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This field is never read so I think we can drop it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1309">Patch Set #4, Line 1309:</a> <code style="font-family:monospace,monospace"> return NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's return "Unknown". In theory this should be unreachable but it will make sure we return something valid if we ever add a new value to the enum but forget to add it to the `load_results` array.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1313">Patch Set #4, Line 1313:</a> <code style="font-family:monospace,monospace"> * \since 12</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The "Load" event will not be part of 12 so this is incorrect.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1327">Patch Set #4, Line 1327:</a> <code style="font-family:monospace,monospace"> * \since 12</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ditto for "Unload" event.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1345">Patch Set #4, Line 1345:</a> <code style="font-family:monospace,monospace"> S_OR(name, "All");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">S_OR doesn't modify name, it returns name or "All". I would just pass it directly to publish_load_message_type:</p><p style="white-space: pre-wrap; word-wrap: break-word;">publish_load_message_type("Reload", S_OR(name, "All"), res_buffer);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1605">Patch Set #4, Line 1605:</a> <code style="font-family:monospace,monospace"> return required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We need to replace this return statement with:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> res = required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;<br> if (ast_fully_booted && !ast_shutdown_final()) {<br> publish_load_message(resource_name, res);<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> return res;</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8166">change 8166</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/8166"/><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: Ib916c41eddd63651952998f2f49c57c42ef87a64 </div>
<div style="display:none"> Gerrit-Change-Number: 8166 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </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 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 12 Feb 2018 09:35:12 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>