Merge pull request #211 from ceph-dovecot/jrse_#209

Jrse #209

.travis.yml

@@ -36,13 +36,17 @@ script:
 - docker exec build sh -c 'doveadm -D altmove -r -u t2 ALL'
 
 - docker exec build sh -c 'doveadm -Dv backup -u t1 -m INBOX mdbox:/usr/local/var/mail/mdbox/t1'
-- docker exec build sh -c 'doveadm -D fetch -u t1 "guid date.received seq size.virtual uid user mailbox-guid mailbox" ALL > /root/rbox.t1.mails'
-- docker exec build sh -c 'doveadm -D -c /usr/local/etc/dovecot_mdbox/dovecot.conf fetch -u t1 "guid date.received seq size.virtual uid user mailbox-guid mailbox" ALL > /root/mdbox.t1.mails'
-- docker exec build sh -c 'diff -y -W 200 /root/rbox.t1.mails /root/mdbox.t1.mails'
+- docker exec build sh -c 'doveadm -D fetch -u t1 "guid date.received date.sent flags pop3.uidl seq size.virtual uid user mailbox-guid mailbox" ALL > /root/rbox.t1.mails'
+- docker exec build sh -c 'doveadm -D -c /usr/local/etc/dovecot_mdbox/dovecot.conf fetch -u t1 "guid date.received date.sent flags pop3.uidl seq size.virtual uid user mailbox-guid mailbox" ALL > /root/mdbox.t1.mails'
+- docker exec build sh -c 'cd repo/src/scripts; ./sort.sh /root/rbox.t1.mails /root/rbox.t1.mails.sorted'
+- docker exec build sh -c 'cd repo/src/scripts; ./sort.sh /root/mdbox.t1.mails /root/mdbox.t1.mails.sorted'
+
+- docker exec build sh -c 'diff -y -W 1500 /root/rbox.t1.mails.sorted /root/mdbox.t1.mails.sorted'
 - docker exec build sh -c 'rm -r /usr/local/var/mail/rbox/t1'
 - docker exec build sh -c 'doveadm -Dv -c /usr/local/etc/dovecot_mdbox/dovecot.conf backup -u t1 -m INBOX rbox:/usr/local/var/mail/rbox/t1'
-- docker exec build sh -c 'doveadm -D fetch -u t1 "guid date.received seq size.virtual uid user mailbox-guid mailbox" ALL > /root/rbox.t1.mails'
-- docker exec build sh -c 'diff -y -W 200 /root/rbox.t1.mails /root/mdbox.t1.mails'
+- docker exec build sh -c 'doveadm -D fetch -u t1 "guid date.received date.sent flags pop3.uidl seq size.virtual uid user mailbox-guid mailbox" ALL > /root/rbox.t1.mails'
+- docker exec build sh -c 'cd repo/src/scripts; ./sort.sh /root/rbox.t1.mails /root/rbox.t1.mails.sorted'
+- docker exec build sh -c 'diff -y -W 1500 /root/rbox.t1.mails.sorted /root/mdbox.t1.mails.sorted'
 
 - docker exec build sh -c 'rm -r /usr/local/var/mail/rbox'
 - docker exec build sh -c 'cd /usr/local/bin; ./imaptest user=t%d pass=t port=10110 profile=/root/pop3-profile.conf users=100 clients=10 error_quit secs=150 output=/var/log/imaptest.log'

src/scripts/sort.sh

@@ -0,0 +1 @@
+perl -n -e 'next if (/^doveadm/); if (/^(flags:\s*)(.*)/) {$_ = $1 . join(" ", sort split(" ",$2)) . "\n";}  print;' $1 | sed -e ':1;s/\f$/\f/;t 2;N;b 1;:2;y/\n/\t/' | sort -t  '	' -k 1 > $2

src/storage-rbox/rbox-copy.cpp

@@ -47,8 +47,9 @@ int rbox_mail_copy(struct mail_save_context *_ctx, struct mail *mail) {
 
   r_ctx->copying = _ctx->saving != TRUE && strcmp(mail->box->storage->name, "rbox") == 0 &&
                    strcmp(mail->box->storage->name, storage_name) == 0;
-
-  int ret = rbox_mail_storage_copy(_ctx, mail);
+  int ret = 0;
+  T_BEGIN { ret = rbox_mail_storage_copy(_ctx, mail); }
+  T_END;
   // cppcheck-suppress redundantAssignment
   r_ctx->copying = FALSE;
 
@@ -328,21 +329,14 @@ static int rbox_mail_storage_try_copy(struct mail_save_context **_ctx, struct ma
     if (ctx->moving != TRUE) {
       if (copy_mail(ctx, rados_storage, rmail, &ns_src, &ns_dest) < 0) {
         FUNC_END_RET("ret == -1, copy mail failed");
-        i_debug("OK FOPY MAIL FAILED");
         return -1;
       }
     }
+
     if (ctx->moving) {
-      int ret = 0;
-      T_BEGIN {
-        if (move_mail(ctx, rados_storage, mail, &ns_src, &ns_dest) < 0) {
-          FUNC_END_RET("ret == -1, move mail failed");
-          ret - 1;
-        }
-      }
-      T_END;
-      if (ret < 0) {
-        return ret;
+      if (move_mail(ctx, rados_storage, mail, &ns_src, &ns_dest) < 0) {
+        FUNC_END_RET("ret == -1, move mail failed");
+        return -1;
       }
     }
 
@@ -368,14 +362,12 @@ int rbox_mail_storage_copy(struct mail_save_context *ctx, struct mail *mail) {
 #endif
   r_ctx->finished = TRUE;
 
-#if DOVECOT_PREREQ(2, 3)
   if (ctx->data.keywords != NULL) {
     /* keywords gets unreferenced twice: first in
        mailbox_save_cancel()/_finish() and second time in
        mailbox_copy(). */
     mailbox_keywords_ref(ctx->data.keywords);
   }
-#endif
 
   enum mail_flags flags = index_mail_get_flags(mail);
   bool alt_storage = is_alternate_storage_set(flags) && is_alternate_pool_valid(mail->box);

src/storage-rbox/rbox-mail.cpp

@@ -221,6 +221,7 @@ static int rbox_mail_get_save_date(struct mail *_mail, time_t *date_r) {
   if (ret_val < 0) {
     if (ret_val != -ENOENT) {
       FUNC_END_RET("ret == -1; cannot stat object to get received date and object size");
+      mail_set_expunged(_mail);
     }
     return -1;
   }
@@ -391,7 +392,6 @@ static int rbox_mail_get_stream(struct mail *_mail, bool get_body ATTR_UNUSED, s
     }
     rmail->rados_mail->get_mail_buffer()->clear();
 
-    _mail->transaction->stats.open_lookup_count++;
     int physical_size = rados_storage->read_mail(rmail->rados_mail->get_oid(), rmail->rados_mail->get_mail_buffer());
     if (physical_size < 0) {
       if (physical_size == -ENOENT) {

src/storage-rbox/rbox-save.cpp

@@ -62,6 +62,7 @@ struct mail_save_context *rbox_save_alloc(struct mailbox_transaction_context *t)
     r_ctx->ctx.transaction = t;
     r_ctx->mbox = rbox;
     r_ctx->trans = t->itrans;
+    t->save_ctx = &r_ctx->ctx;
   } else {
     r_ctx->failed = FALSE;
     r_ctx->finished = FALSE;
@@ -70,8 +71,6 @@ struct mail_save_context *rbox_save_alloc(struct mailbox_transaction_context *t)
   }
   r_ctx->rados_mail = nullptr;
 
-  t->save_ctx = &r_ctx->ctx;
-
   FUNC_END();
   return t->save_ctx;
 }
@@ -99,16 +98,17 @@ void rbox_index_append(struct mail_save_context *_ctx) {
   FUNC_START();
   struct rbox_save_context *r_ctx = (struct rbox_save_context *)_ctx;
 
-/* add to index */
-#if DOVECOT_PREREQ(2, 3)
-  if ((r_ctx->ctx.transaction->flags & MAILBOX_TRANSACTION_FLAG_FILL_IN_STUB) == 0) {
-    mail_index_append(r_ctx->trans, _ctx->data.uid, &r_ctx->seq);
-  } else {
-    r_ctx->seq = _ctx->data.stub_seq;
-  }
-#else
+  /* add to index */
+  /*#if DOVECOT_PREREQ(2, 3)
+    // dovecot 2.3 is using different mechanism for uid assignment. we ignore it for now.
+    if ((r_ctx->ctx.transaction->flags & MAILBOX_TRANSACTION_FLAG_FILL_IN_STUB) == 0) {
+      mail_index_append(r_ctx->trans, _ctx->data.uid, &r_ctx->seq);
+    } else {
+      r_ctx->seq = _ctx->data.stub_seq;
+    }
+  #else*/
   mail_index_append(r_ctx->trans, _ctx->data.uid, &r_ctx->seq);
-#endif
+  // #endif
 
   mail_index_update_flags(r_ctx->trans, r_ctx->seq, MODIFY_REPLACE,
                           static_cast<enum mail_flags>(_ctx->data.flags & ~MAIL_RECENT));
@@ -232,6 +232,13 @@ int rbox_save_continue(struct mail_save_context *_ctx) {
     return -1;
   }
 
+#if DOVECOT_PREREQ(2, 3)
+  if (index_storage_save_continue(_ctx, r_ctx->input, _ctx->dest_mail) < 0) {
+    r_ctx->failed = TRUE;
+    FUNC_END();
+    return -1;
+  }
+#else
   do {
     if (o_stream_send_istream(_ctx->data.output, r_ctx->input) < 0) {
       if (!mail_storage_set_error_from_errno(storage)) {
@@ -245,9 +252,11 @@ int rbox_save_continue(struct mail_save_context *_ctx) {
     index_mail_cache_parse_continue(_ctx->dest_mail);
     /* both tee input readers may consume data from our primary
      input stream. we'll have to make sure we don't return with
-     one of the streams still having data in them. */
+     one of the streams still having data in them.*/
   } while (i_stream_read(r_ctx->input) > 0);
 
+#endif
+
   FUNC_END();
   return 0;
 }
@@ -370,9 +379,6 @@ static void clean_up_failed(struct rbox_save_context *r_ctx) {
     delete_ret = r_storage->s->delete_mail(*it_cur_obj);
     if (delete_ret < 0 && delete_ret != -ENOENT) {
       i_error("Librados obj: %s, could not be removed", (*it_cur_obj)->get_oid().c_str());
-    } else {
-      i_error("mail object successfully %s removed from objectstore due to previous error",
-              (*it_cur_obj)->get_oid().c_str());
     }
   }
   // clean up index
@@ -413,27 +419,42 @@ int rbox_save_finish(struct mail_save_context *_ctx) {
 
   if (!r_ctx->failed) {
     if (_ctx->data.save_date != (time_t)-1) {
-      index_mail_cache_add((struct index_mail *)_ctx->dest_mail, MAIL_CACHE_SAVE_DATE, &_ctx->data.save_date,
-                           sizeof(_ctx->data.save_date));
+      uint32_t t = _ctx->data.save_date;
+      index_mail_cache_add((struct index_mail *)_ctx->dest_mail, MAIL_CACHE_SAVE_DATE, &t, sizeof(t));
     }
 
-    if (r_ctx->ctx.data.output != r_ctx->output_stream) {
 #if DOVECOT_PREREQ(2, 3)
+    int ret = 0;
+    if (r_ctx->ctx.data.output != r_ctx->output_stream) {
       /* e.g. zlib plugin had changed this. make sure we
              successfully write the trailer. */
-      o_stream_finish(r_ctx->ctx.data.output);
+      ret = o_stream_finish(r_ctx->ctx.data.output);
+    } else {
+      /* no plugins - flush the output so far */
+      ret = o_stream_flush(r_ctx->ctx.data.output);
+    }
+    if (ret < 0) {
+      mail_set_critical(r_ctx->ctx.dest_mail, "write(%s) failed: %s", o_stream_get_name(r_ctx->ctx.data.output),
+                        o_stream_get_error(r_ctx->ctx.data.output));
+      r_ctx->failed = TRUE;
+    }
 #else
-      if (o_stream_nfinish(r_ctx->ctx.data.output) < 0) {
-        mail_storage_set_critical(r_ctx->ctx.transaction->box->storage, "write(%s) failed: %m",
-                                  o_stream_get_name(r_ctx->ctx.data.output));
-        r_ctx->failed = TRUE;
-      }
+    if (o_stream_nfinish(r_ctx->ctx.data.output) < 0) {
+      mail_storage_set_critical(r_ctx->ctx.transaction->box->storage, "write(%s) failed: %m",
+                                o_stream_get_name(r_ctx->ctx.data.output));
+      r_ctx->failed = TRUE;
+    }
+    if (r_ctx->ctx.data.output == NULL) {
+      i_assert(r_ctx->failed);
+    }
 #endif
+    if (r_ctx->ctx.data.output != r_ctx->output_stream) {
       /* e.g. zlib plugin had changed this */
       o_stream_ref(r_ctx->output_stream);
       o_stream_destroy(&r_ctx->ctx.data.output);
       r_ctx->ctx.data.output = r_ctx->output_stream;
     }
+
     // reset virtual size
     index_mail_cache_parse_deinit(_ctx->dest_mail, r_ctx->ctx.data.received_date, !r_ctx->failed);
     // always save to primary storage
@@ -563,10 +584,9 @@ int rbox_transaction_save_commit_pre(struct mail_save_context *_ctx) {
     i_error("mail_index_get_header failed");
     return -1;
   }
+  // note dovecot 2.3 is using stashed away uids, this mechanism is not used for now.
   mail_index_append_finish_uids(r_ctx->trans, hdr->next_uid, &_ctx->transaction->changes->saved_uids);
 
-  struct seq_range_iter iter;
-  seq_range_array_iter_init(&iter, &_ctx->transaction->changes->saved_uids);
   if (rbox_save_assign_uids(r_ctx, &_ctx->transaction->changes->saved_uids) < 0) {
     rbox_transaction_save_rollback(_ctx);
     return -1;
@@ -595,7 +615,9 @@ void rbox_transaction_save_commit_post(struct mail_save_context *_ctx,
 
   mail_index_sync_set_commit_result(r_ctx->sync_ctx->index_sync_ctx, result);
 
-  (void)rbox_sync_finish(&r_ctx->sync_ctx, TRUE);
+  if (rbox_sync_finish(&r_ctx->sync_ctx, TRUE) < 0) {
+    r_ctx->failed = TRUE;
+  }
   rbox_transaction_save_rollback(_ctx);
 
   FUNC_END();

src/storage-rbox/rbox-storage.cpp

@@ -381,7 +381,7 @@ static int rbox_open_mailbox(struct mailbox *box) {
   return 0;
 }
 
-int read_plugin_configuration(struct mailbox *box) {
+void read_plugin_configuration(struct mailbox *box) {
   FUNC_START();
   struct rbox_storage *r_storage = (struct rbox_storage *)box->storage;
 
@@ -404,7 +404,6 @@ int read_plugin_configuration(struct mailbox *box) {
   }
 
   FUNC_END();
-  return 0;
 }
 
 bool is_alternate_storage_set(uint8_t flags) { return (flags & RBOX_INDEX_FLAG_ALT) != 0; }
@@ -802,11 +801,9 @@ int rbox_mailbox_get_metadata(struct mailbox *box, enum mailbox_metadata_items i
                               struct mailbox_metadata *metadata_r) {
   FUNC_START();
 
-  if (items != 0) {
-    if (index_mailbox_get_metadata(box, items, metadata_r) < 0) {
-      FUNC_END_RET("ret == -1");
-      return -1;
-    }
+  if (index_mailbox_get_metadata(box, items, metadata_r) < 0) {
+    FUNC_END_RET("ret == -1");
+    return -1;
   }
 
   if ((items & MAILBOX_METADATA_GUID) != 0) {

src/storage-rbox/rbox-storage.h

@@ -57,7 +57,7 @@ extern int rbox_open_rados_connection(struct mailbox *box, bool alt_storage);
  * @brief reads the 90-plugin.conf section
  * @param[in] box mailbox (state open).
  */
-extern int read_plugin_configuration(struct mailbox *box);
+extern void read_plugin_configuration(struct mailbox *box);
 /**
  * @brief: deletes the given mailbox
  */

src/tests/dict_rados.supp

@@ -12,6 +12,19 @@
    fun:clone
 }
 
+{
+   <async_connection_>
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:_Znwm
+   fun:_Z14decode_messageP11CephContextiR15ceph_msg_headerR15ceph_msg_footerRN4ceph6buffer4listES8_S8_P10Connection
+   fun:_ZN15AsyncConnection7processEv
+   fun:_ZN11EventCenter14process_eventsEiPNSt6chrono8durationImSt5ratioILl1ELl1000000000EEEE
+   obj:/usr/lib/x86_64-linux-gnu/ceph/libceph-common.so.0
+   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25
+   fun:start_thread
+   fun:clone
+}
 {
    <dict_rados_addr8_1>
    Memcheck:Addr8