<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1883/">https://reviewboard.asterisk.org/r/1883/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 24th, 2012, 12:10 p.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1883/diff/1/?file=27529#file27529line14785" style="color: black; font-weight: bold; text-decoration: underline;">/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct ast_str *vm_mailbox_snapshot_str(const char *mailbox, const char *context)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">14770</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"Message %d is out of range. Last message is %d</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">cur_msg</span><span class="p">,</span> <span class="n">vms</span><span class="o">-></span><span class="n">lastmsg</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">14758</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">other_msg_id</span> <span class="o">=</span> <span class="n">ast_variable_retrieve</span><span class="p">(</span><span class="n">msg_cfg</span><span class="p">,</span> <span class="s">"message"</span><span class="p">,</span> <span class="s">"msg_id"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Mark</p>
<br />
<p>On April 24th, 2012, 11:46 a.m., Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and Matt Jordan.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated April 24, 2012, 11:46 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">As you'll see in this review, I have updated the unit tests in tests/test_voicemail_api.c. They all pass.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c <span style="color: grey">(363307)</span></li>
<li>/team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h <span style="color: grey">(363307)</span></li>
<li>/team/mmichelson/trunk-digiumphones/tests/test_voicemail_api.c <span style="color: grey">(363307)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1883/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>