[Home]

Summary:ASTERISK-04295: [patch] [needs testers] Add context and mailbox to database with odbc_storage
Reporter:Jason Parker (jparker)Labels:
Date Opened:2005-05-28 00:56:30Date Closed:2008-01-15 15:45:28.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) extended-storage-vm-mwi-07-20-05.txt
( 1) voicemail.20050530-1230.diff
Description:Juggie requested a patch to add users context and mailbox to the database when using odbc_storage, instead of depending on the dir column.

This patch does just that.  Requestor has tested, and approved.

I've never claimed to be the best programmer, so try to go easy on me. :)

****** ADDITIONAL INFORMATION ******

Disclaimer on file - under a.k.a. North Antara

Patch updates apps/voicemail.c and doc/README.odbcstorage
Comments:By: Jason Parker (jparker) 2005-05-28 00:59:11

It seems Mantis hosed my timestamp, thinking it was a note.  That should be "05/27/2005 approximately 8:00PM PDT".
By: Donny Kavanagh (donnyk) 2005-05-28 01:01:05

I can confirm this works as requested.

Thanks! Alot easier then parseing dir field.

A nice Web VM-Interface should now be easy, no more parsing files.
By: Donny Kavanagh (donnyk) 2005-05-28 23:36:27

north,

When i try and check my voicemail over the phone, it dies with May 29 00:22:46 WARNING[21644]: app_voicemail.c:3373 play_message: No origtime?!

The odbc bind is attaching to the wrong field maybe?  Can you take a look.

Thanks.
By: Jason Parker (jparker) 2005-05-29 20:34:45

Did this work before the patch?

Does this always happen?

It almost seems like ast_config_load isn't returning anything.

Can somebody explain why there are two for loops in ast_variable_retrieve()?

if (category) {
       for (v = ast_variable_browse(config, category); v; v = v->next)
               if (variable == v->name)
                       return v->value;
       for (v = ast_variable_browse(config, category); v; v = v->next)
               if (!strcasecmp(variable, v->name))
                       return v->value;
}

Couldn't the above be simplified as something like below, using only one loop?

if (category) {
       for (v = ast_variable_browse(config, category); v; v = v->next)
       {
               if ((variable == v->name) || (!strcasecmp(variable, v->name)))
                       return v->value;
       }
}


It would be far easier to test this, if I knew how to even set it up...got any hints?

On second thought...here is the code around line 3373

msg_cfg = ast_config_load(filename);
[...snip...]
if (!(origtime = ast_variable_retrieve(msg_cfg, "message", "origtime"))) {
       ast_log(LOG_WARNING, "No origtime?!\n");
       DISPOSE(vms->curdir, vms->curmsg);
       return 0;
}

The table is hardcoded as "voicemessages".  From what I'm reading in the code, ast_config_load() can pull a config from a DB, but... "message" (from the ast_variable_retrieve() line) != "voicemessages".  Is it trying to pull it from the "wrong" config table?

If the above is true, I don't really know how to proceed.  We could put a bunch of #ifdef's around some stuff, or simply rename the table in the DB.  Would renaming the table conflict with anything else?
By: Jason Parker (jparker) 2005-05-30 01:56:00

Okay, error was reproducible for me (with or without my patch).  I spent a bit looking at the code, before realizing it might be a DB problem.  Show me your create table, would ya?  I was getting the same error, until I removed msgnum from the primary key.

CREATE TABLE `voicemessages` (
 `msgnum` int(11) NOT NULL default '0',
 `dir` varchar(80) default '',
 `context` varchar(80) default '',
 `macrocontext` varchar(80) default '',
 `callerid` varchar(40) default '',
 `origtime` varchar(40) default '',
 `duration` varchar(20) default '',
 `mailboxuser` varchar(80) default '',
 `mailboxcontext` varchar(80) default '',
 `recording` longblob,
 KEY `dir` (`dir`)
) TYPE=MyISAM;

The reason it gives that error, is because the msgXXXX.txt file is empty when it tries to read it.  This appears to be caused by the query returning 0 rows.  Perhaps there should be an else with an ast_log on the if (rowcount), to remove the ambiguity from the "no origtime" error?

I'll fix the patch up a little tomorrow, and I'll be sure to include the MySQL create table syntax above (if anybody wants to submit theirs for other DB engines and/or for MySQL, that'd be very helpful).
By: Jason Parker (jparker) 2005-05-30 12:34:56

New (20050530-1030) patch is no good...hang on.
By: Jason Parker (jparker) 2005-05-30 12:43:40

ok, new patch (20050530-1040) uploaded.  This adds an else if for the rowcount, to show a warning if there are no rows and msgnum is > -1.  msgnum is -1 when the message hasn't been saved yet (at first (in 20050530-1030) I made it an else...which was no good).

Juggie (and others), can you re-test with the latest patch?
By: Jason Parker (jparker) 2005-05-30 14:45:22

1040 patch didn't apply cleanly...spaces got stuck in there, where tabs should have been.  1230 fixes this (tested...twice).

Tip: Don't copy/paste patches together, and expect them to work.
By: Donny Kavanagh (donnyk) 2005-05-30 14:51:55

Patch applies cleanly.

Functionality wise i was able to leave/retreive a message over the phone, and pull it from a blob in mysql.  Which was obviously accessed via odbc.

I say this goes in cvs, so bug marshalls, do your thing.
By: Donny Kavanagh (donnyk) 2005-05-31 18:17:10

*Ping* can we get some reviews on this code and get it into cvs... thanks.
By: Jason Parker (jparker) 2005-05-31 19:43:38

More testers would probably be a good start...

Anybody willing to do so?
By: Donny Kavanagh (donnyk) 2005-06-02 15:12:41

I refuse to let this die, lets get a tester, or just get it in cvs.  This works fine for me.
By: Kevin P. Fleming (kpfleming) 2005-06-02 21:58:30

The code looks fine, and I can see the value in having this information in the database tables, but I'm concerned about forcing people to upgrade their table structures at their next CVS HEAD pull... I know we break compatibility all the time in config files, but I don't think we want to break the database format this way without a really, really good reason.

Can someone test this patch against a database that does _not_ have the new columns and see if it's still possible for app_voicemail to operate normally?
By: Jason Parker (jparker) 2005-06-02 22:44:47

kpfleming, you're almost certainly correct about breaking the existing structure.  Do you have any suggestions on what I can do differently to allow both ways to work still?

Perhaps we can check for the existance of the columns in the database, and..
A) modify the queries depending on if the column exist
B) warn about the existance of the new columns to the CLI

If anyone comes up with good ideas, I'd be more then happy to try to implement them.
By: Donny Kavanagh (donnyk) 2005-06-02 23:09:08

While i agree in theory, i think backword compability would make app_voicemail even uglier then it is now, so i dont think its necessairy.  Head is supposed to be a changing environment and i think this falls within those guidelines.

I think if we are going to break it, we should look over the table structure and see if anything else needs to be added and then go ahead and do it.
By: Michael Jerris (mikej) 2005-06-02 23:24:51

If we are going to keep doing a bunch of db config stuff, we need a tabledef versioning system.  One table, statically name (with a conf override) with a simple 2 column of tablename(or type or somthing) and version would do it.. .then we could use it for many things, and check that the table is the correct version when we load.  It could be used either to turn on and off functionality, or just to fail loading and tell the user why.  This has the nice benefit of the scripts required to update the db could check it for correct version before update, and update this table all in one script.  Thoughts?
By: silik0n (silik0n) 2005-06-03 00:16:59

with the MACRO stubs for this stuff, it might be an idea to make it odbc_storage2 with a different define to uncomment. would make for more source, but not more object code etc then we would get with the patch (of course i'm on drugs right now so i could be talking out my butt)
By: Donny Kavanagh (donnyk) 2005-06-03 18:06:27

Ping... so any ideas on how to make this backwords compatible?  Or should we just report a error?
By: Kevin P. Fleming (kpfleming) 2005-06-03 18:40:06

I think the best solution for now is to do the inserts and updates in two phases; one that modifies the existing columns, and a following one that updates the new columns. The second one can ignore failures, except for one time per module load, when it would log a warning message to the console requesting that the user add the new columns to their database.

At some point in the future (60 days?) we can remove the split updates and the warning.
By: Donny Kavanagh (donnyk) 2005-06-05 10:44:47

Sounds good to me.
By: Donny Kavanagh (donnyk) 2005-06-17 00:25:50

Whats going on here, can we get this updated with the requested changes and get it in cvs?
By: drmac (drmac) 2005-06-20 14:49:57

could we please remove the hardcoded table names? instead add a voicemail.conf variable like "tablename" or something and default it to voicemessages if it isn't defined.
By: Donny Kavanagh (donnyk) 2005-06-22 15:55:08

It seems qwell has gotten busy, would anyone be willing to take this patch and make the reccomended changes?
By: Jason Parker (jparker) 2005-06-23 10:17:35

I've been a bit busy.  I'll see what I can do on Monday or Tuesday.
By: Donny Kavanagh (donnyk) 2005-07-06 23:47:54

Ping?
By: drmac (drmac) 2005-07-07 08:23:16

juggie, im interested to see how you are streaming the audio via a web interface. Instead of going OT on mantis with this, can you contact me offlist? (mboehm at cytelcom dot com) I'd appreciate it. Thanks.
By: Kevin P. Fleming (kpfleming) 2005-07-07 19:11:38

Yes, this needs to get updated, or it will probably not make it into 1.2, since I' trying to prepare the tree for some testing releases and we'll need to have a feature freeze at some point in the near future.
By: Donny Kavanagh (donnyk) 2005-07-07 21:52:12

Is someone willing to take the helm here, it is probally a 30min job, but would take me days to figure out... i wish i could take a look but i never have time.

drmac you just set the mime type, and echo it out... i'll show a little test app here that i wrote, it just pulls out based on the message id... i dont have anything in production as i am waiting for 1.2, i will email as well.

<?
header("Content-type: audio/wav"); // act as a wav file to browser

$database = mysql_connect("trn-db1", "root", "toor");
mysql_select_db("asterisk", $database);

$query = "SELECT *
FROM voicemessages
WHERE msgnum = $id";
$result = mysql_query($query, $database);

$row = mysql_fetch_array($result);
$wav = $row["recording"];
echo $wav;
?>

so simple, supports many servers, you can use replication etc... awesome redundancy.. as you can see id like to get this fixed and in, if someone could make the few changes to get this into cvs i'd be grateful.


By: Donny Kavanagh (donnyk) 2005-07-11 13:28:51

KP, is there anyway you could take a look @ this and see if you can make the required changes... north/qwell on irc has been unresponsive to getting this taken care of.  How about just do what mark does and "commit with slight changes" :)  I'd really like to get this taken care of.  It opens the door for lots of possibilities.
By: Michael Jerris (mikej) 2005-07-11 13:57:46

Juggie, kevin has been busy with bugs, have you considered contracting this work?
By: Donny Kavanagh (donnyk) 2005-07-11 14:06:01

Its 10minutes work for someone who knows what they are doing, contracting is not an option, but i cant sign a disclaimer for things i do on work time.  I will end up making the changes at work and not releasing it and that obviously isnt the best thing to do.
By: drmac (drmac) 2005-07-11 14:29:51

comments on me adding a #ifdef EXTENDED_ODBC_STORAGE into the code for these 2 new columns? That way, no breakage in old version cept perhaps a "Warning, Storage will change soon." message.

For those that want the new columns, just edit the apps makefile.

Sound good? This way there isn't a bunch of if-checks and more blah blah to make app_voicemail even larger.
By: Donny Kavanagh (donnyk) 2005-07-11 14:32:26

Sounds good to me.  When you post the patch i will test it and report.
By: drmac (drmac) 2005-07-11 16:29:52

try the pound_define_version.txt

or i forgot to add the patch for the apps/Makefile

put in CFLAGS+=-DEXTENDED_ODBC_STORAGE and recompile
By: Jason Parker (jparker) 2005-07-11 20:03:11

Sorry, I've been a bit too busy to handle this myself...

drmac, that looks pretty much right to me.  I'll test it later tonight if my 7960 is working again...
By: Donny Kavanagh (donnyk) 2005-07-13 08:36:33

It looks good to me.
By: Kevin P. Fleming (kpfleming) 2005-07-13 12:48:50

The new patch looks acceptable, but it will need to actually modify the README and Makefile before it can be committed :-)
By: Jason Parker (jparker) 2005-07-13 15:09:41

Can my changes to README.odbcstorage be included in the next patch as well?


By: Donny Kavanagh (donnyk) 2005-07-13 18:51:04

drmac, i tried to apply against latest head, i get.

[root@asterisk2 asterisk]# patch -p0 < vm.txt
patching file apps/app_voicemail.c
Hunk #1 succeeded at 223 (offset 11 lines).
Hunk #2 succeeded at 830 (offset 31 lines).
Hunk #3 succeeded at 1017 (offset 17 lines).
Hunk #4 succeeded at 1051 (offset 31 lines).
Hunk ASTERISK-1 succeeded at 1050 (offset 17 lines).
Hunk ASTERISK-2 succeeded at 1092 (offset 31 lines).
Hunk ASTERISK-3 succeeded at 1149 (offset 17 lines).
Hunk ASTERISK-4 succeeded at 1189 (offset 31 lines).
Hunk ASTERISK-5 succeeded at 1209 (offset 17 lines).
Hunk ASTERISK-6 succeeded at 1243 (offset 31 lines).
Hunk ASTERISK-7 succeeded at 1242 (offset 17 lines).
Hunk ASTERISK-8 FAILED at 2016.
Hunk ASTERISK-9 succeeded at 2331 (offset 54 lines).
Hunk ASTERISK-10 FAILED at 2356.
Hunk ASTERISK-11 succeeded at 2335 with fuzz 2 (offset 18 lines).
Hunk ASTERISK-12 succeeded at 2414 with fuzz 1 (offset 59 lines).
Hunk ASTERISK-13 FAILED at 3528.
Hunk ASTERISK-14 FAILED at 3550.
Hunk ASTERISK-15 succeeded at 5863 (offset 123 lines).
Hunk ASTERISK-16 succeeded at 6063 (offset 60 lines).
4 out of 20 hunks FAILED -- saving rejects to file apps/app_voicemail.c.rej

can you take a look?
By: Donny Kavanagh (donnyk) 2005-07-13 19:51:21

Ignore that last patch, it crashes when you exit the vm system... not sure why yet...
By: drmac (drmac) 2005-07-13 22:13:17

i think there were some changes to voicemail after i sent my pound patch..i see juggies applies fine and has the README and makefile changes too. but since it crashes..

i dont have odbc otherwise i'd help test. i actually re-wrote the odbc storage stuff to use native mysql commands and we use that.
By: Donny Kavanagh (donnyk) 2005-07-14 14:44:37

Theres another bug in cvs-head which is prohibiting this patch.  When that is resolved i think my patch is finished to solve this issue.  I'll post it when 4711 is resolved.


By: Donny Kavanagh (donnyk) 2005-07-14 23:22:13

4711 has been resolved, so i will post the patch to this from work tomorow.
By: Donny Kavanagh (donnyk) 2005-07-15 10:18:14

I just attached the complete patch (extendedvmodbc-07-15-2005.txt), which affects apps/app_voicemail.c, apps/Makefile & doc/README.odbcstorage and adresses all issues in this ticket.  Thanks to everyone who contributed on this one.  And north for writing the original patch.  This patches cleanly on head, and works fine in my testing.  My disclaimer will be on its way shortly.
By: Donny Kavanagh (donnyk) 2005-07-15 11:04:15

Disclaimer faxed.
By: drmac (drmac) 2005-07-15 11:11:17

i see the tablename is still hardcoded..i guess thats just a peeve of mine.
By: Donny Kavanagh (donnyk) 2005-07-15 12:40:33

I can take a stab @ that.
By: Donny Kavanagh (donnyk) 2005-07-15 13:06:35

Table name change implemented, please see doc/README.odbcstorage for documentation.

drmac, what is your nickname on irc?

extendedvmodbc-07-15-2005-tablename.txt  = latest patch.


By: Donny Kavanagh (donnyk) 2005-07-15 13:13:15

I'm dumb.

extendedvmodbc-07-15-2005-tablename-woops.txt = latest.
By: drmac (drmac) 2005-07-15 13:44:55

juggie,
check out this line in your latest patch

+ retval->maxmsg=maxmsg;

I think that was fixed in another voicemail app patch.

i use the same nick on irc but im never on it
By: Donny Kavanagh (donnyk) 2005-07-15 14:55:57

Offending line removed in latest version.

extendedvmodbc-07-15-2005-tablename-2.txt
By: Donny Kavanagh (donnyk) 2005-07-17 23:20:50

Kevin, can you review this when you get a chance.  Just want to make sure it gets into your tree for 1.2

Thanks
By: Donny Kavanagh (donnyk) 2005-07-18 18:30:03

Incorporated 4403 changes with the MWI odbcstorage related changes from 4592.

Can anyone test?

extended-vm-odbc-with-mwi.txt = latest.
By: outcast (outcast) 2005-07-20 08:40:54

is anyone planning on getting this to work with res_mysql?


By: Donny Kavanagh (donnyk) 2005-07-20 09:37:46

That may be possible, but that is not the intention of this patch.  Please test for what it is and report back.  Furthermore as i understand it, Asterisk is only supporting odbc in cvs.  (except for cdr)
By: drmac (drmac) 2005-07-20 09:54:22

i have this implemented with native mysql api. My only reason for not releasing it to the public is because we would once again have an app_voicemail with 2 database trees like we did with STABLE.

i think the ultimate goal is to adapt realtime to be able to handle binary files (ie: ast_realtime_load_binary)

then we can remove all specific database stuff from app_voicemail and trim it down a bit.


By: outcast (outcast) 2005-07-20 10:33:22

agreed, has anyone submitted the feature request for it?
By: Donny Kavanagh (donnyk) 2005-07-20 10:56:49

There was a patch for that but so far it has not been accepted, i'm not sure if that is the intention of realtime (perhaps the intention needs to be expanded) however it would be ideal to be able to do everything database through one unified database driver within *.
By: drmac (drmac) 2005-07-20 13:03:49

i searched mantis for the patch juggie, couldnt find it. you have it somewhere?
By: Donny Kavanagh (donnyk) 2005-07-20 13:16:37

Yes, as a matter of fact I do.

http://bugs.digium.com/view.php?id=4354
By: Olle Johansson (oej) 2005-07-20 13:47:15

Sent mail to mailing list, asking for help testing this feature.
By: Donny Kavanagh (donnyk) 2005-07-20 16:07:23

The MWI implementation i yanked from 4592 was broken (maybe it worked at one time?) here it is again fixed.

extended-storage-vm-mwi-07-20-05.txt = latest.
By: Jason Parker (jparker) 2005-07-20 23:53:43

Tested, works great.  Can we get a review on this?  Would be great to finally see this into cvs head.  It looks like this has grown a little bit more then was originally intended, BUT, it covers a few other bugs.
By: Donny Kavanagh (donnyk) 2005-07-21 13:29:50

drmac, outcast, you expressed some intreast can you give this a try.
By: drmac (drmac) 2005-07-21 14:30:49

i dont do odbc..sorry..cant test..
By: Michael Cramer (micc) 2005-07-21 19:02:21

I've tested this with freetds-0.63 and Microsoft SQL Server 2000. It seems to be working a little bit. To get this to work with Microsoft SQL Server you will need to use image as the datatype for recording and regular int instead of int(11).

There also seems to be an intermittent problem with freetds and SQLRowCount. It seems to set rowcount to -1 on some select statements. The solution to this seems to be to remove the || (rowcount < 1) in the error catching code after the call. And if you do that then you should also do if(rowcount > 0) instead of if(rowcount) in retrieve_file

Another note about the calls to SQLBindParameter. Shouldn't msgnum be passed as SQL_C_SLONG, SQL_INTEGER, 0, 0, &msgnum, 0, NULL); ? That is the way I am doing it. Maybe mysql isn't picky but freetds or ms sql server seems to be a little picky about its parameter types matching the data types in the database.

SQLRowCount seems to be returning a rowcount of -1 when there are actually some rows. So I think this could be an problem with freetds or unixODBC. I don't see any way to grab error information or to debug this any further without hacking into freetds so I'm going back to the filesystem until freetds comes out with a new stable version.


By: Donny Kavanagh (donnyk) 2005-07-22 13:45:51

Sounds to me like most of these problems are related to freetds.  Works as expected on mysql with unixodbc.  When i find some time i may try another database type.
By: Donny Kavanagh (donnyk) 2005-07-22 13:46:49

Could you try without the patch in 4403 (eg just cvs) see if the same problems still occur.
By: Michael Cramer (micc) 2005-07-22 13:56:15

I agree it is probably all related to freetds. I am not going to test this anymore right now. Hopefully I'll get a chance to come back to this in a month or so and do it right.
By: Donny Kavanagh (donnyk) 2005-07-22 19:10:40

Could you to humor me test without the patch here, just a clean cvs copy.  I just want to ensure the problems are freetds related, and not related to this patch.  This would help ensure this patch does not get held up on your findings.

Thanks
By: Jason Parker (jparker) 2005-07-24 15:00:36

What is it going to take to get this completed/commited before a 1.2 code freeze?  I think several of us would really like to see this in 1.2.
By: Donny Kavanagh (donnyk) 2005-07-30 01:50:37

What are we waiting on here?
By: Michael Cramer (micc) 2005-08-02 14:55:37

I was working on another project that works with SQL and I just had a thought about the RowCount problem. I haven't tested this but it might be the case. That you have to call SQLFetch before SQLRowCount. This is just a thought I just had, I have no idea if it is valid.
By: Donny Kavanagh (donnyk) 2005-08-07 10:50:36

Thats a possibility, but not related to this patch, if thats true its a problem with the entire odbc implementation not just this code.  If you can prove it, you test it, it works etc... submit a bug/patch for that issue as it is seperate.
By: Donny Kavanagh (donnyk) 2005-08-16 09:44:03

I dont know whats taking so long here, but i should also add that this is a bug fix as well.  ODBC Storage w/MWI does not work on CVS.  This patch along with adding the two fields in the table, fixes that.
By: Kevin P. Fleming (kpfleming) 2005-08-24 23:14:13

Committed to CVS HEAD with minor mod (removed extra ast_log(LOG_ERROR, ...)). Thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:45:28.000-0600

Repository: asterisk
Revision: 6404

U   trunk/apps/Makefile
U   trunk/apps/app_voicemail.c
U   trunk/doc/README.odbcstorage

------------------------------------------------------------------------
r6404 | kpfleming | 2008-01-15 15:45:27 -0600 (Tue, 15 Jan 2008) | 2 lines

add optional 'extended ODBC storage' mode (issue ASTERISK-4295)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6404