<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7873">View Change</a></p><p>Patch set 9:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(19 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7873/9/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/7873/9/main/loader.c@317">Patch Set #9, Line 317:</a> <code style="font-family:monospace,monospace">Elements are only safe read while module_list remains locked.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/only safe read/safely read only/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@316">Patch Set #9, Line 316:</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;">Caller is responsible for initializing and freeing the vector on<br> * success.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove "on success" at the end of the first sentence here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The caller is entirely responsible for initializing and cleaning up the vector.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@336">Patch Set #9, Line 336:</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 (missing) {<br> /* We had an allocation error so the list was incomplete anyways. */<br> AST_VECTOR_FREE(missing);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Delete this. This is redundant and the doxygen note says it is the caller's responsibility anyway. The only caller that passes in a missing vector frees the vector when we return error.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@353">Patch Set #9, Line 353:</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;"> * An error from this function usually means a required module is not even<br> * loaded. This function is safe from infinite recursion, but dependency<br> * loops are not reported as an error from here. On success missingdeps<br> * will contain a list of every module that needs to be running before this<br> * module can start.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add to the end:</p><p style="white-space: pre-wrap; word-wrap: break-word;">missingdeps is sorted by load priority so any missing deps could be started if needed.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@364">Patch Set #9, Line 364:</a> <code style="font-family:monospace,monospace"> struct ast_module *tmp;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">tmp is a lousy name as it lacks any kind of algorithmic meaning and all variables are temporary.</p><p style="white-space: pre-wrap; word-wrap: break-word;">How about direct_dep or just dep.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@379">Patch Set #9, Line 379:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">unnecessary blank line</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@385">Patch Set #9, Line 385:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_REMOVE(&localdeps, i, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add comment:<br>Skip common dependency. We have already searched it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@398">Patch Set #9, Line 398:</a> <code style="font-family:monospace,monospace"> /* We've already confirmed tmp is loaded in the first loop. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/tmp/new-variable-name/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@478">Patch Set #9, Line 478:</a> <code style="font-family:monospace,monospace"> AST_LIST_HEAD_INIT(&mod->users);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should add requires, optional_modules, enhances, and reffed_deps explicit vector initialization here even if it's just a formality.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@498">Patch Set #9, Line 498:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_CALLBACK_VOID(&mod->reffed_deps, ast_module_unref);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add comment to emphasize:<br>Unref all module dependencies.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@524">Patch Set #9, Line 524:</a> <code style="font-family:monospace,monospace"> if (mod && !mod->usecount) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a comment:</p><p style="white-space: pre-wrap; word-wrap: break-word;">We are intentionally leaking mod here if usecount is not zero because other modules may still be referencing mod in their dependency list/vector. This can happen if we force the unloading of the module.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@781">Patch Set #9, Line 781:</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;"> * Or the lazy resolution of a global symbol (very likely, since that is<br> * how we load all of our modules that export global symbols).<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this part of the comment is no longer accurate and should be removed. RTLD_LAZY is no longer used to load modules for running.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Should be fixed for all branches.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@929">Patch Set #9, Line 929:</a> <code style="font-family:monospace,monospace"> } while (somethingchanged && !final);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We are currently staying in the loop only while things are changing. The "final" variable is meaningless because it can never get set. It has been this way since the original code was added by commit c0b3c923a46afc12837698a78a11b17a7fc384d8. The "final" variable and code controlled by it can be removed as dead code.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Not forcing the unload of modules with a use count appears to be intensional since the return value is true if the module list is not empty. The return value is used by the caller to switch to the fast shutdown mode (Commit f373de302032c13487cfcaa616fc070f10d68b57).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Should fix this in all branches.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1386">Patch Set #9, Line 1386:</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_VECTOR_FREE(&missing);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing a return here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">return AST_MODULE_LOAD_FAILURE;</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1440">Patch Set #9, Line 1440:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "Module '%s' already exists.\n", resource_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably should change this message for all branches to:</p><p style="white-space: pre-wrap; word-wrap: break-word;">"Module '%s' already loaded and running.\n"</p><p style="white-space: pre-wrap; word-wrap: break-word;">It is more descriptive so if you have preload=module and load=module in modules.conf you would have a better clue that you have tried to explicitly load the module twice. A change to load_modules() that I point out later avoids this warning situation.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For manually loading modules it is also more explicit.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1530">Patch Set #9, Line 1530:</a> <code style="font-family:monospace,monospace"> order->resource = ast_strdup(resource);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Check for NULL return here and cleanup to return failure.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Should fix this in all branches. Use of resource after we return does not expect it to be NULL.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1611">Patch Set #9, Line 1611:</a> <code style="font-family:monospace,monospace"> struct ast_module *tmp = AST_VECTOR_GET(&missingdeps, i);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How about renaming tmp to dep?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1634">Patch Set #9, Line 1634:</a> <code style="font-family:monospace,monospace"> /* We've failed to mod because of dependencies. Really abort? */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We can just delete the comment as it restates the error message.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1796">Patch Set #9, Line 1796:</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 (!strcasecmp(v->name, preload_only ? "preload" : "load")) {<br> add_to_load_order(v->value, &load_order, 0);<br> }<br> if (!strcasecmp(v->name, preload_only ? "preload-require" : "require")) {<br> /* Add the module to the list and make sure it's required */<br> add_to_load_order(v->value, &load_order, 1);<br> ast_debug(2, "Adding module to required list: %s (%s)\n", v->value, v->name);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We could avoid a load warning about the module already loaded and running at non-preload time if we do a find_resource() check before adding to the load order. Auto load already has this check.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be done for all branches.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7873">change 7873</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/7873"/><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: I9be08d1dd331aceadc1dcba00b804d71360b2fbb </div>
<div style="display:none"> Gerrit-Change-Number: 7873 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </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: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 17 Jan 2018 04:12:12 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>