CORRECT PATCH Re: sync_client bails when encountering a deleted
message
John Capo
jc at irbs.com
Tue May 15 19:19:15 EDT 2007
Quoting Ken Murchison (murch at andrew.cmu.edu):
> I don't think I was clear. With my proposal, we're well past "UPDATE". I'm talking about cutting the command short after "(<flags>)". No header_size or anything after.
I guess I don't understand what you mean.
It sure looks to me like cmd_upload() has to detect that the header
size it is waiting for will not arrive and recover gracefully rather
than sending a BAD that the client sees as the response for its
next command. Patch attached.
John Capo
Tuffmail.com
>
> (from my phone)
> --
> Kenneth Murchison
> Systems Programmer
> Project Cyrus Developer/Maintainer
> Carnegie Mellon University
>
> -----Original Message-----
> From: "John Capo" <jc at irbs.com>
> To: "Ken Murchison" <murch at andrew.cmu.edu>
> Cc: cyrus-devel at lists.andrew.cmu.edu
> Sent: 5/15/07 3:34 PM
> Subject: Re: CORRECT PATCH Re: sync_client bails when encountering a deleted message
>
> Quoting Ken Murchison (murch at andrew.cmu.edu):
> > John Capo wrote:
> > >Quoting Ken Murchison (murch at andrew.cmu.edu):
> > >>Ken Murchison wrote:
> > >>>Obviously, the chances of header_size being 0xdeadbeef is remote, but it
> > >>>is possible. Would it make more sense to use ULONG_MAX as the "failure
> > >>>size"?
> > >>Or better yet, how about just using 0 (zero)? IIRC, RFC2822 stipulates
> > >>that the message header has to be non-zero (Date and From are mandatory)
> > >
> > >I have seen zero size messages created with IMAP uploads from desktop
> > >clients. This is probably a bug elsewhere. I do not know if the
> > >zero size messages were replicated.
> > >
> > >Sending more than one magic number would be the safest way. The
> > >value of the second magic number could be used to signal other
> > >conditions if needed. I can't imagine what that would be other
> > >than aborting the upload. Two ULONG_MAX is a row can't be a valid
> > >message.
> >
> > After thinking about this some more,do we even need a magic number? If
> > we don't send anything after the flags list, shouldn't this be enough to
> > signal that we have an invalid/missing message?
>
> We still need to see what's next in the stream if anything. When
> upload_message_work() returns due to not being able to open a
> message, the next byte in the stream should be the first letter of
> a command. Testing the next character returned by prot_getc() for
> alpha or numeric could signal an abort. If its alpha then its the
> next command. If its numeric then its the header size. prot_ungetc()
> the character and return or fetch the header size from the stream
> and continue.
>
> If the connection is closed prot_getc() would return EOF and
> cmd_upload() would return to cmd_loop(). cmd_loop() would also see
> EOF since its a flag in the prot structure. Not sure how to test
> the EOF case.
>
> I'll see if the basic idea works later today.
>
> John Capo
>
-------------- next part --------------
Common subdirectories: cyrus-imapd-2.3.7/imap/CVS and cyrus-imapd-2.3.7-abort-upload/imap/CVS
diff -u cyrus-imapd-2.3.7/imap/sync_client.c cyrus-imapd-2.3.7-abort-upload/imap/sync_client.c
--- cyrus-imapd-2.3.7/imap/sync_client.c Sun May 13 11:56:58 2007
+++ cyrus-imapd-2.3.7-abort-upload/imap/sync_client.c Tue May 15 19:08:07 2007
@@ -1179,6 +1179,9 @@
if (r) {
syslog(LOG_ERR, "IOERROR: opening message file %lu of %s: %m",
record->uid, mailbox->name);
+ prot_printf(toserver, " "); /* A space so flags parsing completes */
+ prot_flush(toserver);
+ sync_msgid_remove(msgid_onserver, &record->uuid);
return(IMAP_IOERROR);
}
diff -u cyrus-imapd-2.3.7/imap/sync_server.c cyrus-imapd-2.3.7-abort-upload/imap/sync_server.c
--- cyrus-imapd-2.3.7/imap/sync_server.c Sun May 13 11:56:58 2007
+++ cyrus-imapd-2.3.7-abort-upload/imap/sync_server.c Tue May 15 18:36:46 2007
@@ -1870,13 +1870,24 @@
goto parse_err;
}
- message = sync_message_add(message_list, &item->uuid);
+ if ((c = prot_getc(sync_in)) == EOF || !isdigit(c))
+ {
+ /* Make sure cache data flushed to disk before we commit */
+ sync_message_fsync(message_list);
+ sync_message_list_cache_flush(message_list);
+ sync_upload_list_free(&upload_list);
+ syslog(LOG_ERR, "IOERROR: UPLOAD ABORT");
+ return;
+ }
+
+ prot_ungetc(c, sync_in);
/* Parse Message (header size, content lines, cache, message body */
if ((c = getastring(sync_in, sync_out, &arg)) != ' ') {
err = "Invalid Header Size";
goto parse_err;
}
+ message = sync_message_add(message_list, &item->uuid);
message->hdr_size = sync_atoul(arg.s);
if ((c = getastring(sync_in, sync_out, &arg)) != ' ') {
diff -u cyrus-imapd-2.3.7/imap/sync_support.c cyrus-imapd-2.3.7-abort-upload/imap/sync_support.c
--- cyrus-imapd-2.3.7/imap/sync_support.c Sun May 13 11:56:59 2007
+++ cyrus-imapd-2.3.7-abort-upload/imap/sync_support.c Sun May 13 12:03:25 2007
@@ -544,6 +544,25 @@
return(NULL);
}
+struct sync_msgid *sync_msgid_remove(struct sync_msgid_list *l,
+ struct message_uuid *uuid)
+{
+ int offset = message_uuid_hash(uuid, l->hash_size);
+ struct sync_msgid *msgid;
+
+ if (message_uuid_isnull(uuid))
+ return(NULL);
+
+ for (msgid = l->hash[offset] ; msgid ; msgid = msgid->hash_next) {
+ if (message_uuid_compare(&msgid->uuid, uuid))
+ {
+ msgid->uuid.value[0] = 0;
+ return;
+ }
+ }
+ return(NULL);
+}
+
/* ====================================================================== */
struct sync_folder_list *sync_folder_list_create(void)
diff -u cyrus-imapd-2.3.7/imap/sync_support.h cyrus-imapd-2.3.7-abort-upload/imap/sync_support.h
--- cyrus-imapd-2.3.7/imap/sync_support.h Sun May 13 11:56:59 2007
+++ cyrus-imapd-2.3.7-abort-upload/imap/sync_support.h Sun May 13 12:11:21 2007
@@ -143,6 +143,9 @@
struct sync_msgid *sync_msgid_add(struct sync_msgid_list *list,
struct message_uuid *uuid);
+struct sync_msgid *sync_msgid_remove(struct sync_msgid_list *list,
+ struct message_uuid *uuid);
+
struct sync_msgid *sync_msgid_lookup(struct sync_msgid_list *list,
struct message_uuid *uuid);
More information about the Cyrus-devel
mailing list