[Asterisk-code-review] manager: Add AMI event Load/Unload (asterisk[master])

[Corey Farrell]

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

Change subject: manager: Add AMI event Load/Unload
......................................................................


Patch Set 2: Code-Review-1

(12 comments)

https://gerrit.asterisk.org/#/c/8166/2/include/asterisk/module.h
File include/asterisk/module.h:

https://gerrit.asterisk.org/#/c/8166/2/include/asterisk/module.h@68
PS2, Line 68: enum ast_module_unload_result {
This enum is not needed.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c
File main/loader.c:

https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1054
PS2, Line 1054: 		publish_unload_message(resource_name, AST_MODULE_UNLOAD_SUCCESS);
This could just directly use publish_load_message_type, passing "Success" for the status argument (see comment on that function about changing the argument).


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1246
PS2, Line 1246: static void publish_load_message_type(const char* type, const char *name, int result)
I think 'int result' should be replaced with 'const char *status'.

For Load/Unload I'd prefer we create a string table.  So instead of Status: -1 for Load we should have Status: Failure.  This will keep the numeric values out of AMI/Stasis for the new events (we don't even use the numeric values in C, we use enum elements).  Although I think the Reload event should be modified to use string based values for Status instead of numeric values that should not be done as part of this commit.

So publish_reload_message would have:
snprintf(res_buffer, sizeof(res_buffer), "%u", result);

Then pass res_buffer to this function.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1254
PS2, Line 1254: 	if ((!type) || (!name)) {
This check is not needed, type can never be NULL because this function is static and the argument is never NULL.  It's also impossible to reach this point with a NULL module name.  If you want use:
ast_assert(type != NULL);
ast_assert(!ast_strlen_zero(name));

This would add the check for dev-mode only.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1314
PS2, Line 1314: 	const char* module_name;
Nit: space before the '*', not after:
const char *module_name;


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1316
PS2, Line 1316: 	module_name = name;
              : 	if (!module_name) {
              : 		module_name = "All";
              : 		return;
              : 	}
Please just use:
module_name = S_OR(name, "All");

This way we use "All" if name points to a blank string ("", not NULL).

I'd go as far as saying the module_name variable is unnecessary, could just pass S_OR(name, "All") directly to publish_load_message_type.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1530
PS2, Line 1530: static enum ast_module_load_result load_resource(
Please revert this change, see comment below.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1557
PS2, Line 1557: 			ast_log(LOG_WARNING, "Failed to initialize dependency structures for module '%s'.\n", resource_name);
              : 			unload_dynamic_module(mod);
              : 
              : 			return required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;
This can be replaced with 'goto prestart_error;' so the Load error will be reported when you add it to that label.  Don't worry about changing the text of the LOG_WARNING, the one in prestart_error is good enough.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1579
PS2, Line 1579: 	if (notify) {
Please use the following instead:
if (ast_fully_booted && !ast_shutdown_final()) {


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1588
PS2, Line 1588: 	return required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;
We should probably report this Load failure since errors are documented.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1595
PS2, Line 1595: 	res = load_resource(resource_name, 0, NULL, 0, 1);
Revert.


https://gerrit.asterisk.org/#/c/8166/2/main/loader.c@1786
PS2, Line 1786: 			lres = load_resource(order->resource, !lasttry, &module_priorities, order->required, 0);
Revert.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib916c41eddd63651952998f2f49c57c42ef87a64
Gerrit-Change-Number: 8166
Gerrit-PatchSet: 2
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Thu, 08 Feb 2018 05:22:06 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180207/ed444e20/attachment.html>