lib-storage: Report critical mail errors without redundant mail prefix

Don't report mail prefix unless it's different from last tracked mail
uid, similar to mailbox_get_last_internal_error().

diff --git a/src/lib-storage/mail-storage-private.h b/src/lib-storage/mail-storage-private.h
index d8bc614cf66376c5e3c2f3213b4ef8708e6d9360..61096de2542ed7d442ced7a6fd7aae121ca0f8eb 100644 (file)
--- a/src/lib-storage/mail-storage-private.h
+++ b/src/lib-storage/mail-storage-private.h
@@ -130,6 +130,10 @@ struct mail_storage_error {
        enum mail_error error;
        char *last_internal_error;
        char *last_internal_error_mailbox;
+       /* Note: This is the uid of the mail that triggered the error. Use
+                UINT32_MAX to denote 'unset', as 0 is used for mails currently
+                being saved. */
+       uint32_t last_internal_error_mail_uid;
        bool last_error_is_internal;
 };
 
@@ -166,6 +170,7 @@ struct mail_storage {
        /* Last error set in mail_storage_set_critical(). */
        char *last_internal_error;
        char *last_internal_error_mailbox;
+       uint32_t last_internal_error_mail_uid;
 
        char *error_string;
        enum mail_error error;

diff --git a/src/lib-storage/mail-storage.c b/src/lib-storage/mail-storage.c
index 512ac683d6ba9eaacbd1b4a59daa844826c11a12..57731820dfb8ae35a93df21f01a1a15243a408cf 100644 (file)
--- a/src/lib-storage/mail-storage.c
+++ b/src/lib-storage/mail-storage.c
@@ -428,6 +428,10 @@ mail_storage_create_full_real(struct mail_namespace *ns, const char *driver,
        storage->user = ns->user;
        storage->set = ns->mail_set;
        storage->flags = flags;
+       /* Set to UINT32_MAX manually to denote 'unset', as the default 0 is
+          used for mails currently being saved. */
+       storage->last_internal_error_mail_uid = UINT32_MAX;
+
        storage->event = event_create(ns->user->event);
        if (storage_class->event_category != NULL)
                event_add_category(storage->event, storage_class->event_category);
@@ -560,6 +564,7 @@ void mail_storage_clear_error(struct mail_storage *storage)
        i_free(storage->last_internal_error_mailbox);
        storage->last_error_is_internal = FALSE;
        storage->error = MAIL_ERROR_NONE;
+       storage->last_internal_error_mail_uid = UINT32_MAX;
 }
 
 void mail_storage_set_error(struct mail_storage *storage,
@@ -571,6 +576,7 @@ void mail_storage_set_error(struct mail_storage *storage,
        }
        storage->last_error_is_internal = FALSE;
        storage->error = error;
+       storage->last_internal_error_mail_uid = UINT32_MAX;
 }
 
 void mail_storage_set_internal_error(struct mail_storage *storage)
@@ -588,11 +594,12 @@ void mail_storage_set_internal_error(struct mail_storage *storage)
        storage->last_error_is_internal = FALSE;
        i_free(storage->last_internal_error);
        i_free(storage->last_internal_error_mailbox);
+       storage->last_internal_error_mail_uid = UINT32_MAX;
 }
 
 static void
 mail_storage_set_critical_error(struct mail_storage *storage, const char *str,
-                               const char *mailbox_vname)
+                               const char *mailbox_vname, uint32_t mail_uid)
 {
        char *old_error = storage->error_string;
        char *old_internal_error = storage->last_internal_error;
@@ -608,6 +615,7 @@ mail_storage_set_critical_error(struct mail_storage *storage, const char *str,
 
        storage->last_internal_error = i_strdup(str);
        storage->last_internal_error_mailbox = i_strdup(mailbox_vname);
+       storage->last_internal_error_mail_uid = mail_uid;
        storage->last_error_is_internal = TRUE;
 
        /* free the old_error and old_internal_error only after the new error
@@ -625,7 +633,7 @@ void mail_storage_set_critical(struct mail_storage *storage,
        va_start(va, fmt);
        T_BEGIN {
                const char *str = t_strdup_vprintf(fmt, va);
-               mail_storage_set_critical_error(storage, str, NULL);
+               mail_storage_set_critical_error(storage, str, NULL, UINT32_MAX);
                e_error(storage->event, "%s", str);
        } T_END;
        va_end(va);
@@ -638,7 +646,8 @@ void mailbox_set_critical(struct mailbox *box, const char *fmt, ...)
        va_start(va, fmt);
        T_BEGIN {
                const char *str = t_strdup_vprintf(fmt, va);
-               mail_storage_set_critical_error(box->storage, str, box->vname);
+               mail_storage_set_critical_error(box->storage, str, box->vname,
+                                               UINT32_MAX);
                e_error(box->event, "%s", str);
        } T_END;
        va_end(va);
@@ -650,20 +659,17 @@ void mail_set_critical(struct mail *mail, const char *fmt, ...)
 
        va_start(va, fmt);
        T_BEGIN {
-               if (mail->saving) {
-                       mailbox_set_critical(mail->box, "Saving mail: %s",
-                               t_strdup_vprintf(fmt, va));
-               } else {
-                       mailbox_set_critical(mail->box, "UID=%u: %s",
-                               mail->uid, t_strdup_vprintf(fmt, va));
-               }
+               const char *formatted_msg = t_strdup_vprintf(fmt, va);
+               mail_storage_set_critical_error(mail->box->storage, formatted_msg,
+                                               mail->box->vname, mail->uid);
+               e_error(mail_event(mail), "%s", formatted_msg);
        } T_END;
        va_end(va);
 }
 
 /* Note: mail_storage_get_last_internal_error() will always include
-         the mailbox prefix, while mailbox_get_last_internal_error()
-        usually will not. */
+        the mailbox prefix, while mailbox_get_last_internal_error() and
+        mail_get_last_internal_error() usually will not. */
 const char *mail_storage_get_last_internal_error(struct mail_storage *storage,
                                                 enum mail_error *error_r)
 {
@@ -671,14 +677,26 @@ const char *mail_storage_get_last_internal_error(struct mail_storage *storage,
                *error_r = storage->error;
        if (storage->last_error_is_internal) {
                i_assert(storage->last_internal_error != NULL);
-               if (storage->last_internal_error_mailbox == NULL)
-                       return storage->last_internal_error;
-               else {
+
+               bool is_mailbox_error_set = storage->last_internal_error_mailbox != NULL;
+               bool is_mail_error_set =
+                       storage->last_internal_error_mail_uid != UINT32_MAX;
+
+               if (is_mail_error_set) {
+                       i_assert(is_mailbox_error_set);
                        return t_strdup_printf(
-                               "Mailbox %s: %s",
+                               "Mailbox %s: UID %u: %s",
                                mailbox_name_sanitize(storage->last_internal_error_mailbox),
+                               storage->last_internal_error_mail_uid,
                                storage->last_internal_error);
                }
+               if (is_mailbox_error_set)
+                       return t_strdup_printf(
+                               "Mailbox %s: %s",
+                               mailbox_name_sanitize(storage->last_internal_error_mailbox),
+                               storage->last_internal_error);
+
+               return storage->last_internal_error;
        }
        return mail_storage_get_last_error(storage, error_r);
 }
@@ -696,6 +714,35 @@ const char *mailbox_get_last_internal_error(struct mailbox *box,
            strcmp(last_mailbox, box->vname) != 0)
                return mail_storage_get_last_internal_error(storage, error_r);
 
+       if (error_r != NULL)
+               *error_r = storage->error;
+       if (storage->last_error_is_internal) {
+               i_assert(storage->last_internal_error != NULL);
+               if (storage->last_internal_error_mail_uid != UINT32_MAX)
+                       return t_strdup_printf("UID %u: %s",
+                                              storage->last_internal_error_mail_uid,
+                                              storage->last_internal_error);
+               return storage->last_internal_error;
+       }
+       return mail_storage_get_last_error(storage, error_r);
+}
+
+/* Note: mail_get_last_internal_error() will include the mail prefix only when
+        mail->uid does not match last_internal_error_mail_uid, while
+        mail_storage_get_last_internal_error() always does. */
+const char *
+mail_get_last_internal_error(struct mail *mail, enum mail_error *error_r)
+{
+       struct mail_storage *storage = mailbox_get_storage(mail->box);
+       const char *last_mailbox = storage->last_internal_error_mailbox;
+       if (last_mailbox != NULL &&
+           strcmp(last_mailbox, mail->box->vname) != 0)
+               return mail_storage_get_last_internal_error(storage, error_r);
+
+       uint32_t last_mail_uid = storage->last_internal_error_mail_uid;
+       if (last_mail_uid == UINT32_MAX || last_mail_uid != mail->uid)
+               return mailbox_get_last_internal_error(mail->box, error_r);
+
        if (error_r != NULL)
                *error_r = storage->error;
        if (storage->last_error_is_internal) {
@@ -835,10 +882,16 @@ void mail_storage_last_error_push(struct mail_storage *storage)
        err->error_string = i_strdup(storage->error_string);
        err->error = storage->error;
        err->last_error_is_internal = storage->last_error_is_internal;
+       /* Initially set to UINT32_MAX manually to denote 'unset', as the
+          default 0 is used for mails currently being saved. If there is no
+          internal error, the attribute would not be set otherwise. */
+       err->last_internal_error_mail_uid = UINT32_MAX;
        if (err->last_error_is_internal) {
                err->last_internal_error = i_strdup(storage->last_internal_error);
                err->last_internal_error_mailbox =
                        i_strdup(storage->last_internal_error_mailbox);
+               err->last_internal_error_mail_uid =
+                       storage->last_internal_error_mail_uid;
        }
 }
 
@@ -856,6 +909,7 @@ void mail_storage_last_error_pop(struct mail_storage *storage)
        storage->last_error_is_internal = err->last_error_is_internal;
        storage->last_internal_error = err->last_internal_error;
        storage->last_internal_error_mailbox = err->last_internal_error_mailbox;
+       storage->last_internal_error_mail_uid = err->last_internal_error_mail_uid;
        array_delete(&storage->error_stack, count-1, 1);
 }
 

diff --git a/src/lib-storage/mail-storage.h b/src/lib-storage/mail-storage.h
index 7f53db97d3dacc7b7ea7f1306044ceb1dc717b79..b2895518a3bec328ba2d8e15c5e9cecbe11ad8c0 100644 (file)
--- a/src/lib-storage/mail-storage.h
+++ b/src/lib-storage/mail-storage.h
@@ -505,6 +505,10 @@ mail_storage_get_last_internal_error(struct mail_storage *storage,
 const char * ATTR_NOWARN_UNUSED_RESULT
 mailbox_get_last_internal_error(struct mailbox *box,
                                enum mail_error *error_r) ATTR_NULL(2);
+/* Wrapper for mail_storage_get_last_internal_error(); */
+const char * ATTR_NOWARN_UNUSED_RESULT
+mail_get_last_internal_error(struct mail *mail,
+                            enum mail_error *error_r) ATTR_NULL(2);
 
 /* Save the last error until it's popped. This is useful for cases where the
    storage has already failed, but the cleanup code path changes the error to