[Asterisk-code-review] loader: Process module dependencies. (asterisk[master])

Tue Jan 16 22:12:12 CST 2018

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/7873 )

Change subject: loader: Process module dependencies.
......................................................................

Patch Set 9: Code-Review-1

(19 comments)

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c
File main/loader.c:

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@317
PS9, Line 317: Elements are only safe read while module_list remains locked.
s/only safe read/safely read only/

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@316
PS9, Line 316: Caller is responsible for initializing and freeing the vector on
             :  *       success.
Remove "on success" at the end of the first sentence here.

The caller is entirely responsible for initializing and cleaning up the vector.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@336
PS9, Line 336: 	if (missing) {
             : 		/* We had an allocation error so the list was incomplete anyways. */
             : 		AST_VECTOR_FREE(missing);
             : 	}
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.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@353
PS9, Line 353:  * An error from this function usually means a required module is not even
             :  * loaded.  This function is safe from infinite recursion, but dependency
             :  * loops are not reported as an error from here.  On success missingdeps
             :  * will contain a list of every module that needs to be running before this
             :  * module can start.
Add to the end:

missingdeps is sorted by load priority so any missing deps could be started if needed.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@364
PS9, Line 364: 	struct ast_module *tmp;
tmp is a lousy name as it lacks any kind of algorithmic meaning and all variables are temporary.

How about direct_dep or just dep.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@379
PS9, Line 379: 
unnecessary blank line

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@385
PS9, Line 385: 			AST_VECTOR_REMOVE(&localdeps, i, 0);
Add comment:
Skip common dependency.  We have already searched it.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@398
PS9, Line 398: 		/* We've already confirmed tmp is loaded in the first loop. */
s/tmp/new-variable-name/

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@478
PS9, Line 478: 	AST_LIST_HEAD_INIT(&mod->users);
Should add requires, optional_modules, enhances, and reffed_deps explicit vector initialization here even if it's just a formality.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@498
PS9, Line 498: 	AST_VECTOR_CALLBACK_VOID(&mod->reffed_deps, ast_module_unref);
Add comment to emphasize:
Unref all module dependencies.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@524
PS9, Line 524: 	if (mod && !mod->usecount) {
Add a comment:

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.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@781
PS9, Line 781: 	 * Or the lazy resolution of a global symbol (very likely, since that is
             : 	 * how we load all of our modules that export global symbols).
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.

Should be fixed for all branches.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@929
PS9, Line 929: 	} while (somethingchanged && !final);
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.

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).

Should fix this in all branches.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1386
PS9, Line 1386: 		AST_VECTOR_FREE(&missing);
              : 	}
Missing a return here.

return AST_MODULE_LOAD_FAILURE;

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1440
PS9, Line 1440: 			ast_log(LOG_WARNING, "Module '%s' already exists.\n", resource_name);
Probably should change this message for all branches to:

"Module '%s' already loaded and running.\n"

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.

For manually loading modules it is also more explicit.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1530
PS9, Line 1530: 	order->resource = ast_strdup(resource);
Check for NULL return here and cleanup to return failure.

Should fix this in all branches.  Use of resource after we return does not expect it to be NULL.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1611
PS9, Line 1611: 				struct ast_module *tmp = AST_VECTOR_GET(&missingdeps, i);
How about renaming tmp to dep?

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1634
PS9, Line 1634: 			/* We've failed to mod because of dependencies.  Really abort? */
We can just delete the comment as it restates the error message.

https://gerrit.asterisk.org/#/c/7873/9/main/loader.c@1796
PS9, Line 1796: 		if (!strcasecmp(v->name, preload_only ? "preload" : "load")) {
              : 			add_to_load_order(v->value, &load_order, 0);
              : 		}
              : 		if (!strcasecmp(v->name, preload_only ? "preload-require" : "require")) {
              : 			/* Add the module to the list and make sure it's required */
              : 			add_to_load_order(v->value, &load_order, 1);
              : 			ast_debug(2, "Adding module to required list: %s (%s)\n", v->value, v->name);
              : 		}
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.

This should be done for all branches.

-- 
To view, visit https://gerrit.asterisk.org/7873
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9be08d1dd331aceadc1dcba00b804d71360b2fbb
Gerrit-Change-Number: 7873
Gerrit-PatchSet: 9
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 17 Jan 2018 04:12:12 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180116/ca74c953/attachment.html>