[asterisk-dev] [Code Review]: Switch public voicemail APIs over to use unique message IDs instead of message indexes.

[Mark Michelson]

> On April 24, 2012, 12:10 p.m., Tilghman Lesher wrote:
> > /team/mmichelson/trunk-digiumphones/apps/app_voicemail.c, line 14785
> > <https://reviewboard.asterisk.org/r/1883/diff/1/?file=27529#file27529line14785>
> >
> >     Please add a call to ast_realtime_require_field with this field name, expected type, and length.  This is needed to support ODBC storage correctly.
> >     
> >     Additionally, I'd like to see IMAP and ODBC-optimized versions of the retrieval of this new field, because both of those would benefit (and in fact, would be faster than the file-based approach).

Your comment about adding ast_realtime_require_field() got me thinking about another thing that should probably be done. Since msg_id is a new piece of metadata for voicemails, doing an upgrade to Asterisk 11 would likely mean that many voicemails would exist without a msg_id associated with them. So on startup, it would probably be good to iterate through all voicemails and add a unique message ID to any messages that currently do not have a msg_id. I can add that in a separate review. Oh, and I will also add the ast_realtime_require_field()

Regarding your suggestion for IMAP and ODBC-optimized retrievals, I agree that they would be good to have. In fact, a RETRIEVE-like macro that grabbed all metadata into memory without writing to disk or bothering to get media would be a handy thing to use throughout app_voicemail since there are many places where we retrieve a message simply to get at metadata. My main worry here is scope creep, though. If I were to make an IMAP and ODBC-optimized path, then I'd want to apply it all over the code, and that goes beyond the scope of making the voicemail APIs use unique message IDs instead of message indexes.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1883/#review6047
-----------------------------------------------------------


On April 24, 2012, 11:46 a.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1883/
> -----------------------------------------------------------
> 
> (Updated April 24, 2012, 11:46 a.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Voicemail has some new public functions added to it in order to interoperate with Digium phones. These public functions give fine control over the ability to move, remove, forward, and play back messages. In the implementation in 1.8-digiumphones and 10-digiumphones, a message index is used to identify which message to operate on. This is potentially unreliable since mailboxes can be acted on by multiple sources. While the most likely result is that a user may try to manipulate a message that no longer exists, it is also possible that a user may end up manipulating an incorrect message instead. This could be disastrous depending on which wrong message is manipulated and how it is.
> 
> This patches the trunk version of app_voicemail to use unique message IDs instead of message indexes. It is not possible to manipulate the wrong message. The result of this is a performance decrease since there is more message retrieval from whatever storage is used, and there are more disk reads in order to check message IDs. A long-term goal would be to store message IDs in memory so that finding a message by its ID is not as expensive. However, this is a dark and treacherous road to go down and way beyond the scope of what needs to be done for now.
> 
> If there are additional changes that should be added with regards to the voicemail APIs, let me know.
> 
> 
> Diffs
> -----
> 
>   /team/mmichelson/trunk-digiumphones/apps/app_voicemail.c 363307 
>   /team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h 363307 
>   /team/mmichelson/trunk-digiumphones/tests/test_voicemail_api.c 363307 
> 
> Diff: https://reviewboard.asterisk.org/r/1883/diff
> 
> 
> Testing
> -------
> 
> As you'll see in this review, I have updated the unit tests in tests/test_voicemail_api.c. They all pass.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120424/22a98a64/attachment-0001.htm>