[Asterisk-code-review] loader: support for permanent dlopen() (...asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11217 )

Change subject: loader: support for permanent dlopen()
......................................................................


Patch Set 1: Code-Review-1

(11 comments)

https://gerrit.asterisk.org/#/c/11217/1/main/loader.c 
File main/loader.c:

https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@162 
PS1, Line 162: 	const char *name;
You could save an allocation by putting `char name[0];` last and including space for the string in the `info_list_obj` allocation.  Then you wouldn't need `info_list_obj_dtor`.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@179 
PS1, Line 179: 	new_entry = ao2_alloc(sizeof(*new_entry), info_list_obj_dtor);
Need to check for allocation failure.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@180 
PS1, Line 180: 	new_entry->name = ast_strdup(name);
If you do not take the `char name[0]` advice then you will need to verify that name was successfully allocated.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@285 
PS1, Line 285: #if defined(HAVE_PERMANENT_DLOPEN)
Can this be merged into the first `#if defined(HAVE_PERMANENT_DLOPEN)`?


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@286 
PS1, Line 286: static int info_list_obj_cmp_fn(void *obj1, void *obj2, int flags)
Please use `AO2_STRING_FIELD_CMP_FN(info_list_obj, name)` to construct this callback function.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@340 
PS1, Line 340: 			if (obj_tmp) {
You need to `ao2_ref(obj_tmp, -1)` otherwise you leak a reference.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@366 
PS1, Line 366: 			ast_module_unregister(obj_tmp->info);
ao2_ref(obj_tmp, -1); after this line.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@722 
PS1, Line 722: 	if (!info_list) {
This needs to be allocated near the start of load_modules, prevent startup if the allocation fails (by returning -1 from load_modules).

This container will leak at shutdown, I think that's unavoidable so please comment near the allocation must not be cleaned at shutdown.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@728 
PS1, Line 728: 		struct info_list_obj *obj_tmp = ao2_find(info_list, info->name,
Need to clean reference to obj_tmp.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@732 
PS1, Line 732: 			ao2_link(info_list, info_list_obj_alloc(info->name, info));
Need to check for allocation failure from `info_list_obj_alloc`.


https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@736 
PS1, Line 736: 	if (!info_list) {
Please just delete this.  Returning -1 from load_modules will be adequate / the right way to handle this situation.  We don't want to use `exit` directly from here as it skips running mandatory shutdown functions.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I86693a0ecf25d5ba81c73773a03df4abc3426875
Gerrit-Change-Number: 11217
Gerrit-PatchSet: 1
Gerrit-Owner: Sebastian Kemper <sebastian_ml at gmx.net>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Comment-Date: Wed, 03 Apr 2019 13:13:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190403/a77cbaee/attachment.html>