[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 4: Code-Review-1

(9 comments)

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

https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@98
PS4, Line 98: 					<para>The numeric status code denoting the success or failure
"The result of the load request."


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@104
PS4, Line 104: 						<enum name="Skip"><para>Module was skipped for some reason</para></enum>
             : 						<enum name="Priority"><para>Module is not loaded yet, but is added to priority list</para></enum>
Are these values even possible outside of startup?


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@119
PS4, Line 119: 					<para>The numeric status code denoting the success or failure
"The result of the unload request."


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@206
PS4, Line 206: 	const char *desc;
This field is never read so I think we can drop it.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1309
PS4, Line 1309: 	return NULL;
Let's return "Unknown".  In theory this should be unreachable but it will make sure we return something valid if we ever add a new value to the enum but forget to add it to the `load_results` array.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1313
PS4, Line 1313:  * \since 12
The "Load" event will not be part of 12 so this is incorrect.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1327
PS4, Line 1327:  * \since 12
Ditto for "Unload" event.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1345
PS4, Line 1345: 	S_OR(name, "All");
S_OR doesn't modify name, it returns name or "All".  I would just pass it directly to publish_load_message_type:

publish_load_message_type("Reload", S_OR(name, "All"), res_buffer);


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1605
PS4, Line 1605: 	return required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;
We need to replace this return statement with:

	res = required ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_DECLINE;
	if (ast_fully_booted && !ast_shutdown_final()) {
		publish_load_message(resource_name, res);
	}

	return res;



-- 
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: 4
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: Mon, 12 Feb 2018 09:35:12 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180212/2902919f/attachment.html>