<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 2:<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/8166/2/include/asterisk/module.h">File include/asterisk/module.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/include/asterisk/module.h@68">Patch Set #2, Line 68:</a> <code style="font-family:monospace,monospace">enum ast_module_unload_result {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This enum is not needed.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8166/2/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/2/main/loader.c@1054">Patch Set #2, Line 1054:</a> <code style="font-family:monospace,monospace"> publish_unload_message(resource_name, AST_MODULE_UNLOAD_SUCCESS);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This could just directly use publish_load_message_type, passing "Success" for the status argument (see comment on that function about changing the argument).</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1246">Patch Set #2, Line 1246:</a> <code style="font-family:monospace,monospace">static void publish_load_message_type(const char* type, const char *name, int result)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think 'int result' should be replaced with 'const char *status'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For Load/Unload I'd prefer we create a string table. So instead of Status: -1 for Load we should have Status: Failure. This will keep the numeric values out of AMI/Stasis for the new events (we don't even use the numeric values in C, we use enum elements). Although I think the Reload event should be modified to use string based values for Status instead of numeric values that should not be done as part of this commit.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So publish_reload_message would have:<br>snprintf(res_buffer, sizeof(res_buffer), "%u", result);</p><p style="white-space: pre-wrap; word-wrap: break-word;">Then pass res_buffer to this function.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1254">Patch Set #2, Line 1254:</a> <code style="font-family:monospace,monospace"> if ((!type) || (!name)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This check is not needed, type can never be NULL because this function is static and the argument is never NULL. It's also impossible to reach this point with a NULL module name. If you want use:<br>ast_assert(type != NULL);<br>ast_assert(!ast_strlen_zero(name));</p><p style="white-space: pre-wrap; word-wrap: break-word;">This would add the check for dev-mode only.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1314">Patch Set #2, Line 1314:</a> <code style="font-family:monospace,monospace"> const char* module_name;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nit: space before the '*', not after:<br>const char *module_name;</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1316">Patch Set #2, Line 1316:</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;"> module_name = name;<br> if (!module_name) {<br> module_name = "All";<br> return;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please just use:<br>module_name = S_OR(name, "All");</p><p style="white-space: pre-wrap; word-wrap: break-word;">This way we use "All" if name points to a blank string ("", not NULL).</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd go as far as saying the module_name variable is unnecessary, could just pass S_OR(name, "All") directly to publish_load_message_type.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1530">Patch Set #2, Line 1530:</a> <code style="font-family:monospace,monospace">static enum ast_module_load_result load_resource(</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please revert this change, see comment below.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1557">Patch Set #2, Line 1557:</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_log(LOG_WARNING, "Failed to initialize dependency structures for module '%s'.\n", resource_name);<br> unload_dynamic_module(mod);<br><br> return required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can be replaced with 'goto prestart_error;' so the Load error will be reported when you add it to that label. Don't worry about changing the text of the LOG_WARNING, the one in prestart_error is good enough.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1579">Patch Set #2, Line 1579:</a> <code style="font-family:monospace,monospace"> if (notify) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please use the following instead:<br>if (ast_fully_booted && !ast_shutdown_final()) {</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1588">Patch Set #2, Line 1588:</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 should probably report this Load failure since errors are documented.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1595">Patch Set #2, Line 1595:</a> <code style="font-family:monospace,monospace"> res = load_resource(resource_name, 0, NULL, 0, 1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Revert.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1786">Patch Set #2, Line 1786:</a> <code style="font-family:monospace,monospace"> lres = load_resource(order->resource, !lasttry, &module_priorities, order->required, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Revert.</p></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: 2 </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: Thu, 08 Feb 2018 05:22:06 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>