[Asterisk-code-review] ARI: Added new functionality to get all module information. (asterisk[13])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: ARI: Added new functionality to get all module information.
......................................................................


Patch Set 1:

(2 comments)

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

Line 1438: 	if (AST_DLLIST_LOCK(&module_list)) {
         : 		unlock = 0;
         : 	}
Since this routine isn't using TRYLOCK - which is good, since TRYLOCK around the traversal here is scary - you don't really need to test the return value here.

This should just be:

AST_DLLIST_LOCK(&module_list);

That will also allow you to remove the unlock local variable.


Line 1433: 	struct ast_module *cur;
         : 	int unlock = -1;
         : 	int total_mod_loaded = 0;
         : 	AST_LIST_HEAD_NOLOCK(, ast_module) alpha_module_list = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
         : 
         : 	if (AST_DLLIST_LOCK(&module_list)) {
         : 		unlock = 0;
         : 	}
         : 
         : 	AST_DLLIST_TRAVERSE(&module_list, cur, entry) {
         : 		AST_LIST_INSERT_SORTALPHA(&alpha_module_list, cur, list_entry, resource);
         : 	}
         : 
         : 	while ((cur = AST_LIST_REMOVE_HEAD(&alpha_module_list, list_entry))) {
         : 		total_mod_loaded += modentry(cur->resource, cur->info->description, cur->usecount,
         : 		        cur->flags.running? "Running" : "Not Running", like, cur->info->support_level, data);
         : 	}
         : 
         : 	if (unlock) {
         : 		AST_DLLIST_UNLOCK(&module_list);
         : 	}
         : 
         : 	return total_mod_loaded;
> I have a bit of a problem with this function's implementation. It basically
That's what I was hoping to do as well. However, that's problematic here, as the callback function prototype would also have to be updated. That means the 'common' routine - even if hidden in the implementation - would have four different parameters passed to it.

I advocated for the duplication approach, if for no other reason than the function is small. That felt better than two different callback function definitions being passed to a common function, but I could be wrong.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I63cbbf0ec0c3544cc45ed2a588dceabe91c5e0b0
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes