Fix some memory freeing

osd.cpp

@@ -98,18 +98,19 @@ osd_op_t::~osd_op_t()
     {
         delete bs_op;
     }
+    if (op_data)
+    {
+        free(op_data);
+    }
+    if (rmw_buf)
+    {
+        free(rmw_buf);
+    }
     if (buf)
     {
         // Note: reusing osd_op_t WILL currently lead to memory leaks
         // So we don't reuse it, but free it every time
-        if (op_type == OSD_OP_IN &&
+        free(buf);
-            req.hdr.opcode == OSD_OP_SHOW_CONFIG)
-        {
-            std::string *str = (std::string*)buf;
-            delete str;
-        }
-        else
-            free(buf);
     }
 }
 
@@ -307,7 +308,6 @@ void osd_t::exec_op(osd_op_t *cur_op)
         delete cur_op;
         return;
     }
-    inflight_ops++;
     if (cur_op->req.hdr.magic != SECONDARY_OSD_OP_MAGIC ||
         cur_op->req.hdr.opcode < OSD_OP_MIN || cur_op->req.hdr.opcode > OSD_OP_MAX ||
         (cur_op->req.hdr.opcode == OSD_OP_SECONDARY_READ || cur_op->req.hdr.opcode == OSD_OP_SECONDARY_WRITE ||
@@ -315,10 +315,13 @@ void osd_t::exec_op(osd_op_t *cur_op)
         (cur_op->req.sec_rw.len > OSD_RW_MAX || cur_op->req.sec_rw.len % OSD_RW_ALIGN || cur_op->req.sec_rw.offset % OSD_RW_ALIGN))
     {
         // Bad command
-        cur_op->bs_op->retval = -EINVAL;
+        cur_op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC;
-        secondary_op_callback(cur_op);
+        cur_op->reply.hdr.id = cur_op->req.hdr.id;
+        cur_op->reply.hdr.opcode = cur_op->req.hdr.opcode;
+        cur_op->reply.hdr.retval = -EINVAL;
         return;
     }
+    inflight_ops++;
     if (cur_op->req.hdr.opcode == OSD_OP_TEST_SYNC_STAB_ALL)
     {
         exec_sync_stab_all(cur_op);

osd.h

@@ -102,6 +102,7 @@ struct osd_op_t
     osd_any_reply_t reply;
     blockstore_op_t *bs_op = NULL;
     void *buf = NULL;
+    void *rmw_buf = NULL;
     osd_primary_op_data_t* op_data = NULL;
     std::function<void(osd_op_t*)> callback;
 
@@ -209,7 +210,6 @@ class osd_t
     void handle_read_op(osd_client_t *cl);
     void handle_read_reply(osd_client_t *cl);
     void send_replies();
-    void make_reply(osd_op_t *op);
     void handle_send(ring_data_t *data, int peer_fd);
     void outbox_push(osd_client_t & cl, osd_op_t *op);
 

osd_peering.cpp

@@ -268,17 +268,22 @@ void osd_t::start_pg_peering(int pg_idx)
                 it = pg.peering_state->list_ops.begin();
             }
         }
-        for (auto & p: pg.peering_state->list_results)
+        for (auto it = pg.peering_state->list_results.begin(); it != pg.peering_state->list_results.end(); it++)
         {
             int role;
             for (role = 0; role < pg.cur_set.size(); role++)
             {
-                if (pg.cur_set[role] == p.first)
+                if (pg.cur_set[role] == it->first)
                     break;
             }
             if (pg.state == PG_INCOMPLETE || role >= pg.cur_set.size())
             {
-                pg.peering_state->list_results.erase(p.first);
+                if (it->second.buf)
+                {
+                    free(it->second.buf);
+                }
+                pg.peering_state->list_results.erase(it);
+                it = pg.peering_state->list_results.begin();
             }
         }
     }
@@ -380,7 +385,7 @@ void osd_t::start_pg_peering(int pg_idx)
                     .total_count = (uint64_t)op->reply.hdr.retval,
                     .stable_count = op->reply.sec_list.stable_count,
                 };
-                // so it doesn't get freed. FIXME: do it better
+                // set op->buf to NULL so it doesn't get freed
                 op->buf = NULL;
                 ps->list_done++;
                 ps->list_ops.erase(role_osd);

osd_peering_pg.h

@@ -54,7 +54,7 @@ struct pg_osd_set_state_t
 
 struct pg_list_result_t
 {
-    obj_ver_id *buf;
+    obj_ver_id *buf = NULL;
     uint64_t total_count;
     uint64_t stable_count;
 };

osd_primary.cpp

@@ -19,8 +19,6 @@ struct osd_primary_op_data_t
     int degraded = 0, pg_size, pg_minsize;
     osd_rmw_stripe_t *stripes;
     osd_op_t *subops = NULL;
-    void *rmw_buf = NULL;
-
     bool should_read_version = false;
 };
 
@@ -240,7 +238,7 @@ void osd_t::submit_read_subops(int read_pg_size, const uint64_t* osd_set, osd_op
                 subops[subop].buf = stripes[role].read_buf;
                 subops[subop].callback = [cur_op, this](osd_op_t *subop)
                 {
-                    // so it doesn't get freed. FIXME: do it better
+                    // so it doesn't get freed
                     subop->buf = NULL;
                     handle_primary_read_subop(cur_op, subop->reply.hdr.retval == subop->req.sec_rw.len);
                 };
@@ -276,7 +274,7 @@ void osd_t::exec_primary_write(osd_op_t *cur_op)
     // Check if there are other write requests to the same object
     
     // Determine blocks to read
-    op_data->rmw_buf = calc_rmw_reads(cur_op->buf, op_data->stripes, pg.cur_set.data(), pg.pg_size, pg.pg_minsize, pg.pg_cursize);
+    cur_op->rmw_buf = calc_rmw_reads(cur_op->buf, op_data->stripes, pg.cur_set.data(), pg.pg_size, pg.pg_minsize, pg.pg_cursize);
     op_data->should_read_version = true;
     // Read required blocks
     submit_read_subops(pg.pg_size, pg.cur_set.data(), cur_op);

osd_receive.cpp

@@ -149,8 +149,8 @@ void osd_t::handle_read_reply(osd_client_t *cl)
     if (op->reply.hdr.opcode == OSD_OP_SECONDARY_READ &&
         op->reply.hdr.retval > 0)
     {
-        // Read data
+        // Read data. In this case we assume that the buffer is preallocated by the caller (!)
-        // FIXME: op->buf must be allocated
+        assert(op->buf);
         cl->read_state = CL_READ_REPLY_DATA;
         cl->read_reply_id = op->req.hdr.id;
         cl->read_buf = op->buf;

osd_secondary.cpp

@@ -2,19 +2,46 @@
 
 #include "json11/json11.hpp"
 
-void osd_t::secondary_op_callback(osd_op_t *cur_op)
+void osd_t::secondary_op_callback(osd_op_t *op)
 {
     inflight_ops--;
-    auto cl_it = clients.find(cur_op->peer_fd);
+    auto cl_it = clients.find(op->peer_fd);
     if (cl_it != clients.end())
     {
+        op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC;
+        op->reply.hdr.id = op->req.hdr.id;
+        op->reply.hdr.opcode = op->req.hdr.opcode;
+        op->reply.hdr.retval = op->bs_op->retval;
+        if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST)
+        {
+            op->reply.sec_list.stable_count = op->bs_op->version;
+        }
+        else if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ ||
+            op->req.hdr.opcode == OSD_OP_SECONDARY_WRITE)
+        {
+            op->reply.sec_rw.version = op->bs_op->version;
+        }
+        else if (op->req.hdr.opcode == OSD_OP_SECONDARY_DELETE)
+        {
+            op->reply.sec_del.version = op->bs_op->version;
+        }
+        if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ &&
+            op->reply.hdr.retval > 0)
+        {
+            op->send_list.push_back(op->buf, op->reply.hdr.retval);
+        }
+        else if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST &&
+            op->reply.hdr.retval > 0)
+        {
+            op->buf = op->bs_op->buf; // allocated by blockstore
+            op->send_list.push_back(op->buf, op->reply.hdr.retval * sizeof(obj_ver_id));
+        }
         auto & cl = cl_it->second;
-        make_reply(cur_op);
+        outbox_push(cl, op);
-        outbox_push(cl, cur_op);
     }
     else
     {
-        delete cur_op;
+        delete op;
     }
 }
 
@@ -74,10 +101,15 @@ void osd_t::exec_secondary(osd_op_t *cur_op)
 void osd_t::exec_show_config(osd_op_t *cur_op)
 {
     // FIXME: Send the real config, not its source
-    std::string *cfg_str = new std::string(std::move(json11::Json(config).dump()));
+    std::string cfg_str = json11::Json(config).dump();
-    cur_op->buf = cfg_str;
+    cur_op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC;
+    cur_op->reply.hdr.id = cur_op->req.hdr.id;
+    cur_op->reply.hdr.opcode = cur_op->req.hdr.opcode;
+    cur_op->reply.hdr.retval = cfg_str.size()+1;
+    cur_op->buf = malloc(cfg_str.size()+1);
+    memcpy(cur_op->buf, cfg_str.c_str(), cfg_str.size()+1);
     auto & cl = clients[cur_op->peer_fd];
-    make_reply(cur_op);
+    cur_op->send_list.push_back(cur_op->buf, cur_op->reply.hdr.retval);
     outbox_push(cl, cur_op);
 }
 
@@ -104,38 +136,3 @@ void osd_t::exec_sync_stab_all(osd_op_t *cur_op)
     bs->enqueue_op(cur_op->bs_op);
 #endif
 }
-
-void osd_t::make_reply(osd_op_t *op)
-{
-    op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC;
-    op->reply.hdr.id = op->req.hdr.id;
-    op->reply.hdr.opcode = op->req.hdr.opcode;
-    if (op->req.hdr.opcode == OSD_OP_SHOW_CONFIG)
-    {
-        std::string *str = (std::string*)op->buf;
-        op->reply.hdr.retval = str->size()+1;
-        op->send_list.push_back((void*)str->c_str(), op->reply.hdr.retval);
-    }
-    else
-    {
-        op->reply.hdr.retval = op->bs_op->retval;
-        if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST)
-            op->reply.sec_list.stable_count = op->bs_op->version;
-        else if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ ||
-            op->req.hdr.opcode == OSD_OP_SECONDARY_WRITE)
-            op->reply.sec_rw.version = op->bs_op->version;
-        else if (op->req.hdr.opcode == OSD_OP_SECONDARY_DELETE)
-            op->reply.sec_del.version = op->bs_op->version;
-    }
-    if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ &&
-        op->reply.hdr.retval > 0)
-    {
-        op->send_list.push_back(op->buf, op->reply.hdr.retval);
-    }
-    else if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST &&
-        op->reply.hdr.retval > 0)
-    {
-        op->buf = op->bs_op->buf; // allocated by blockstore
-        op->send_list.push_back(op->buf, op->reply.hdr.retval * sizeof(obj_ver_id));
-    }
-}