<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 />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<p>- Tilghman</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>