[Asterisk-code-review] app voicemail: Remove need to subscribe to stasis (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10132 )

Change subject: app_voicemail: Remove need to subscribe to stasis
......................................................................


Patch Set 5: Code-Review-1

(17 comments)

https://gerrit.asterisk.org/#/c/10132/5//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/10132/5//COMMIT_MSG@24
PS5, Line 24: Also fixed cleanup in some of the uint tests that not only left
s/uint/unit/


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c
File apps/app_voicemail.c:

https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@1011
PS5, Line 1011: #define POLL_LIST_BUCKETS 193
You might want to consider 511 buckets to better accommodate larger installations.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@12193
PS5, Line 12193: 	size_t len = 0;
I think you should initialize to 1 for the terminator rather than possibly not ever adding room for it if you don't have a context string.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@12202
PS5, Line 12202: 		len += strlen(vmu->context) + 2; /* Allow for seperator and terminator */
Just add 1 for the separator and initialize len to 1 for the terminator to guarantee that there will be room for the terminator.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@13182
PS5, Line 13182: 	struct poll_state *poll_state;
               : 
               : 	AST_RWLIST_RDLOCK(&poll_list);
               : 	AST_RWLIST_TRAVERSE(&poll_list, poll_state, entry) {
               : 		if (!ast_strlen_zero(poll_state->mailbox)) {
               : 			imap_logout(poll_state->mailbox);
               : 		}
               : 	}
               : 	AST_RWLIST_UNLOCK(&poll_list);
Woops.  This needs to be converted to an ao2_callback.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@13222
PS5, Line 13222: 	const char *context = astman_get_header(m, "Context");
               : 	const char *mailbox = astman_get_header(m, "Mailbox");
This is now done for every poll_state in the container.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14138
PS5, Line 14138: 		if ((val = ast_variable_retrieve(cfg, "general", "tps_queue_high"))) {
               : 			ast_log(LOG_NOTICE, "Parameter tps_queue_high is obsolete and will be ignored\n");
               : 		}
               : 		if ((val = ast_variable_retrieve(cfg, "general", "tps_queue_low"))) {
               : 			ast_log(LOG_NOTICE, "Parameter tps_queue_low is obsolete and will be ignored\n");
               : 		}
Still need to update configs/samples/voicemail.conf.sample about these two options becoming obsolete or removing them from the file.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14495
PS5, Line 14495: 	memset(&svm, 0, sizeof(svm));
svm is now unused and can be deleted


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14496
PS5, Line 14496: 	if (!(vmu = find_or_create(testcontext, testmailbox))) {
We really should call find_or_create() with the users list locked:

AST_LIST_LOCK(&users);
vmu = find_or_create(...);
AST_LIST_UNLOCK(&users);
if (!vmu) {
}


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14618
PS5, Line 14618: 	free_user(vmu);
vmu was inserted into the users list so we cannot free it directly.  That's what load_config() is doing for us.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14672
PS5, Line 14672: 	if (!(vmu = find_or_create(testcontext, testmailbox))) {
Same


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14674
PS5, Line 14674: 		res = AST_TEST_NOT_RUN;
               : 		goto end;
revert.  Change not needed if find_or_create() fails.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14678
PS5, Line 14678: 	if (vmu != &vmus && !(vmu = find_user(&vmus, testcontext, testmailbox))) {
               : 		ast_test_status_update(test, "Cannot find vmu structure?!!\n");
               : 		res = AST_TEST_NOT_RUN;
               : 		goto end;
               : 	}
This code makes no sense now as we have just created the vmu.  Should delete the code.


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14733
PS5, Line 14733: end:
label no longer needed


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14734
PS5, Line 14734: 	free_user(vmu);
same


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14868
PS5, Line 14868: 	if (!(vmu = find_or_create(testcontext, testmailbox))) {
same


https://gerrit.asterisk.org/#/c/10132/5/apps/app_voicemail.c@14898
PS5, Line 14898: 	free_user(vmu);
same



-- 
To view, visit https://gerrit.asterisk.org/10132
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I5cceb737246949f9782955c64425b8bd25a9e9ee
Gerrit-Change-Number: 10132
Gerrit-PatchSet: 5
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 14 Sep 2018 19:07:32 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180914/4584d7d3/attachment.html>