Mercurial > dovecot > core-2.2
changeset 20805:073625e2d619
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.
author | Timo Sirainen <timo.sirainen@dovecot.fi> |
---|---|
date | Thu, 29 Sep 2016 14:15:32 +0300 |
parents | 5332b9d8315c |
children | 096e9dea4145 |
files | src/lib-storage/mail-copy.c src/lib-storage/mail-storage-private.h src/lib-storage/mail-storage.c |
diffstat | 3 files changed, 64 insertions(+), 14 deletions(-) [+] |
line wrap: on
line diff
--- a/src/lib-storage/mail-copy.c Thu Sep 29 14:00:49 2016 +0300 +++ b/src/lib-storage/mail-copy.c Thu Sep 29 14:15:32 2016 +0300 @@ -92,6 +92,8 @@ 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
--- a/src/lib-storage/mail-storage-private.h Thu Sep 29 14:00:49 2016 +0300 +++ b/src/lib-storage/mail-storage-private.h Thu Sep 29 14:15:32 2016 +0300 @@ -643,12 +643,18 @@ 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 {
--- a/src/lib-storage/mail-storage.c Thu Sep 29 14:00:49 2016 +0300 +++ b/src/lib-storage/mail-storage.c Thu Sep 29 14:15:32 2016 +0300 @@ -2081,8 +2081,15 @@ 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 @@ 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 @@ } 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 @@ 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 @@ 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 @@ 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 @@ 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)