[Asterisk-code-review] loader: Retry dlopen when loading fails (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change.

Change subject: loader: Retry dlopen when loading fails
......................................................................


Patch Set 6:

(6 comments)

https://gerrit.asterisk.org/#/c/2073/6/main/loader.c
File main/loader.c:

Line 554: 	if (!(lib = dlopen(fn, RTLD_LAZY | RTLD_GLOBAL))) {
> Why this change to RTLD_GLOBAL? Should this be the case only for global typ
gtjoseph: Just the second one. I'm still curious about the change to RTLD_LOCAL vs RTLD_GLOBAL
<gtjoseph> oh gotcha.  want an explanation here or in the comments?
<kharwell> either is fine
<gtjoseph> of only the LOCAL flag is set, then any symbols exported by a library won't be available fir libraries loaded in the future
<gtjoseph> so in the first pass a lot will fail
<gtjoseph> and the loop will never pick them up.
<kharwell> my worry was (and I don't know if the scenario is even possible) that a module would load using the RTLD_GLOBAL flag and potentially overwrite a previously loaded module's symbols
<kharwell> it looked like that call would try to load the module. If it succeeded then it would unload it and do it again
<gtjoseph> it does and always did.
<kharwell> hrm now that I say that I guess if the scenario was possible either way it wouldn't matter. Since it gets unloaded
<gtjoseph> the problem is we don't know if a module exports global symbols unless we open it forst.
<kharwell> Is that what the first call to load is doing? Checking if contains global symbols?
<gtjoseph> the first time we open it we have to use GLOBAL.  The second time we open it, we can use LOCAL or GLOBAL depending on the module settings we got the first time
<kharwell> Interesting. I wonder how it was working before then since it was using LOCAL before
<gtjoseph> yep.  we can only tell if we actually call dlopen.
<gtjoseph> it wasn't.  hence the patch. :)
<kharwell> aah heh okay that makes sense
<gtjoseph> yep.  there was no handling of one global module requiring another.
<gtjoseph> because the first dlopen wasn't allowing the symbols to be exported.


Line 1206: 		enum ast_module_load_result lres;
         : 		/* Suppress log messages unless this is the last pass */
> Please add a blank line after the declaration.  It makes declarations stand
Done


Line 1250: 			case AST_MODULE_LOAD_SUCCESS:
> If it was successfully loaded shouldn't it be removed from the retry list?
Done


Line 1250: 			case AST_MODULE_LOAD_SUCCESS:
> Never mind I see now it is the same as the above loop. Supplying a heap mak
Done


Line 1283: 	while ((order = AST_LIST_REMOVE_HEAD(&load_retries, entry))) {
         : 		ast_free(order->resource);
         : 		ast_free(order);
         : 	}
         : 
> This code should be moved to the done label.  Otherwise, the list members c
Done


Line 1290: 		enum ast_module_load_result lres;
         : 		lres = start_resource(mod);
> Please add a blank line after the declaration.  It makes declarations stand
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iddae1d97cd2f00b94e61662447432765755f64bb
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes