[Asterisk-code-review] Add new AMI action for app voicemail (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/7494 )

Change subject: Add new AMI action for app_voicemail
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c
File apps/app_voicemail.c:

https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c@13299
PS3, Line 13299: 	AST_LIST_LOCK(&users);
               : 
               : 	/* no any voicemail user */
               : 	if (AST_LIST_EMPTY(&users)) {
               : 		astman_send_ack(s, m, "There are no voicemail users currently defined.");
               : 		AST_LIST_UNLOCK(&users);
               : 		return RESULT_SUCCESS;
               : 	}
This can be removed (The locking/unlocking and empty check). The call to find_user locks the list before searching. If it is empty find_user will return NULL.


https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c@13308
PS3, Line 13308: 	// find user
This needs to be converted to 'C' style comments, /* */


https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c@13312
PS3, Line 13312: 		// could not find it
This too


https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c@13320
PS3, Line 13320: 	// get mailbox count
One more.


https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c@13321
PS3, Line 13321: 	inboxcount(vmu->mailbox, &new, &old);
               : 
               : 	astman_append(s,
Some of the code in this function is a copy of the code from manager_list_voicemail_users (at least the two items highlighted if not more).

I'd prefer this to be rewritten to avoid the code duplication. For instance a third function with the shared code that both can call.


https://gerrit.asterisk.org/#/c/7494/3/apps/app_voicemail.c@13396
PS3, Line 13396: 
vmu leaks memory. You need to call free_user(vmu) when done with it.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4eba1424a142e5fbd1d9fb1821a3fc1a1e238b7
Gerrit-Change-Number: 7494
Gerrit-PatchSet: 3
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 15:43:15 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171212/292e2ea7/attachment.html>