#79 unnecessary stat in rbox_mail_get_stream

it is not necessary to do a additional stat to determine the object
size, it is sufficient to read the object with max int size from rados.

- changed interface method read_mail
- replaced bufferlist.copy with memcpy.
- removed memset call for read_buffer.
- changed librmb_unit test to eval read_mail.

src/librmb/interfaces/rados-storage-interface.h

@@ -35,7 +35,6 @@ class RadosStorage {
   virtual int split_buffer_and_exec_op(const char *buffer, size_t buffer_length, RadosMailObject *current_object,
                                        librados::ObjectWriteOperation *write_op_xattr, uint64_t max_write) = 0;
 
-  virtual int read_mail(const std::string &oid, uint64_t *size_r, char *mail_buffer) = 0;
   virtual int load_metadata(RadosMailObject *mail) = 0;
   virtual int set_metadata(const std::string &oid, const RadosXAttr &xattr) = 0;
 
@@ -48,7 +47,7 @@ class RadosStorage {
   virtual int open_connection(const std::string &poolname, const std::string &ns) = 0;
   virtual bool wait_for_write_operations_complete(
       std::map<librados::AioCompletion*, librados::ObjectWriteOperation*>* completion_op_map) = 0;
-
+  virtual int read_mail(librados::bufferlist *buffer, const std::string &oid) = 0;
 };
 
 }  // namespace librmb

src/librmb/rados-storage.cpp

@@ -14,6 +14,7 @@
 #include <rados/librados.hpp>
 #include "rados-storage.h"
 #include "encoding.h"
+#include "limits.h"
 
 using std::string;
 
@@ -66,27 +67,14 @@ int RadosStorageImpl::split_buffer_and_exec_op(const char *buffer, size_t buffer
   return ret_val;
 }
 
-int RadosStorageImpl::read_mail(const std::string &oid, uint64_t *size_r, char *mail_buffer) {
-  int offset = 0;
-  librados::bufferlist mail_data_bl;
-
-  std::string str_buf;
+int RadosStorageImpl::read_mail(librados::bufferlist *buffer, const std::string &oid) {
   int ret = 0;
-  do {
-    mail_data_bl.clear();
-    ret = get_io_ctx().read(oid, mail_data_bl, *size_r, offset);
-    if (ret < 0) {
-      return ret;
-    }
-    if (ret == 0) {
-      break;
-    }
-    mail_data_bl.copy(0, (unsigned)ret, mail_buffer);
-    offset += ret;
-  } while (ret > 0);
+  size_t max = INT_MAX;
+  ret = get_io_ctx().read(oid, *buffer, max, 0);
   return ret;
 }
 
+
 int RadosStorageImpl::load_metadata(RadosMailObject *mail) {
   int ret = -1;
 

src/librmb/rados-storage.h

@@ -39,7 +39,6 @@ class RadosStorageImpl : public RadosStorage {
   int split_buffer_and_exec_op(const char *buffer, size_t buffer_length, RadosMailObject *current_object,
                                librados::ObjectWriteOperation *write_op_xattr, uint64_t max_write);
 
-  int read_mail(const std::string &oid, uint64_t *size_r, char *mail_buffer);
   int load_metadata(RadosMailObject *mail);
   int set_metadata(const std::string &oid, const RadosXAttr &xattr);
 
@@ -54,6 +53,8 @@ class RadosStorageImpl : public RadosStorage {
   bool wait_for_write_operations_complete(
       std::map<librados::AioCompletion *, librados::ObjectWriteOperation *> *completion_op_map);
 
+  int read_mail(librados::bufferlist *buffer, const std::string &oid);
+
  private:
   RadosCluster *cluster;
   int max_write_size;

src/librmb/tools/rmb/rmb.cpp

@@ -200,10 +200,15 @@ static void query_mail_storage(std::vector<librmb::RadosMailObject *> *mail_obje
           if (!value.empty()) {
             size_r = std::stol(value);
           }
+          librados::bufferlist buffer;
+
           char *mail_buffer = new char[size_r + 1];
           (*it_mail)->set_mail_buffer(mail_buffer);
+
           (*it_mail)->set_object_size(size_r);
-          if (storage->read_mail(oid, &size_r, mail_buffer) == 0) {
+          if (storage->read_mail(&buffer, oid) > 0) {
+            // if (storage->read_mail(oid, &size_r, mail_buffer) == 0) {
+            memcpy(mail_buffer, buffer.c_str(), size_r);
             mail_buffer[size_r] = '\0';
             // std::cout << mail_buffer << std::endl;
             if (tools.save_mail((*it_mail)) < 0) {

src/storage-rbox/rbox-mail.cpp

@@ -30,6 +30,7 @@ extern "C" {
 #include "ostream.h"
 #include "index-mail.h"
 #include "debug-helper.h"
+#include "limits.h"
 }
 
 #include "rados-mail-object.h"
@@ -260,38 +261,6 @@ static int rbox_mail_get_physical_size(struct mail *_mail, uoff_t *size_r) {
   FUNC_END();
   return 0;
 }
-static int rbox_get_mail_size(struct mail *_mail, uoff_t *size_r) {
-  FUNC_START();
-  struct rbox_storage *r_storage = (struct rbox_storage *)_mail->box->storage;
-  struct rbox_mail *rmail = (struct rbox_mail *)_mail;
-  uint64_t file_size = -1;
-  time_t time = 0;
-
-  if (rbox_open_rados_connection(_mail->box) < 0) {
-    FUNC_END_RET("ret == -1;  connection to rados failed");
-    return -1;
-  }
-
-  int ret_val = (r_storage->s)->stat_mail(rmail->mail_object->get_oid(), &file_size, &time);
-  if (ret_val < 0) {
-    if (ret_val == ((-1) * ENOENT)) {
-      i_debug("no_object set_expunged: rmail->mail_object->get_oid() %s, size %lu, uid=%d",
-              rmail->mail_object->get_oid().c_str(), file_size, _mail->uid);
-      rbox_mail_set_expunged(rmail);
-      return -1;
-    } else {
-      i_debug("no_object: rmail->mail_object->get_oid() %s, size %lu, uid=%d", rmail->mail_object->get_oid().c_str(),
-              file_size, _mail->uid);
-      FUNC_END_RET("ret == -1; rbox_read");
-      return -1;
-    }
-   }
-
-   *size_r = (uoff_t)file_size;
-
-   FUNC_END();
-   return 0;
-}
 
 static int get_mail_stream(struct rbox_mail *mail, char *buffer, uint64_t physical_size, struct istream **stream_r) {
   struct mail_private *pmail = &mail->imail.mail;
@@ -330,44 +299,39 @@ static int rbox_mail_get_stream(struct mail *_mail, bool get_body ATTR_UNUSED, s
       return -1;
     }
 
-    // real object size
-    if (rbox_get_mail_size(_mail, &size_r) < 0) {
-      FUNC_END_RET("ret == -1; get mail size");
-      return -1;
-    }
-
     if (rmail->mail_buffer != NULL) {
       i_free(rmail->mail_buffer);
     }
 
-    rmail->mail_buffer = p_new(default_pool, char, size_r);
-    if (rmail->mail_buffer == NULL) {
-      FUNC_END_RET("ret == -1; out of memory");
-      return -1;
-    }
-
-    memset(rmail->mail_buffer, '\0', sizeof(char) * size_r);
     _mail->transaction->stats.open_lookup_count++;
 
-    ret = r_storage->s->read_mail(rmail->mail_object->get_oid(), &size_r, rmail->mail_buffer);
-    if (ret < 0) {
-      if (ret == -ENOENT) {
+    librados::bufferlist mail_data_bl;
+    size_r = r_storage->s->read_mail(&mail_data_bl, rmail->mail_object->get_oid());
+    if (size_r <= 0) {
+      if (size_r == -ENOENT) {
         rbox_mail_set_expunged(rmail);
         return -1;
       } else {
-        i_debug("error code: %d", ret);
+        i_debug("error code: %d", size_r);
         FUNC_END_RET("ret == -1");
         return -1;
       }
-    }
-
-    get_mail_stream(rmail, rmail->mail_buffer, size_r, &input);
-
-    uoff_t size_decompressed = -1;
-    i_stream_get_size(input, TRUE, &size_decompressed);
-
-    data->stream = input;
-    index_mail_set_read_buffer_size(_mail, input);
+     } else {
+       rmail->mail_buffer = p_new(default_pool, char, size_r + 1);
+       if (rmail->mail_buffer == NULL) {
+         FUNC_END_RET("ret == -1; out of memory");
+         return -1;
+       }
+       memcpy(rmail->mail_buffer, mail_data_bl.c_str(), size_r);
+     }
+     // ret = r_storage->s->read_mail(rmail->mail_object->get_oid(), &size_r, rmail->mail_buffer);
+     get_mail_stream(rmail, rmail->mail_buffer, size_r, &input);
+
+     uoff_t size_decompressed = -1;
+     i_stream_get_size(input, TRUE, &size_decompressed);
+
+     data->stream = input;
+     index_mail_set_read_buffer_size(_mail, input);
   }
   ret = index_mail_init_stream(&rmail->imail, hdr_size, body_size, stream_r);
 

src/storage-rbox/rbox-save.cpp

@@ -159,6 +159,7 @@ int rbox_save_begin(struct mail_save_context *_ctx, struct istream *input) {
   FUNC_START();
   rbox_save_context *r_ctx = (struct rbox_save_context *)_ctx;
   struct istream *crlf_input;
+  int initial_mail_buffer_size = 512;
 
   r_ctx->failed = FALSE;
   if (_ctx->dest_mail == NULL) {
@@ -183,8 +184,9 @@ int rbox_save_begin(struct mail_save_context *_ctx, struct istream *input) {
     // make 100% sure, buffer is empty!
     buffer_free(&buffer);
   }
-  r_ctx->current_object->set_mail_buffer(reinterpret_cast<char *>(buffer_create_dynamic(default_pool, 1024)));
-  // r_ctx->mail_buffer = ;
+  r_ctx->current_object->set_mail_buffer(
+      reinterpret_cast<char *>(buffer_create_dynamic(default_pool, initial_mail_buffer_size)));
+
   if (r_ctx->current_object->get_mail_buffer() == NULL) {
     FUNC_END_RET("ret == -1");
     return -1;
@@ -252,14 +254,12 @@ static int rbox_save_mail_write_metadata(struct rbox_save_context *ctx, librados
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
   {
-    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_MAILBOX_GUID,
-                           guid_128_to_string(ctx->mbox->mailbox_guid));
+    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_MAILBOX_GUID, guid_128_to_string(ctx->mbox->mailbox_guid));
 
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
   {
-    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_GUID,
-                            guid_128_to_string(ctx->mail_guid));
+    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_GUID, guid_128_to_string(ctx->mail_guid));
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
   {
@@ -269,33 +269,29 @@ static int rbox_save_mail_write_metadata(struct rbox_save_context *ctx, librados
   }
   {
     if (mdata->pop3_uidl != NULL) {
-      RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_POP3_UIDL,
-                                   mdata->pop3_uidl);
+      RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_POP3_UIDL, mdata->pop3_uidl);
       write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
     }
   }
   {
     if (mdata->pop3_order != 0) {
-      RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_POP3_ORDER,
-                                   mdata->pop3_order);
+      RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_POP3_ORDER, mdata->pop3_order);
       write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
     }
   }
   {
     if (mdata->from_envelope != NULL) {
-      RadosXAttr xattr(
-          rbox_metadata_key::RBOX_METADATA_FROM_ENVELOPE, mdata->from_envelope);
+      RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_FROM_ENVELOPE, mdata->from_envelope);
       write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
     }
   }
   {
     uoff_t vsize = -1;
     if (mail_get_virtual_size(ctx->ctx.dest_mail, &vsize) < 0) {
-      i_debug("failed, unable to determine virtual size:");
+      i_debug("failed, unable to determine virtual size, using physical size instead.");
+      vsize = ctx->input->v_offset;
     }
-
-    librmb::RadosXAttr xattr(
-        rbox_metadata_key::RBOX_METADATA_VIRTUAL_SIZE, vsize);
+    librmb::RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_VIRTUAL_SIZE, vsize);
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
   {
@@ -305,20 +301,17 @@ static int rbox_save_mail_write_metadata(struct rbox_save_context *ctx, librados
   }
   {
     std::string flags = std::to_string(mdata->flags);
-    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_OLDV1_FLAGS,
-                                  flags);
+    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_OLDV1_FLAGS, flags);
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
 
   {
     std::string pvt_flags = std::to_string(mdata->pvt_flags);
-    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_PVT_FLAGS,
-                                  pvt_flags);
+    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_PVT_FLAGS, pvt_flags);
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
   {
-    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_ORIG_MAILBOX,
-                            ctx->mbox->box.name);
+    RadosXAttr xattr(rbox_metadata_key::RBOX_METADATA_ORIG_MAILBOX, ctx->mbox->box.name);
     write_op_xattr->setxattr(xattr.key.c_str(), xattr.bl);
   }
 

src/tests/librmb/it_test_librmb.cpp

@@ -167,8 +167,11 @@ TEST(librmb1, read_mail) {
   time_t save_date;
   int ret_stat = storage.stat_mail(obj.get_oid(), &size, &save_date);
 
-  char *buff = new char[size];
-  int ret = storage.read_mail(obj.get_oid(), &size, &buff[0]);
+  char *buff = new char[size + 1];
+  librados::bufferlist bl;
+  int copy_mail_ret = storage.read_mail(&bl, obj.get_oid());
+  memcpy(buff, bl.c_str(), size);
+  EXPECT_EQ(buff[size], '\0');
 
   // remove it
   int ret_remove = storage.delete_mail(obj.get_oid());
@@ -178,7 +181,7 @@ TEST(librmb1, read_mail) {
   EXPECT_EQ(ret_storage, 0);
   EXPECT_EQ(ret_stat, 0);
   EXPECT_EQ(ret_remove, 0);
-  EXPECT_EQ(ret, 0);
+  EXPECT_EQ(copy_mail_ret, 14);
   EXPECT_EQ(buff[0], 'a');
   EXPECT_EQ(buff[1], 'b');
   EXPECT_EQ(buff[2], 'c');

src/tests/mocks/mock_test.h

@@ -32,7 +32,6 @@ class RadosStorageMock : public RadosStorage {
   MOCK_METHOD0(get_max_write_size_bytes, int());
   MOCK_METHOD5(split_buffer_and_exec_op, int(const char *buffer, size_t buffer_length, RadosMailObject *current_object,
                                              librados::ObjectWriteOperation *write_op_xattr, uint64_t max_write));
-  MOCK_METHOD3(read_mail, int(const std::string &oid, uint64_t *size_r, char *mail_buffer));
   MOCK_METHOD1(load_metadata, int(RadosMailObject *mail));
   MOCK_METHOD2(set_metadata, int(const std::string &oid, const RadosXAttr &xattr));
 
@@ -43,6 +42,7 @@ class RadosStorageMock : public RadosStorage {
   MOCK_METHOD1(find_mails, librados::NObjectIterator(const RadosXAttr *attr));
   MOCK_METHOD2(open_connection, int(const std::string &poolname, const std::string &ns));
   MOCK_METHOD1(wait_for_write_operations_complete,bool(std::map<librados::AioCompletion*, librados::ObjectWriteOperation*>* completion_op_map));
+  MOCK_METHOD2(read_mail, int(librados::bufferlist *buffer, const std::string &oid));
 };
 
 using librmb::RadosDictionary;