[Asterisk-code-review] loader: Improve error handling. (asterisk[16])

[Richard Mudgett]

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

Change subject: loader: Improve error handling.
......................................................................


Patch Set 4: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/10330/4//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/10330/4//COMMIT_MSG@21
PS4, Line 21: Module load/start errors are delayed until the end the loader startup.
s/the end the/the end of/


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c
File main/loader.c:

https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@144
PS4, Line 144: #define STR_APPEND_TEXT(txt, str) \
             : 	ast_str_append(&str, 0, "%s%s", \
             : 		ast_str_strlen(str) > 0 ? ", " : "", \
             : 		txt)
str must be used as a pointer pointer
struct ast_str **str

ast_str_append(str, ...)
ast_str_strlen(*str)...


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@156
PS4, Line 156: #define module_load_error(fmt, ...) \
Looks like the previous function attribute was wrong:

static __attribute__((format(printf, 1, 0))) void module_load_error(const char *fmt, ...)

should have been:

static __attribute__((format(printf, 1, 2))) void module_load_error(const char *fmt, ...)


That and there was a nominal return path that didn't do a va_end().


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@903
PS4, Line 903: static int load_dlopen_missing(struct ast_str *list, struct ast_vector_string *deps)
list must be passed as a pointer pointer
struct ast_str **list

Otherwise when STR_APPEND_TEXT increases the list size the caller will have a stale pointer to list that it cannot free.


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@969
PS4, Line 969: 			resource_being_loaded = NULL;
             : 			module_load_error("Error loading module '%s': %s\n", resource_in, dlerror_msg);
             : 
             : 			goto error_return;
Don't we need to logged_dlclose() here if mod->lib?


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@981
PS4, Line 981: 			c = load_dlopen_missing(list, &mod->requires);
             : 			c += load_dlopen_missing(list, &mod->enhances);
             : #ifndef OPTIONAL_API
             : 			c += load_dlopen_missing(list, &mod->optional_modules);
pass list as
&list


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@1810
PS4, Line 1810: 		AST_VECTOR_CALLBACK_VOID(&localdeps, STR_APPEND_TEXT, printmissing);
Pass &printmissing

or assign *printmissing_ptr = printmissing earlier and use printmissing_ptr instead.


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@1863
PS4, Line 1863: 					"This is a bug in the module.\n", ast_module_name(mod));
unnecessary extra indention


https://gerrit.asterisk.org/#/c/10330/4/main/loader.c@1924
PS4, Line 1924: 				AST_VECTOR_CALLBACK_VOID(&localdeps, STR_APPEND_TEXT, printmissing);
Pass &printmissing



-- 
To view, visit https://gerrit.asterisk.org/10330
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-MessageType: comment
Gerrit-Change-Id: I046052c71331c556c09d39f47a3b92975f3e1758
Gerrit-Change-Number: 10330
Gerrit-PatchSet: 4
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 01 Oct 2018 21:43:09 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181001/1ca858cc/attachment.html>