Summary:	ASTERISK-04129: [patch] Define maximum number of messages per folder in voicemail.conf
Reporter:	ssljivic (ssljivic)	Labels:
Date Opened:	2005-05-10 09:45:28	Date Closed:	2008-01-15 15:40:58.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) vm-maxmsg-option-1.2.patch
Description:	Hi, I'm submitting the patch that adds one more option in voicemail.conf. This is option maxmsg that defines maximum number of messages per voice mailbox folder. Currently this number (100) is hard coded in the voicemail.c. The patch was made against CVS-HEAD-05/10/05-13:26:31. The disclaimer is on file. Regards, Stojan Sljivic
Comments:	By: Olle Johansson (oej) 2005-05-10 10:27:08 CAn we get the same option per account and per vmcontext as well? By: ssljivic (ssljivic) 2005-05-11 10:33:36 I have uploaded new patch version. This version now includes maxmsg option in [general] section, as well as in the context and mailbox definition. During testing I have encountered that if Old folder is full all heard messages will be deleted but not saved to the Old folder. This way those messages will be lost. I have changed behavior so that the message are not deleted if move operation was not successful. Regards, Stojan Sljivic By: ssljivic (ssljivic) 2005-05-12 11:05:46 What is the current status of this issue/patch? Regards, Stojan Sljivic By: Kevin P. Fleming (kpfleming) 2005-05-14 23:10:32 Notes: - This will need to work for Realtime voicemail users as well, please modify find_user_realtime() appropriately. - There needs to be a hard upper limit on this value of 9999, since we use a 4-digit format for creating file names. Please warn the user and set the value to 9999 if they try to set it higher (pathological, I know). - Don't cast the return value from malloc/calloc/etc, it's never necessary to cast to/from a 'void *'. - You need to call ast_config_destroy() when the module is unloaded, or there will be a memory leak. - When 'maxmsg' is set at the per-context or per-user level, there is no checking for invalid or out-of-range values. - You moved a bunch of statements to inside an 'if (!cmd)' block, but didn't remove the if statements in front of them, which are now unnecessary. Also, the snprintf() in that sequence was executed before even if cmd was non-NULL, but now it's not, so check to make sure nothing has broken. By: ssljivic (ssljivic) 2005-05-16 05:51:20 I have posted new patch. For details, see my comments below. Regards, Stojan Sljivic - This will need to work for Realtime voicemail users as well, please modify find_user_realtime() appropriately. [Stojan Sljivic] This already works for Realtime users since find_user_realtime() is using apply_option(). - There needs to be a hard upper limit on this value of 9999, since we use a 4-digit format for creating file names. Please warn the user and set the value to 9999 if they try to set it higher (pathological, I know). [Stojan Sljivic] Implemented. - Don't cast the return value from malloc/calloc/etc, it's never necessary to cast to/from a 'void *'. [Stojan Sljivic] Implemented as suggested. On the other hand I really think that this is a rule of good coding. It is not necessary for compiler, but it makes code more readable and easier to maintain. - You need to call ast_config_destroy() when the module is unloaded, or there will be a memory leak. [Stojan Sljivic] Fixed. - When 'maxmsg' is set at the per-context or per-user level, there is no checking for invalid or out-of-range values. [Stojan Sljivic] Implemented. - You moved a bunch of statements to inside an 'if (!cmd)' block, but didn't remove the if statements in front of them, which are now unnecessary. Also, the snprintf() in that sequence was executed before even if cmd was non-NULL, but now it's not, so check to make sure nothing has broken. [Stojan Sljivic] Updated as requested. I have moved the snprintf() outside the 'if (!cmd)' in order to avoid any issue, though I think that leaving it in the 'if (!cmd)' block wouldn't harm. By: Kevin P. Fleming (kpfleming) 2005-05-16 09:59:29 Looks good to me. I'll disagree that adding extra casts makes the code more 'maintainable', but reasonable people can disagree :-) If you end up making another version of this patch for some reason, you can make a single function to parse/validate the 'maxmsg' value instead of repeating it three times, but I don't think you need to make a new patch just for that. This now just needs some people to do functionality testing and mark it pass or fail... By: Kevin P. Fleming (kpfleming) 2005-05-16 10:01:21 Needs functionality testers... bug marshals, unite! By: Michael Jerris (mikej) 2005-06-26 18:16:06 It seems no one, not even the OP is willing to spend any time to post testing results on this.  If anyone is still interested in getting this in, please reopen the bug with testing results. By: ssljivic (ssljivic) 2005-06-27 04:43:18 Hi, I'll find someone to test this issue and post results. Can we reopen it? Regards By: Boris Zolotarev (boriszolotarev) 2005-06-27 04:54:13 Hi, We are using asterisk in our offices for voicemail and communication, and we had to recompile code and to change hardcoded value in order to increase max. number of messages per mailbox. Initial 99 messages was to small for our needs. We are running voicemail application for our client's call center as well. We found this feature very useful and we just recompiled asterisk code with this patch and it is working fine. I would like to see this feature included into asterisk stable version some time in the future. Please let me know if you need some more feedback or test results (what will be the test results format?) from me. Cheers, Boris Zolotarev By: Kevin P. Fleming (kpfleming) 2005-07-11 22:45:56 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:40:58.000-0600 Repository: asterisk Revision: 6099 U   trunk/apps/app_voicemail.c U   trunk/configs/voicemail.conf.sample ------------------------------------------------------------------------ r6099 | kpfleming | 2008-01-15 15:40:57 -0600 (Tue, 15 Jan 2008) | 2 lines support a configurable number of mailboxes per folder (bug ASTERISK-4129) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=6099