[Home]

Summary:ASTERISK-07821: [patch] fixes for IMAP storage support
Reporter:Sergey Okhapkin (sokhapkin)Labels:
Date Opened:2006-09-26 10:11:51Date Closed:2006-11-07 12:51:51.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app_voicemail.c.diff.092906
( 1) app_voicemail.c.diff-1.4-beta2
( 2) app_voicemail.c.diff-1.4-beta2-1
( 3) app_voicemail.c.diff-1.4-beta2-2
( 4) app_voicemail.c.diff-trunk
( 5) app_voicemail.c.diff-trunk-44431
( 6) imapstorage.txt.diff
( 7) imapstorage.txt.diff.092806
( 8) imapstorage.txt.diff-1.4-beta2
( 9) vmail-patch-1.4
(10) vmail-patch-trunk
Description:IMAP voicemail storage support is broken in 1.4.0-beta2. I will use this bugreport to provide patches against beta2 to get the support back to work. The TODO list for the beta is:

1. Get it to work with most imap servers and any version of c-client.
3. Support imap servers without '/authuser' parameter support.
4. Configurable path to voicemail folder on imap server.

Additional features planned for trunk are:
2. Per-mailbox imap server configuration.
5. Configurable voicemail storage backend (files/odbc/imap).
Comments:By: Sergey Okhapkin (sokhapkin) 2006-09-26 10:20:17

The patch app_voicemail.c.diff.092606 removes calls to undefined function ast_strlen (coredump on message retreival), fixes some problems and introduces new per-mailbox parameter "imappassword". The parameter is used to access imap storage if authuser/authpassword global parameters are not set or unsupported by imap server.

The patch tested with courier imapd and MS Echange server. Leave a message and voicemailmain() now works with imap storage. Mail forward and other features not tested.

More fixes soon.
By: Serge Vecher (serge-v) 2006-09-26 10:32:37

sergey: thanks for working on this. Let's only address the bugfixes in this report (Item 1), as that's what will make it back into the 1.4 branch and [hopefully] the 1.4.x release.

Items 2-4 look like new features too me and patches will need to be provided against trunk, not the 1.4 branch. This needs to be separated into a new report.

Item 5: there is bug ASTERISK-6329, which seems to be attempting the same thing. Unfortunately, the original poster (chops) has abandonded the effort -- it would be great if you could look at the patch there, see what's still usable, and make a new patch for the latest trunk.

Thank you!
By: Sergey Okhapkin (sokhapkin) 2006-09-26 10:53:39

Sure the item 1 is the top priority, but item 3 is closely related and already implemented in the patch. I will do an additional cleanup and testing of 1 and 3 before working on other issues in trunk version.

A lot of files-related cleanup is required, currently imap backend do not remove message files sent to user's inbox and files downloaded on voicemail access from phone.
By: Sergey Okhapkin (sokhapkin) 2006-09-26 14:29:12

Oops... MWI support is broken in imap storage. Will fix for 1.4
By: Serge Vecher (serge-v) 2006-09-26 14:32:47

sokhapkin: you can also combine all fixes into one patchfile ...
By: Russell Bryant (russell) 2006-09-26 16:19:19

What is this #include "linkage.c"  ?
By: Sergey Okhapkin (sokhapkin) 2006-09-26 16:38:55

russel: see c-client documentation at http://www.washington.edu/imap/documentation/internal.txt.html

void mail_link (DRIVER *driver);
driver pointer to the driver to be added

    This function adds the specified driver to the list of mailbox
drivers.  Initially there are no drivers lunk, so all programs which
intend to use c-client need to have at least one call to this function.

    A function which uses IMAP4 would have a statement such as:
mail_link (&imapdriver); /* link in IMAP driver */
early in the program's initialization.  Normally, this is done by the
statement
#include "linkage.c"
which will include the "system standard driver linkage" defined when
c-client was built.  By using linkage.c instead of explicit mail_link()
calls, you are guaranteed that you will have a consistant linkage among
all software built on this system.
By: Dax Kelson (daxkelson) 2006-09-27 02:06:30

Please consider item #4 (Configurable path to voicemail folder on imap server) a 1.4 issue.

On Sept 15th, Kevin Fleming, talking about item 4, said:

"I thought that was already implemented; if not, it needs to be before we go to release."

http://lists.digium.com/pipermail/asterisk-dev/2006-September/023239.html
By: Serge Vecher (serge-v) 2006-09-27 09:23:03

daxkelson: thanks for the reference, that helps ...
sokhapkin: I'm updating 'the additional information' field as per discussion here.
By: Sergey Okhapkin (sokhapkin) 2006-09-27 09:57:19

New patch app_voicemail.c.diff.092706 (please remove the old one!) brings MWI back to life and has all fixes and cleanup for items 1 and 3.

I need to think more about the design of item 4, because it seems to me very tied to item 5 (need to implement more generic routines to work on top of storage backend drivers to have a complete solution). Also it would be nice to get rid of scattered through the code "#ifdef IMAP_STORAGE" directives.
By: merriam (merriam) 2006-09-27 17:00:20

The current app_voicemail seems to find and count voicemail messages by asking the imap server to search for the voicemail headers.  My understanding is it will have to examine every message, which would be slow with a large mailbox.  

The Courier documenation says, however, that it can find messages with IMAP keywords very quickly, because it maintains an index.  The C-Client code seems to support keyword searches and I believe most major IMAP servers support keyword tagging.  If the other IMAP servers also maintain keyword indexes this would seem the fast way to keep track of voicemail.
By: Sergey Okhapkin (sokhapkin) 2006-09-27 17:13:17

merriam: to my understanding keywords are applicable to message body only. Anyway, the search is performed by imap server, but not the client.
By: merriam (merriam) 2006-09-27 18:16:49

I have 5000 emails in my INBOX.  Whether Courier searches those or Asterisk searches those it will take a while.  Clearly putting all of your voicemails into a seperate mailbox goes a long way toward speeding that up.  I only have 10 old voicemails.  

I poked around in the Courier documentation trying to figure out how to make the authuser concept work and ran across keywords in the process.  Whoever designed IMAP keywords extension must have intended us to use them.  Here is what Courier says about it.

http://www.courier-mta.org/imap/?README.imapkeywords.html
By: Sergey Okhapkin (sokhapkin) 2006-09-28 11:31:27

New patches add global configuration parameter "imapfolder" to specify IMAP folder to store voicemail messages instead of INBOX. Items 1,3 and 4 completed. The only remaining enchancement for item 4 is to not use "sendmail" to deliver voicemail message to inbox, but to use native IMAP commands.

merriam: you don't need to scan 5000 messages in inbox now, just use another imap folder for voicemail:-)
By: Sergey Okhapkin (sokhapkin) 2006-09-29 20:21:29

More cleanup and merge of imap and main code. It still works:-) The lesser differences, the better. No native mail_append yet...


Please delete old app_voicemail patches.
By: Leif Madsen (lmadsen) 2006-09-30 09:46:44

Old app_voicemail files deleted upon request
By: Sergey Okhapkin (sokhapkin) 2006-10-01 08:45:53

Here they are. The last two patches for app_voicemail and documentation are "supposed-to-be-final":-) for Asterisk-1.4-beta2. The patch implements

1. Get it to work with most imap servers and any version of c-client.
3. Support imap servers without '/authuser' parameter support.
4. Configurable path to voicemail folder on imap server.

As suggested, I will work on remaining new features in trunk asterisk version.

The final patch files are imapstorage.txt.diff-1.4-beta2 and app_voicemail.c.diff-1.4-beta2-1 (had to reupload because I found broken files-based storage introduced by my patch). Other patch files can be deleted.

As usual, bug reports are welcome:-)
By: Sergey Okhapkin (sokhapkin) 2006-10-01 10:31:52

Uploaded a corresponding patch against asterisk-trunk.
By: Sergey Okhapkin (sokhapkin) 2006-10-02 08:39:19

Added fix to address issue 8016.
By: Matt Selsky (selsky) 2006-10-04 18:34:11

Do you have an updated patch for trunk?  The last patch no longer applies cleanly.
By: Sergey Okhapkin (sokhapkin) 2006-10-04 20:13:43

trunk version of the patch updated to latest trunk.
By: Serge Vecher (serge-v) 2006-10-05 08:26:20

to everybody who is testing this patch -- lack of feedback is what's preventing this patch from being merged into the 1.4/trunk...
By: jerjer (jerjer) 2006-10-22 23:01:08

Lets spend a few clock cycles at Astricon to test.
By: Matt Selsky (selsky) 2006-10-27 12:20:44

I'm using 1.4.0 beta2 (since the latest beta patch doesn't apply to beta3) with Cyrus IMAPd.

When an Asterisk user tries to leave a VM for another user, the context doesn't seem to be used.  Only the "default" context is working.  When checking VM, the context is used properly as defined.
By: Matt O'Gorman (mogorman) 2006-10-31 16:18:08.000-0600

updated with some fixes, can people please look over and respond so i can be more sure in committing.

mog
By: Sergey Okhapkin (sokhapkin) 2006-10-31 16:42:46.000-0600

mogorman: I'm not sure if delimiter_lock stuff is really needed. The delimiter variable will be moved to struct ast_vm_user in the future to implement per-user or group imap server configuration.
By: Matt O'Gorman (mogorman) 2006-10-31 16:48:24.000-0600

as a private variable I would agree. But It seems possible in the once instance it is altered multiple things could alter at same time.  thus we must protect it.
By: Sergey Okhapkin (sokhapkin) 2006-10-31 16:59:25.000-0600

Even if multiple instances will alter the variable, all instances will alter it to the same value, because all instances will update it to the same imap server. As soon as the variable is altered, the check "if(delimiter == '\0')" in init_mailstream will do the work.
By: Matt O'Gorman (mogorman) 2006-11-01 10:34:26.000-0600

well if need be we can take it out, i just went over the global variables we have added and just looked where we could have  a threading conflict.  2 threads could enter that section of code at same time and both could change variable , or thats how it looks to me.  I just took rode of better safe than sorry.

How is testing going?

Mog
By: Matt O'Gorman (mogorman) 2006-11-07 12:51:47.000-0600

its been committed, please continue to test and report bugs.
thanks

mog