Summary:	ASTERISK-13098: [patch] Multiple bugs in app_minivm
Reporter:	Bradley Watkins (marquis)	Labels:
Date Opened:	2008-11-21 09:51:08.000-0600	Date Closed:	2009-01-08 15:39:25.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_minivm
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_minivm.c.patch ( 1) minivm_trunk_fixes.patch ( 2) minivm_trunk_fixes2.patch ( 3) minivm_trunk_fixes3.patch
Description:	There are several minor bugs in MiniVM, and this patch resolves most of them.  See Additional Information for details of the rest. ****** ADDITIONAL INFORMATION ****** There is one additional issue/set of issues regarding inconsistency between the names of channel variables set by the application and the names used in the docs shown by 'core show application Minivm*' In some instances, the code set variables using a prefix of MVM_ while the docs say MINIVM_.  In other cases the reverse is true.  I'm happy to provide a patch to make everything consistent, but I would like some guidance as to which way to go (MVM or MINIVM).
Comments:	By: Leif Madsen (lmadsen) 2008-11-24 13:11:26.000-0600 OK this is my suggestion as to how to resolve the variable naming thing. Personally I think the variables should start with MINIVM_ because MVM_ isn't very descriptive. However, we ideally want to maintain backwards compatibility I suppose. How about accepting both MVM_ and MINIVM_ in 1.4, with a deprecation warning in 1.4 stating MVM_ would be removed in future versions of Asterisk. Then in trunk change the prefixes to MINIVM_ straight up. Thoughts? By: Bradley Watkins (marquis) 2008-11-25 06:58:37.000-0600 I think, generally, that this is probably a good approach.  But what about the variables that minivm itself sets?  There is still some inconsistency there.  Should it set variables with *both* the MVM and MINIVM prefixes? By: Leif Madsen (lmadsen) 2008-11-25 07:04:50.000-0600 I would probably say yes to that if we're going to go with the approach I suggested. At that point you would need to mark all MVM_ variables as deprecated I suppose. I think I will ask Corydon76 about this, as he probably has an idea as to the best approach he would prefer to see (he's the syntax guru) By: Leif Madsen (lmadsen) 2008-11-25 07:06:02.000-0600 Hey Corydon76, what do you think about the suggestion in my above note about trying to get variables for miniVM consistant? By: Bradley Watkins (marquis) 2008-11-25 15:06:00.000-0600 I'll create a patch for 1.4 as soon as we figure out exactly what it's supposed to be doing, but for now at least I uploaded a new patch for trunk with the previous bug fixes plus the change to make everything consistently use the MINIVM_ prefix (at least insofar as channel variables and documentation for them goes.  I left all the internal variables alone). By: Tilghman Lesher (tilghman) 2008-11-25 15:48:13.000-0600 The variables for 1.4 must not be renamed, at all.  In fact, given our stated objective of not removing deprecated items in the future, it might be better not to rename those variables in trunk, either.  Twice as many variables uses twice as much storage for no particular gain. By: Bradley Watkins (marquis) 2008-11-25 16:12:36.000-0600 What is the correct direction, then?  The fact is, either the docs are wrong or the variables being set by the code are wrong in certain cases.  One or the other needs to change. I view this as a long-standing bug in MiniVM.  Unless you read the code, you don't know whether the variables begin with MVM or MINIVM.  And even once you do that, your dialplan ends up with a mix of the two for it to even be functional. By: Olle Johansson (oej) 2008-11-26 00:51:40.000-0600 Marquis: Thanks for testing, patching and working with minivm! I think the variables should be called MINIVM all over. Personally, I think it's a bug with all the MVM_ stuff. By: Tilghman Lesher (tilghman) 2008-11-26 10:36:24.000-0600 oej:  We're using MVM because it causes the least change in variable naming.  Given our current policy of not removing deprecated functionality, it means the least duplication of variable use. By: Olle Johansson (oej) 2008-11-26 11:28:46.000-0600 Ok, as long as we are consistent that's fine. I really would like to add more stuff to minivm, just need a customer that wants to fund. The app is sending thousands of voicemails each day in at least five service providers that I'm aware of. It could do more than just record and send though. By: Bradley Watkins (marquis) 2008-12-03 11:23:43.000-0600 Is there anything I can do to move this along? By: Leif Madsen (lmadsen) 2008-12-05 09:40:30.000-0600 Assigning to myself so I realize to test this afternoon. By: Bradley Watkins (marquis) 2008-12-05 10:13:16.000-0600 Thanks to eliel on #asterisk-dev for pointing out that his recent doc change to minivm made my patch fail for some hunks, so here's a new one that will apply to current trunk. By: Bradley Watkins (marquis) 2008-12-18 09:44:47.000-0600 Hrmmmm.... Ping? Anything else I need to do before this is ready? By: Leif Madsen (lmadsen) 2009-01-07 09:15:27.000-0600 Assigned to Corydon76 for a review and commit. I looked at the code, and it looks fine to me! By: Digium Subversion (svnbot) 2009-01-08 15:32:39.000-0600 Repository: asterisk Revision: 167835 U   trunk/apps/app_minivm.c ------------------------------------------------------------------------ r167835 | tilghman | 2009-01-08 15:32:39 -0600 (Thu, 08 Jan 2009) | 6 lines Textual changes, consistency in status variable naming, and other minor bugs. (closes issue ASTERISK-13098) Reported by: Marquis Patches:       minivm_trunk_fixes3.patch uploaded by Marquis (license 32) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=167835 By: Digium Subversion (svnbot) 2009-01-08 15:39:24.000-0600 Repository: asterisk Revision: 167836 _U  branches/1.6.1/ U   branches/1.6.1/apps/app_minivm.c ------------------------------------------------------------------------ r167836 | tilghman | 2009-01-08 15:39:24 -0600 (Thu, 08 Jan 2009) | 13 lines Merged revisions 167835 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........  r167835 | tilghman | 2009-01-08 15:32:45 -0600 (Thu, 08 Jan 2009) | 6 lines    Textual changes, consistency in status variable naming, and other minor bugs.  (closes issue ASTERISK-13098)   Reported by: Marquis   Patches:         minivm_trunk_fixes3.patch uploaded by Marquis (license 32) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=167836