<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/1898/">https://reviewboard.asterisk.org/r/1898/</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/1898/diff/1/?file=27638#file27638line3889" 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; ">static SQLHSTMT insert_data_cb(struct odbc_obj *obj, void *vdata)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3889</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SQLBindParameter</span><span class="p">(</span><span class="n">stmt</span><span class="p">,</span> <span class="mi">12</span><span class="p">,</span> <span class="n">SQL_PARAM_INPUT</span><span class="p">,</span> <span class="n">SQL_C_CHAR</span><span class="p">,</span> <span class="n">SQL_CHAR</span><span class="p">,</span> <span class="n">strlen</span><span class="p">(</span><span class="n">data</span><span class="o">-></span><span class="n">msg_id</span><span class="p">),</span> <span class="mi">0</span><span class="p">,</span> <span class="p">(</span><span class="kt">void</span> <span class="o">*</span><span class="p">)</span> <span class="n">data</span><span class="o">-></span><span class="n">msg_id</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="nb">NULL</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;">If it hasn't been done already, the schema files need to be updated with the new column</pre>
</div>
<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/1898/diff/1/?file=27638#file27638line11343" 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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int add_message_ids_folder(struct ast_vm_user *vmu, int folder)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11343</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="ew"> </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;">Blob</pre>
</div>
<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/1898/diff/1/?file=27638#file27638line11425" 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; ">static struct ast_vm_user *find_or_create(const char *context, const char *box)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11425</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">add_message_ids</span><span class="p">(</span><span class="n">vmu</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11426</font></th>
<td bgcolor="#c5ffc4" 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">"Unable to add unique message IDs for all messages for voicemail user %s@%s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">vmu</span><span class="o">-></span><span class="n">mailbox</span><span class="p">,</span> <span class="n">vmu</span><span class="o">-></span><span class="n">context</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11427</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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;">This is an incredibly expensive operation, as it has to load each voicemail meta file that a user has into memory. I have no idea how expensive this is going to get in the 'real world', but I imagine that if someone hasn't cleaned up their voicemail folders (or has a 100 old messages), its going to take a long time. I have seen some issues reported where response times from some IMAP servers are on the order of seconds - having to pull back each individual message could make this operation run for minutes.
Additionally, once all voicemails have been populated with message identifiers, this step is no longer necessary, but we would continue to perform it. Doing it once during an upgrade is one thing, doing it repeatedly as part of the normal operation is pretty harsh.
I can imagine a few ways (none of which are all that palatable) to alleviate this:
1. Have a configuration override to disable it. After a week or so of painfully slow voicemail user loads, a sys admin can at least disable it.
2. Perform the action in module load, after all voicemail users are loaded. This would be a one time action, after which it would not impact normal operations. This could be combined with option #1 to make it so that it only occurs a single time.
3. Perform the action lazily by not doing it in find_or_create. Instead, do it when we load a voicemail into memory - if a unique ID is missing, generate it and re-store the meta file.
</pre>
</div>
<br />
<p>- Matt</p>
<br />
<p>On May 2nd, 2012, 4:04 p.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.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated May 2, 2012, 4:04 p.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;">There are two main things added here.
1. Voicemail APIs added in trunk rely on voicemail messages possessing a "msg_id" in their metadata. This is how messages are looked up when they are to be moved, played, removed, and forwarded. The issue is that msg_id is a new piece of data and is not present on voicemails saved in older versions of Asterisk. This patch aims to fix that. When voicemail is loaded, each user's voicemail box is searched and each message is checked for msg_id. If it is not present, one is generated and added to that message's metadata. For ODBC, I added a method that specifically is used to issue a SQL UPDATE command to set the msg_id for the message. IMAP does not have this ability because I have no idea how to alter/update stored voicemails in IMAP.
2. I noticed that the ODBC function for storing voicemails did not bother to store the msg_id. I have updated the code to do this properly.</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;">For 1, I have manually tested by leaving a voicemail using Asterisk 1.8 and then loading app_voicemail.so in trunk-digiumphones. The metadata file for the message had a msg_id added to it as expected.
For 2, I attempted to test leaving messages via ODBC, but I ran into failures to execute the SQL INSERT statement when storing the voicemail. I attempted on a clean trunk checkout and on a clean 1.8 checkout and got the same error, so I figure it's not necessarily a coding issue but a setup issue on my end. I figured that's not enough to stop me from posting the review now and getting my setup fixed up after. If it turns out I find any glaring bugs in the code, I'll update the diff here.</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">(364886)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1898/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>