lib-storage: Don't reset mail_save_context.saving too early.

If mailbox_save_using_mail() ended up in mail_storage_copy(), saving was set
to FALSE before the copy() method was finished running. This caused e.g.
notify plugin to think that this was a copy event instead of a save event.

Added comments and asserts to clarify how the logic should work between
all the different copying/moving/saving flags.

diff --git a/src/lib-storage/mail-copy.c b/src/lib-storage/mail-copy.c
index db499724d36a8e8980030be3dd2815d1a58a2948..91df2613bc08f2f381f662929d7d4e7c37edeb74 100644 (file)
--- a/src/lib-storage/mail-copy.c
+++ b/src/lib-storage/mail-copy.c
@@ -92,6 +92,8 @@ mail_storage_try_copy(struct mail_save_context **_ctx, struct mail *mail)
 
 int mail_storage_copy(struct mail_save_context *ctx, struct mail *mail)
 {
+       i_assert(ctx->copying_or_moving);
+
        if (ctx->data.keywords != NULL) {
                /* keywords gets unreferenced twice: first in
                   mailbox_save_cancel()/_finish() and second time in

diff --git a/src/lib-storage/mail-storage-private.h b/src/lib-storage/mail-storage-private.h
index 7682738c1bc2558cbb5d2885b0764357252ab3c3..2a1da3e35ea8baaba6e01298c501cf94a3f4aa8d 100644 (file)
--- a/src/lib-storage/mail-storage-private.h
+++ b/src/lib-storage/mail-storage-private.h
@@ -643,12 +643,18 @@ struct mail_save_context {
        unsigned int unfinished:1;
        /* mailbox_save_finish() or mailbox_copy() is being called. */
        unsigned int finishing:1;
-       /* mail was copied using saving */
+       /* mail was copied or moved using saving (requires:
+          copying_or_moving==TRUE). */
        unsigned int copying_via_save:1;
-       /* mail is being saved, not copied */
+       /* mail is being saved, not copied. However, this is set also with
+          mailbox_save_using_mail() and then copying_or_moving==TRUE. */
        unsigned int saving:1;
-       /* mail is being moved - ignore quota */
+       /* mail is being moved - ignore quota (requires:
+          copying_or_moving==TRUE && saving==FALSE). */
        unsigned int moving:1;
+       /* mail is being copied or moved. However, this is set also with
+          mailbox_save_using_mail() and then saving==TRUE. */
+       unsigned int copying_or_moving:1;
 };
 
 struct mailbox_sync_context {

diff --git a/src/lib-storage/mail-storage.c b/src/lib-storage/mail-storage.c
index d7072102027921d0b198358d81336d896c192872..7bbda4ddc9150a07089fb038b72684d67320c881 100644 (file)
--- a/src/lib-storage/mail-storage.c
+++ b/src/lib-storage/mail-storage.c
@@ -2081,8 +2081,15 @@ int mailbox_save_begin(struct mail_save_context **ctx, struct istream *input)
                return -1;
        }
 
-       if (!(*ctx)->copying_via_save)
+       if (!(*ctx)->copying_or_moving) {
+               /* We're actually saving the mail. We're not being called by
+                  mail_storage_copy() because backend didn't support fast
+                  copying. */
+               i_assert(!(*ctx)->copying_via_save);
                (*ctx)->saving = TRUE;
+       } else {
+               i_assert((*ctx)->copying_via_save);
+       }
        if (box->v.save_begin == NULL) {
                mail_storage_set_error(box->storage, MAIL_ERROR_NOTPOSSIBLE,
                                       "Saving messages not supported");
@@ -2121,6 +2128,23 @@ mailbox_save_add_pvt_flags(struct mailbox_transaction_context *t,
        save->flags = pvt_flags;
 }
 
+static void mailbox_save_context_reset(struct mail_save_context *ctx)
+{
+       i_assert(!ctx->unfinished);
+       if (!ctx->copying_or_moving) {
+               /* we're finishing a save (not copy/move) */
+               i_assert(!ctx->copying_via_save);
+               i_assert(ctx->saving);
+               ctx->saving = FALSE;
+       } else {
+               i_assert(ctx->copying_via_save);
+               /* We came from mailbox_copy(). saving==TRUE is possible here
+                  if we also came from mailbox_save_using_mail(). Don't set
+                  saving=FALSE yet in that case, because copy() is still
+                  running. */
+       }
+}
+
 int mailbox_save_finish(struct mail_save_context **_ctx)
 {
        struct mail_save_context *ctx = *_ctx;
@@ -2154,8 +2178,7 @@ int mailbox_save_finish(struct mail_save_context **_ctx)
        }
        if (keywords != NULL)
                mailbox_keywords_unref(&keywords);
-       i_assert(!ctx->unfinished);
-       ctx->saving = FALSE;
+       mailbox_save_context_reset(ctx);
        return ret;
 }
 
@@ -2178,8 +2201,7 @@ void mailbox_save_cancel(struct mail_save_context **_ctx)
                mail = (struct mail_private *)ctx->dest_mail;
                mail->v.close(&mail->mail);
        }
-       i_assert(!ctx->unfinished);
-       ctx->saving = FALSE;
+       mailbox_save_context_reset(ctx);
 }
 
 struct mailbox_transaction_context *
@@ -2188,7 +2210,7 @@ mailbox_save_get_transaction(struct mail_save_context *ctx)
        return ctx->transaction;
 }
 
-int mailbox_copy(struct mail_save_context **_ctx, struct mail *mail)
+static int mailbox_copy_int(struct mail_save_context **_ctx, struct mail *mail)
 {
        struct mail_save_context *ctx = *_ctx;
        struct mailbox_transaction_context *t = ctx->transaction;
@@ -2211,6 +2233,9 @@ int mailbox_copy(struct mail_save_context **_ctx, struct mail *mail)
                mailbox_save_cancel(&ctx);
                return -1;
        }
+
+       i_assert(!ctx->copying_or_moving);
+       ctx->copying_or_moving = TRUE;
        ctx->finishing = TRUE;
        T_BEGIN {
                ret = t->box->v.copy(ctx, backend_mail);
@@ -2226,28 +2251,45 @@ int mailbox_copy(struct mail_save_context **_ctx, struct mail *mail)
        i_assert(!ctx->unfinished);
 
        ctx->copying_via_save = FALSE;
-       ctx->saving = FALSE;
+       ctx->copying_or_moving = FALSE;
+       ctx->saving = FALSE; /* if we came from mailbox_save_using_mail() */
        return ret;
 }
 
+int mailbox_copy(struct mail_save_context **_ctx, struct mail *mail)
+{
+       struct mail_save_context *ctx = *_ctx;
+
+       i_assert(!ctx->saving);
+       i_assert(!ctx->moving);
+
+       return mailbox_copy_int(_ctx, mail);
+}
+
 int mailbox_move(struct mail_save_context **_ctx, struct mail *mail)
 {
        struct mail_save_context *ctx = *_ctx;
        int ret;
 
+       i_assert(!ctx->saving);
        i_assert(!ctx->moving);
 
        ctx->moving = TRUE;
-       if ((ret = mailbox_copy(_ctx, mail)) == 0)
+       if ((ret = mailbox_copy_int(_ctx, mail)) == 0)
                mail_expunge(mail);
        ctx->moving = FALSE;
        return ret;
 }
 
-int mailbox_save_using_mail(struct mail_save_context **ctx, struct mail *mail)
+int mailbox_save_using_mail(struct mail_save_context **_ctx, struct mail *mail)
 {
-       (*ctx)->saving = TRUE;
-       return mailbox_copy(ctx, mail);
+       struct mail_save_context *ctx = *_ctx;
+
+       i_assert(!ctx->saving);
+       i_assert(!ctx->moving);
+
+       ctx->saving = TRUE;
+       return mailbox_copy_int(_ctx, mail);
 }
 
 bool mailbox_is_inconsistent(struct mailbox *box)