From c94368100582f7aff217ccfaa1ffd73f7057f0c6 Mon Sep 17 00:00:00 2001 From: Timo Sirainen Date: Thu, 29 Sep 2016 14:15:32 +0300 Subject: [PATCH] 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. --- src/lib-storage/mail-copy.c | 2 + src/lib-storage/mail-storage-private.h | 12 +++-- src/lib-storage/mail-storage.c | 64 +++++++++++++++++++++----- 3 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/lib-storage/mail-copy.c b/src/lib-storage/mail-copy.c index db499724d3..91df2613bc 100644 --- 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 7682738c1b..2a1da3e35e 100644 --- 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 d707210202..7bbda4ddc9 100644 --- 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)