[Home]

Summary:ASTERISK-14816: [patch] voicemail should mark messages as read by doing an UPDATE instead of an INSERT followed by a DELETE
Reporter:Kyle Kienapfel (kylek)Labels:
Date Opened:2009-09-10 21:54:20Date Closed:2011-06-07 14:08:18
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail/ODBC
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) attempt00003.patch
( 1) attempt00004.patch
Description:I sniffed out that app_voicemail was doing an INSERT to move a message from Inbox to Old and decided to change it to an UPDATE. I'm running asterisk 1.6.1 svn but I'll try this against trunk in a bit.
Comments:By: Kyle Kienapfel (kylek) 2009-09-11 12:30:56

I accidentally sent in a wrong patch
"++vms.oldmessages" compiled in svn trunk even though thats wrong syntax.
It's supposed to be:
"vms->oldmessages++"

attempt00003.patch is the file to look at,
Third time is the charm.


By: Tilghman Lesher (tilghman) 2009-09-13 18:49:22

There's a few changes that you'll need to make for this patch to be acceptable.  All are outlined in doc/CODING-GUIDELINES.

1) Tab-indentation, not spaces.
2) No C++-style comments.
3) Spaces after commas in function calls, not before.

Also, you may want to make sure that if somebody records a voicemail at the same time as you are listening to voicemail, that this does not create a race whereby the new message is stuck into the slot you just renamed from, and the forthcoming delete in the code (which is supposed to be a no-op) kills the new message.
By: Leif Madsen (lmadsen) 2009-09-16 09:06:54

Changing status to feedback while waiting for reporters changes. Thanks for the patch!
By: Kyle Kienapfel (kylek) 2009-09-19 23:34:24

I've fixed the formatting, but I'm still pondering race conditions.
I need to figure out why the /* Delete ALL remaining messages */ block is there in the first place.

I'll keep plugging away at this as time permits, and try and get rid of that second loop.
By: Leif Madsen (lmadsen) 2009-09-22 09:34:32

OK great, thanks! I'll leave this in Feedback status for now. When you get a chance to update this then I'll set it back to Ready for Testing. Thanks!
By: Leif Madsen (lmadsen) 2009-10-26 09:43:51

Pinging this issue?
By: Leif Madsen (lmadsen) 2009-12-22 14:53:34.000-0600

Ping...
By: Tilghman Lesher (tilghman) 2010-01-12 17:09:52.000-0600

This is wholly not going to work without a major restructuring of the code.  The main problem here is that when you remove a voicemail out of sequence, the code then fails to find messages beyond where that message used to be.  In other words, we depend upon messages being contiguous in the code, so when you create a hole by renaming a file, the rest of the code fails.

I'll suspend the issue for now, and you're welcome to reopen if/when you have a patch that has been tested against the above scenario.