Re: Need advice: mailbox-based \Seen flag
I just committed a patch with your Perl changes and (hopefully) support for replication. Boris Lytochkin wrote: Ken, Thanks for patch! Here it is full patch (with cyradm patch additions) that works OK on my imapd 2.3.9 setup. I can not check replication process for now. Here it is some logs: cyradm: localhost> info user.rootmail {user.rootmail}: condstore: false lastpop: lastupdate: 2-Sep-2007 13:57:45 +0400 partition: default sharedseen: true size: 18220 localhost> info user.lytochkin {user.lytochkin}: condstore: false lastpop: lastupdate: 1-Sep-2007 08:11:08 +0400 partition: default sharedseen: false size: 61128298 local6.debug: Sep 2 13:57:45 svr imaps[7810]: seen_db: user anyone opened /var/imap/user/a/anyone.seen -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: Need advice: mailbox-based \Seen flag
I had some time while waiting for paint to dry, so I whipped up this quick patch for shared seen state, based on our previous discussions. It uses a "/vendor/cmu/cyrus-imapd/sharedseen" mailbox annotation, stored as a mailbox option in the index header, to enable/disable shared seen state, and stores the shared seen state in "anyone.seen". Note that this patch is completely untested, although it compiles against CVS. I also didn't consider replication yet. Feel free to try this and see if it has the intended behavior. Remember that the 's' ACL right controls whether a person can change the \Seen flag on a message. Boris Lytochkin wrote: Hello! I need an advice in implementing per-mailbox \Seen flag (or 'shared' \Seen flag in per-user basis). For now I want to implement it this way: 1) add a new mailbox attribute, say 'sharedseen' 2) switch path for .seen file in imap/seen_*.c:seen_getpath() if 'sharedseen' flag is set on mailbox. 3) changes in replication module? So, setting 'sharedseen' attribute to mailbox will cause using per-mailbox seen-file. What are weak points of this implementation? Is there more correct way to make per-mailbox \Seen flag? -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University Index: annotate.c === RCS file: /afs/andrew/system/cvs/src/cyrus/imap/annotate.c,v retrieving revision 1.36 diff -u -r1.36 annotate.c --- annotate.c 15 Aug 2007 17:20:55 - 1.36 +++ annotate.c 1 Sep 2007 18:00:26 - @@ -869,20 +869,20 @@ output_entryatt(ext_mboxname, entry, "", &attrib, fdata); } -static void annotation_get_condstore(const char *int_mboxname, - const char *ext_mboxname, - const char *entry, - struct fetchdata *fdata, - struct mailbox_annotation_rock *mbrock, - void *rock __attribute__((unused))) +static void annotation_get_mailboxopt(const char *int_mboxname, + const char *ext_mboxname, + const char *entry, + struct fetchdata *fdata, + struct mailbox_annotation_rock *mbrock, + void *rock __attribute__((unused))) { struct mailbox mailbox; -int r = 0; +int flag, r = 0; char value[40]; struct annotation_data attrib; -if(!int_mboxname || !ext_mboxname || !fdata || !mbrock) - fatal("annotation_get_condstore called with bad parameters", +if(!int_mboxname || !ext_mboxname || !entry || !fdata || !mbrock) + fatal("annotation_get_mailboxopt called with bad parameters", EC_TEMPFAIL); get_mb_data(int_mboxname, mbrock); @@ -890,6 +890,15 @@ /* Make sure its a local mailbox */ if (mbrock->server) return; +/* Check entry */ +if (!strcmp(entry, "/vendor/cmu/cyrus-imapd/condstore")) { + flag = OPT_IMAP_CONDSTORE; +} else if (!strcmp(entry, "/vendor/cmu/cyrus-imapd/sharedseen")) { + flag = OPT_IMAP_SHAREDSEEN; +} else { + return; +} + /* Check ACL */ if(!fdata->isadmin && (!mbrock->acl || @@ -906,7 +915,7 @@ } if (!r) { - if (mailbox.options & OPT_IMAP_CONDSTORE) { + if (mailbox.options & flag) { strcpy(value, "true"); } else { strcpy(value, "false"); @@ -1017,7 +1026,9 @@ { "/vendor/cmu/cyrus-imapd/lastpop", BACKEND_ONLY, annotation_get_lastpop, NULL }, { "/vendor/cmu/cyrus-imapd/condstore", BACKEND_ONLY, - annotation_get_condstore, NULL }, + annotation_get_mailboxopt, NULL }, +{ "/vendor/cmu/cyrus-imapd/sharedseen", BACKEND_ONLY, + annotation_get_mailboxopt, NULL }, { NULL, ANNOTATION_PROXY_T_INVALID, NULL, NULL } }; @@ -1694,14 +1705,23 @@ return r; } -static int annotation_set_condstore(const char *int_mboxname, -struct annotate_st_entry_list *entry, -struct storedata *sdata, -struct mailbox_annotation_rock *mbrock, -void *rock __attribute__((unused))) +static int annotation_set_mailboxopt(const char *int_mboxname, + struct annotate_st_entry_list *entry, + struct storedata *sdata, + struct mailbox_annotation_rock *mbrock, + void *rock __attribute__((unused))) { struct mailbox mailbox; -int r = 0; +int flag, r = 0; + +/* Check entry */ +if (!strcmp(entry->entry->name, "/vendor/cmu/cyrus-imapd/condstore")) { + flag = OPT_IMAP_CONDSTORE; +} else if (!strcmp(entry->entry->name, "/vendor/cmu/cyrus-imapd/sharedseen")) { + flag = OPT_IMAP_SHAREDSEEN; +} else { + return IMAP_PERMISSION_DENIED; +} /* Check ACL */ if(!sdata->isadmin && @@ -1717,9 +1737,9 @@ if (!r) { if (!strcmp(entry->shared.value, "true")) { - mailbox.options |= OPT_IMAP_CONDSTORE; + mailbox.options |= flag; } else { - mailbox.options &= ~OPT_IMAP_CONDSTORE; + mailbox.options &= ~flag; } r = mailbox_write_index_header(&mailbox); @@ -1793,7 +1813,10 @
Re: Need advice: mailbox-based \Seen flag
I would really be interested in that feature. It can make the support of strong groupware while the 'spooling' of an email message in a shared mailbox is managed by a group of several people. What's \seen and what's not \seen will be used by the group of people working on that mailbox to know if someone has already get in charge the management of the tasks required for that specific email. For this way of working a shared \seen would be really and seriously wellcome. I will be available for testing as you require because i am really interested in that feature. Fabio Pietrosanti Boris Lytochkin wrote: > Hello! > > I need an advice in implementing per-mailbox \Seen flag (or 'shared' > \Seen flag in per-user basis). > > For now I want to implement it this way: > 1) add a new mailbox attribute, say 'sharedseen' > 2) switch path for .seen file in imap/seen_*.c:seen_getpath() if >'sharedseen' flag is set on mailbox. > 3) changes in replication module? > > So, setting 'sharedseen' attribute to mailbox will cause using > per-mailbox seen-file. > > What are weak points of this implementation? > Is there more correct way to make per-mailbox \Seen flag? > > >
Re: Need advice: mailbox-based \Seen flag
Boris Lytochkin wrote: - When a mailbox is opened that has this annotation enabled, we open the seen state database for the special 'anyone' user instead of the authorized user. This would allow using the same API with minimal changes. Perhaps we could come up with a better userid then 'anyone' but its already reserved as special via ACL. It will work as long as we want to use shared folder for every user. As soon as we want to restrict access to a set of users, this would not work. That's what the 's' right is for. Only give 's' to those users that you want to be able to mark messages as seen. Ah, 'anyone' will be used only to store per-mailbox seen records. (Sorry for misunderstanding, I was still sleeping at the moment. :-) Where to store this file? Again, what about replication? The same place as the rest of the per-user seen databases. It *should* replicate fine, but I haven't thought it through. -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: Need advice: mailbox-based \Seen flag
Boris Lytochkin wrote: My quick thoughts would be the following: - Add a 'sharedseen' mailbox attribute as you suggest (as a mailbox annotation) - When a mailbox is opened that has this annotation enabled, we open the seen state database for the special 'anyone' user instead of the authorized user. This would allow using the same API with minimal changes. Perhaps we could come up with a better userid then 'anyone' but its already reserved as special via ACL. It will work as long as we want to use shared folder for every user. As soon as we want to restrict access to a set of users, this would not work. That's what the 's' right is for. Only give 's' to those users that you want to be able to mark messages as seen. Let me explain why I need this. There is an account that gets all root and robot mail. So shared \Seen would have meaning of 'this mail has been read by admins'. That is why I need to restrict access to these shared folders. -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: Need advice: mailbox-based \Seen flag
Rob Mueller wrote: I actually went digging into seen state and cyrus.index a little today for another reason, and I realized that we wouldn't have to expand/upgrade the index file to accomodate a shared seen state. The existing field for system flags is just a bitmask, so we could easily add a bit for shared \Seen. We could also implement the sharedseen annotation as a mailbox flag in the index header, just like condstore. Based on other changes that I'm thinking about, I'm starting to like this idea better, even thought it *may* involve more code. Doesn't that just seem annoying having two *completely* different ways of storing seen state? There' probably quite a few places in the code that look for the seen state. It just seems much easier to me to have a single flag that controls whether to open a specific users seen db, or the "anyone" user seen db, rather than having to special case lots of code around the place for either pulling from the seen db, or looking up a bit in the index file. Yeah, I talked myself out of this shortly after I sent the message. -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: Need advice: mailbox-based \Seen flag
I actually went digging into seen state and cyrus.index a little today for another reason, and I realized that we wouldn't have to expand/upgrade the index file to accomodate a shared seen state. The existing field for system flags is just a bitmask, so we could easily add a bit for shared \Seen. We could also implement the sharedseen annotation as a mailbox flag in the index header, just like condstore. Based on other changes that I'm thinking about, I'm starting to like this idea better, even thought it *may* involve more code. Doesn't that just seem annoying having two *completely* different ways of storing seen state? There' probably quite a few places in the code that look for the seen state. It just seems much easier to me to have a single flag that controls whether to open a specific users seen db, or the "anyone" user seen db, rather than having to special case lots of code around the place for either pulling from the seen db, or looking up a bit in the index file. Rob
Re: Need advice: mailbox-based \Seen flag
Ken Murchison wrote: Boris Lytochkin wrote: Hello! I need an advice in implementing per-mailbox \Seen flag (or 'shared' \Seen flag in per-user basis). For now I want to implement it this way: 1) add a new mailbox attribute, say 'sharedseen' 2) switch path for .seen file in imap/seen_*.c:seen_getpath() if 'sharedseen' flag is set on mailbox. 3) changes in replication module? So, setting 'sharedseen' attribute to mailbox will cause using per-mailbox seen-file. What are weak points of this implementation? Is there more correct way to make per-mailbox \Seen flag? My quick thoughts would be the following: - Add a 'sharedseen' mailbox attribute as you suggest (as a mailbox annotation) - When a mailbox is opened that has this annotation enabled, we open the seen state database for the special 'anyone' user instead of the authorized user. This would allow using the same API with minimal changes. Perhaps we could come up with a better userid then 'anyone' but its already reserved as special via ACL. - - As an alternative to using a seen db for 'anyone' we could go back to using a local seen db in the mailbox folder, but then we'd have to have function pointers to swap between the functions for local access vs per-user access. - As another alternative, we could keep track of shared seen state in cyrus.index, but that would require an upgrade to the index file format, and more API changes. I actually went digging into seen state and cyrus.index a little today for another reason, and I realized that we wouldn't have to expand/upgrade the index file to accomodate a shared seen state. The existing field for system flags is just a bitmask, so we could easily add a bit for shared \Seen. We could also implement the sharedseen annotation as a mailbox flag in the index header, just like condstore. Based on other changes that I'm thinking about, I'm starting to like this idea better, even thought it *may* involve more code. -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: Need advice: mailbox-based \Seen flag
Boris Lytochkin wrote: Hello! I need an advice in implementing per-mailbox \Seen flag (or 'shared' \Seen flag in per-user basis). For now I want to implement it this way: 1) add a new mailbox attribute, say 'sharedseen' 2) switch path for .seen file in imap/seen_*.c:seen_getpath() if 'sharedseen' flag is set on mailbox. 3) changes in replication module? So, setting 'sharedseen' attribute to mailbox will cause using per-mailbox seen-file. What are weak points of this implementation? Is there more correct way to make per-mailbox \Seen flag? My quick thoughts would be the following: - Add a 'sharedseen' mailbox attribute as you suggest (as a mailbox annotation) - When a mailbox is opened that has this annotation enabled, we open the seen state database for the special 'anyone' user instead of the authorized user. This would allow using the same API with minimal changes. Perhaps we could come up with a better userid then 'anyone' but its already reserved as special via ACL. - - As an alternative to using a seen db for 'anyone' we could go back to using a local seen db in the mailbox folder, but then we'd have to have function pointers to swap between the functions for local access vs per-user access. - As another alternative, we could keep track of shared seen state in cyrus.index, but that would require an upgrade to the index file format, and more API changes. Any thoughts from the gallery on my quick OTH idea? -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University