<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/2220/">https://reviewboard.asterisk.org/r/2220/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 29th, 2012, 6:34 p.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This does fix the msg-id is lost problem when a message is 'moved' from inbox to old, or any other folder move.
I havn't followed this code path, but if you copy (note not move) a message from 1 mailbox to another (if that's possible), then we'd have 2 messages with the same msg_id.
Does msg_id need to be unique?
</pre>
</blockquote>
<p>On November 29th, 2012, 7:43 p.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes it does. A copy should result in a new msg_id.</pre>
</blockquote>
<p>On November 30th, 2012, 10:28 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">We need to come up with a new method for generating the msg_id then, because the one we have won't really cut it if uniqueness is necessary accross all mailboxes. The second half of the string is just a string hash value for a sequence of the extension, context, and the callerid used for the channel that called voicemail, so that is going to stay the same during a copy. The first half is just the epoch time (seconds). If you have a function that copies a message to twice at roughly the same time, that isn't going to work.
So for reference, the current msg_id string is...
(A)-hash((B)(C)(D))
where A is a timestamp unique to each second, B is call_extension, C is call_context, and D is call_callerid.
We could reasonably guarantee uniqueness if we simply used an atomically increasing counter (like how we handle numerous channel names currently) in addition to the epoch time, so I'm thinking the new msg_id string could be...
(U)-(A)-hash((B)(C)(D))
where U is the incrementing counter and everything else is the same. When doing a copy, the timestamp becomes the time at copy.
That probably isn't enough though still. Multiple instances of Asterisk could be using the database and presumably both could do a copy at the same time to the same message. Might as well throw systemname of whichever Asterisk instance did they operation in there for good measure.
(S)(U)-(A)-hash((B)(C)(D))
We might need to increase the character count if we do this. systemnames usually aren't more than 20 characters or so, but adding that to the unique ID (8 hex characters), and the existing stuff (which is usually right around 20 caracters right now and could be larger under certain conditions), the current 40 that we suggest on wiki and in the contrib script would probably be inadequate. Alternatively we could just scrap the call_extension, call_context, and call_callerid hash from the message since it would no longer be necessary to provide uniqueness. We could also convert the timestamp into hex to save some characters.</pre>
</blockquote>
<p>On November 30th, 2012, 10:44 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks like we actually set aside 128 characters for system name. Yuck. Maybe I should hash that and write as hex too.</pre>
</blockquote>
<p>On December 3rd, 2012, 10:30 p.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">found: http://linux.die.net/man/3/uuid_generate
uuid_generate, uuid_generate_random, uuid_generate_time, uuid_generate_time_safe - create a new unique UUID value
The UUID is 16 bytes (128 bits) long, which gives approximately 3.4x10^38 unique values (there are approximately 10^80 elementary particles in the universe according to Carl Sagan's Cosmos). The new UUID can reasonably be considered unique among all UUIDs created on the local system, and among UUIDs created on other systems in the past and in the future.
Maybe something in this as to what it's using for randomness.</pre>
</blockquote>
<p>On December 3rd, 2012, 10:41 p.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">perhaps the work has already been started/done https://reviewboard.asterisk.org/r/2217/</pre>
</blockquote>
<p>On December 4th, 2012, 9:46 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">We can't use that in 1.8 unfortunately. Matt and I talked about this and its important to keep it either the same or similar to the current ID, at least as far as the appearance of the ID is concerned.</pre>
</blockquote>
<p>On December 4th, 2012, 10:09 a.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It wouldn't be 1.8, it'd actually be in 11.
The reason for not making that change in release branches is to not introduce a library dependency in the middle of a version release (and, if the column length changes, potential schema modifications as well). As it turns out, that's less of a concern since pjproject depends on libuuid as well (as Russell discovered), so its possibly not as big of a show stopper - but creating a dependency on a library mid-stream feels bad.</pre>
</blockquote>
<p>On December 4th, 2012, 10:13 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> It wouldn't be 1.8, it'd actually be in 11.
Er, that's what I meant. </pre>
</blockquote>
<p>On December 18th, 2012, 8:03 p.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">asterisk-11 CHANGES.TXT
{code}
All messages created in old Asterisk installations will have a msg_id added to
them when required. This operation should be transparent as well.
{code}
This isn't the case!</pre>
</blockquote>
<p>On December 18th, 2012, 11:08 p.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I haven't really changed anything about the circumstances behind adding msg_ids to messages from old installations, so if is false with the patch, it's false without the patch. Can you please be a little more specific about the scenario for which this isn't true? I've tried using voice mails recorded in old installations with this patch and they seem to work and receive the msg_id when they are read and moved as what you'd generally expect to be required.
I think transparency in this case just means the end user doesn't have to be particularly involved with the process, but if that's what you mean please enlighten me.</pre>
</blockquote>
<p>On December 19th, 2012, 12:47 a.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">CHANGES.txt was wrong before this patch, so you haven't broken anything as you say.
Now with the opening of a voicemail, they would generally be moved old, and you patch will set a msg_id.
The issue we're seeing is the greeting, from an old asterisk install, it didn't have a msg-id, and because it's not moved around, it will never get one.
Thus we always see the error "SQL Get Data error! coltitle=msg_id"
</pre>
</blockquote>
<p>On December 19th, 2012, 1 a.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">does the mysql msg_id column need to change size for this patch?
If so what to?</pre>
</blockquote>
<p>On December 19th, 2012, 9:46 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> does the mysql msg_id column need to change size for this patch?
> If so what to?
As of the latest version of this patch, no, so nothing.
> The issue we're seeing is the greeting, from an old asterisk install, it didn't
> have a msg-id, and because it's not moved around, it will never get one. Thus we
> always see the error "SQL Get Data error! coltitle=msg_id"
>From what I'm looking at in app_voicemail, what you are referring to is a warning
and not an error. I take it you mean one that was already moved out of the inbox
in the older version then. What overall is the impact of this warning? It looks
like we are looking at a separate bug from the whole concept of adding message IDs
as needed. Instead, this appears to be a problem where we haven't accounted for
the possibility of having a field with a NULL value when loading a message under
some circumstance.
Try changing the condition above the code that generates this error from:
if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) {
to:
if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO) && (res != SQL_NO_DATA))
and then see how that impacts this warning. I don't really think it's actually
impeding the operation of voicemail, but I could be mistaken I suppose.
I suppose if msg_id isn't your last column this might prematurely exit the loop
which could be rather bad...</pre>
</blockquote>
<p>On December 19th, 2012, 3:19 p.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is the exact warning:
WARNING[29534][C-000000fc]: app_voicemail.c:3713 retrieve_file: SQL Get Data error! coltitle=msg_id
It's not causing any operational problems, Just feels bad on the console.
refer to the ASTERISK-20717 linked to in this review, for the situations I'd identified when it occurs.</pre>
</blockquote>
<p>On December 19th, 2012, 4:46 p.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So does this problem persist after having played the message once? New messages, or old messages only? With the patch?
I'm going to guess that the warning might be able to be reproducable for the same message multiple times for messages that were already old even with this patch applied, which I'll test tomorrow... but if this isn't the case and we really only see the warning once per message before the field is populated, then this isn't really a problem, just a minor nuisance. If it is though, I'll need to make sure we can update the field when it doesn't exist.</pre>
</blockquote>
<p>On December 20th, 2012, 2:41 p.m., <b>Alec Davis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This warning is 100% repeatable if the greeting was recorded on a 1.8 box, by going into to app_directory, selecting a user, then listen to the recorded name.
[2012-12-21 09:17:49.992112] WARNING[27026][C-00000413]: app_voicemail.c:3713 retrieve_file: SQL Get Data error! coltitle=msg_id
[SELECT * FROM voicemessages WHERE dir=? AND msgnum=?]
-- <DAHDI/i1/45609100-1c1> Playing '/var/spool/asterisk/voicemail/default/8512/greet.gsm' (language 'en')
-- <DAHDI/i1/45609100-1c1> Playing 'dir-instr.slin' (language 'en')
I haven't tried the patch on this review yet, as this is a live server.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">No need. I've already checked it and can cofirm it is repeatable as long as the message was moved prior to using the newer version of Asterisk. Also it appears to happen in 1.8 as well, and that's also an annoyance we need to deal with.
For now though, this review should focus on the correctness of msg_id generation as well as the fact that the field would be dropped when moving it in 11. I won't close the issue when that's resolved since there are still lingering problems.</pre>
<br />
<p>- jrose</p>
<br />
<p>On December 3rd, 2012, 11:22 a.m., jrose 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, Mark Michelson, Matt Jordan, and kmoore.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Dec. 3, 2012, 11:22 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;">When a message is moved between folders (such as after listening to new messages, they are switched to the old messages folder), the msg_id field added in Asterisk 11 isn't copied. This patch simply adds that field to what is copied in the odbc COPY function and appears to solve the problem.</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;">Made messages in a mailbox using app_voicemail. Checked mysql to see the msg_id. Verified an ID was created.
Listened to messages, made sure the COPY function was invoked. Checked mysql again and saw that the IDs were gone.
Applied the patch, repeated the process, the IDs remained intact.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20717">ASTERISK-20717</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/11/apps/app_voicemail.c <span style="color: grey">(376833)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2220/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>