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

[sungtae kim]

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

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


Patch Set 6:

(9 comments)

Hi Corey,

Thank you for your kind review. :) I've fixed what you mentioned.

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 result of the load request.</para>
> "The result of the load request."
Fixed it.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@104
PS4, Line 104: 				</parameter>
             : 			</syntax>
> Are these values even possible outside of startup?
Priority: Nope. It sets only when the asterisk is booting.
Skip: Nope. It sets only when the asterisk is booting.

I removed those values. :)


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@119
PS4, Line 119: 					</enumlist>
> "The result of the unload request."
Fixed it.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@206
PS4, Line 206: 	{ AST_MODULE_LOAD_DECLINE, "Decline" },
> This field is never read so I think we can drop it.
Right, I just thought it might be useful in the future.
But, we can add this whenever it needed. 
So, I just removed it for now. :)


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1309
PS4, Line 1309: /*!
> Let's return "Unknown".  In theory this should be unreachable but it will m
Agree. Fixed it.


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1313
PS4, Line 1313: static void publish_load_message(const char *name, enum ast_module_load_result result)
> The "Load" event will not be part of 12 so this is incorrect.
Thanks!

I just removed \since tag, cause I'm not really sure which version of the Asterisk would be patched. But please feel free to let me know or add the tag :)


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1327
PS4, Line 1327: {
> Ditto for "Unload" event.
Just the same as above. :)


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1345
PS4, Line 1345: {
> S_OR doesn't modify name, it returns name or "All".  I would just pass it d
Thanks! I've fixed it. :)


https://gerrit.asterisk.org/#/c/8166/4/main/loader.c@1605
PS4, Line 1605: int ast_load_resource(const char *resource_name)
> We need to replace this return statement with:
You're right. I couldn't think about this case. 

I've fixed it.



-- 
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: 6
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 22:19:14 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180212/48b64aa5/attachment-0001.html>