common: refactor CommandTable

Avoid handling out raw pointers to the ops,
which are in fact owned by the table.

Signed-off-by: John Spray <john.spray@redhat.com>

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 0da3fd9cd7ccd1d59de4f84ab2adf9b0a9d62b08..58bc293982e8bf612210dc3d0dbd472c184463b7 100644 (file)
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -2545,21 +2545,21 @@ void Client::handle_mds_map(MMDSMap* m)
 
   // Cancel any commands for missing or laggy GIDs
   std::list<ceph_tid_t> cancel_ops;
-  auto commands = command_table.get_commands();
+  auto &commands = command_table.get_commands();
   for (const auto &i : commands) {
-    const MDSCommandOp *op = i.second;
-    const mds_gid_t op_mds_gid = op->mds_gid;
+    auto &op = i.second;
+    const mds_gid_t op_mds_gid = op.mds_gid;
     if (mdsmap->is_dne_gid(op_mds_gid) || mdsmap->is_laggy_gid(op_mds_gid)) {
       ldout(cct, 1) << __func__ << ": cancelling command op " << i.first << dendl;
       cancel_ops.push_back(i.first);
-      if (op->outs) {
+      if (op.outs) {
         std::ostringstream ss;
         ss << "MDS " << op_mds_gid << " went away";
-        *(op->outs) = ss.str();
+        *(op.outs) = ss.str();
       }
-      op->con->mark_down();
-      if (op->on_finish) {
-        op->on_finish->complete(-ETIMEDOUT);
+      op.con->mark_down();
+      if (op.on_finish) {
+        op.on_finish->complete(-ETIMEDOUT);
       }
     }
   }
@@ -5506,21 +5506,21 @@ int Client::mds_command(
     ConnectionRef conn = messenger->get_connection(inst);
 
     // Generate MDSCommandOp state
-    MDSCommandOp *op = command_table.start_command();
+    auto &op = command_table.start_command();
 
-    op->on_finish = gather.new_sub();
-    op->cmd = cmd;
-    op->outbl = outbl;
-    op->outs = outs;
-    op->inbl = inbl;
-    op->mds_gid = target_gid;
-    op->con = conn;
+    op.on_finish = gather.new_sub();
+    op.cmd = cmd;
+    op.outbl = outbl;
+    op.outs = outs;
+    op.inbl = inbl;
+    op.mds_gid = target_gid;
+    op.con = conn;
 
     ldout(cct, 4) << __func__ << ": new command op to " << target_gid
-      << " tid=" << op->tid << cmd << dendl;
+      << " tid=" << op.tid << cmd << dendl;
 
     // Construct and send MCommand
-    MCommand *m = op->get_message(monclient->get_fsid());
+    MCommand *m = op.get_message(monclient->get_fsid());
     conn->send_message(m);
   }
   gather.activate();
@@ -5534,22 +5534,22 @@ void Client::handle_command_reply(MCommandReply *m)
 
   ldout(cct, 10) << __func__ << ": tid=" << m->get_tid() << dendl;
 
-  MDSCommandOp *op = command_table.get_command(tid);
-  if (op == nullptr) {
+  if (!command_table.exists(tid)) {
     ldout(cct, 1) << __func__ << ": unknown tid " << tid << ", dropping" << dendl;
     m->put();
     return;
   }
 
-  if (op->outbl) {
-    op->outbl->claim(m->get_data());
+  auto &op = command_table.get_command(tid);
+  if (op.outbl) {
+    op.outbl->claim(m->get_data());
   }
-  if (op->outs) {
-    *op->outs = m->rs;
+  if (op.outs) {
+    *op.outs = m->rs;
   }
 
-  if (op->on_finish) {
-    op->on_finish->complete(m->r);
+  if (op.on_finish) {
+    op.on_finish->complete(m->r);
   }
 
   command_table.erase(tid);

diff --git a/src/common/CommandTable.h b/src/common/CommandTable.h
index 6be67578eafe3a813b8ca9b56db35bae96a34834..6aab1b8c24b1ba37f203776ff3c32c1d2316d11c 100644 (file)
--- a/src/common/CommandTable.h
+++ b/src/common/CommandTable.h
@@ -3,7 +3,7 @@
 /*
  * Ceph - scalable distributed file system
  *
- * Copyright (C) 2004-2006 Sage Weil <sage@newdream.net>
+ * Copyright (C) 2016 Red Hat Inc
  *
  * This is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -39,15 +39,21 @@ class CommandOp
     return m;
   }
 
-  CommandOp(const ceph_tid_t t) : tid(t) {}
+  CommandOp(const ceph_tid_t t) : tid(t), on_finish(nullptr),
+                                  outbl(nullptr), outs(nullptr) {}
+  CommandOp() : tid(0), on_finish(nullptr), outbl(nullptr), outs(nullptr) {}
 };
 
+/**
+ * Hold client-side state for a collection of in-flight commands
+ * to a remote service.
+ */
 template<typename T>
 class CommandTable
 {
 protected:
   ceph_tid_t last_tid;
-  std::map<ceph_tid_t, T*> commands;
+  std::map<ceph_tid_t, T> commands;
 
 public:
 
@@ -55,34 +61,36 @@ public:
     : last_tid(0)
   {}
 
-  T* start_command()
+  ~CommandTable()
+  {
+    assert(commands.empty());
+  }
+
+  T& start_command()
   {
     ceph_tid_t tid = last_tid++;
-    auto cmd = new T(tid);
-    commands[tid] = cmd;
-    
-    return cmd;
+    commands.insert(std::make_pair(tid, T(tid)) );
+
+    return commands.at(tid);
   }
 
-  const std::map<ceph_tid_t, T*> &get_commands() const
+  const std::map<ceph_tid_t, T> &get_commands() const
   {
     return commands;
   }
 
-  T* get_command(ceph_tid_t tid)
+  bool exists(ceph_tid_t tid) const
+  {
+    return commands.count(tid) > 0;
+  }
+
+  T& get_command(ceph_tid_t tid)
   {
-    auto result = commands.find(tid);
-    if (result == commands.end()) {
-      return nullptr;
-    } else {
-      return result->second;
-    }
+    return commands.at(tid);
   }
 
   void erase(ceph_tid_t tid)
   {
-    auto ptr = commands.at(tid);
-    delete ptr;
     commands.erase(tid);
   }
 };

diff --git a/src/mgr/MgrClient.cc b/src/mgr/MgrClient.cc
index d0baeb4d37446e59de6f618dfd57d107a6f6ec3e..bc272d34381190eecb44b83b59e497df32f18b87 100644 (file)
--- a/src/mgr/MgrClient.cc
+++ b/src/mgr/MgrClient.cc
@@ -109,8 +109,8 @@ bool MgrClient::handle_mgr_map(MMgrMap *m)
       auto commands = command_table.get_commands();
       for (const auto &i : commands) {
         // FIXME be nicer, retarget command on new mgr?
-        if (i.second->on_finish != nullptr) {
-          i.second->on_finish->complete(-ETIMEDOUT);
+        if (i.second.on_finish != nullptr) {
+          i.second.on_finish->complete(-ETIMEDOUT);
         }
         erase_cmds.push_back(i.first);
       }
@@ -296,15 +296,15 @@ int MgrClient::start_command(const vector<string>& cmd, const bufferlist& inbl,
 
   assert(map.epoch > 0);
 
-  MgrCommand *op = command_table.start_command();
-  op->cmd = cmd;
-  op->inbl = inbl;
-  op->outbl = outbl;
-  op->outs = outs;
-  op->on_finish = onfinish;
+  auto &op = command_table.start_command();
+  op.cmd = cmd;
+  op.inbl = inbl;
+  op.outbl = outbl;
+  op.outs = outs;
+  op.on_finish = onfinish;
 
   // Leaving fsid argument null because it isn't used.
-  MCommand *m = op->get_message({});
+  MCommand *m = op.get_message({});
   assert(session);
   assert(session->con);
   session->con->send_message(m);
@@ -319,24 +319,24 @@ bool MgrClient::handle_command_reply(MCommandReply *m)
   ldout(cct, 20) << *m << dendl;
 
   const auto tid = m->get_tid();
-  const auto op = command_table.get_command(tid);
-  if (op == nullptr) {
+  if (!command_table.exists(tid)) {
     ldout(cct, 4) << "handle_command_reply tid " << m->get_tid()
             << " not found" << dendl;
     m->put();
     return true;
   }
 
-  if (op->outbl) {
-    op->outbl->claim(m->get_data());
+  auto &op = command_table.get_command(tid);
+  if (op.outbl) {
+    op.outbl->claim(m->get_data());
   }
 
-  if (op->outs) {
-    *(op->outs) = m->rs;
+  if (op.outs) {
+    *(op.outs) = m->rs;
   }
 
-  if (op->on_finish) {
-    op->on_finish->complete(m->r);
+  if (op.on_finish) {
+    op.on_finish->complete(m->r);
   }
 
   command_table.erase(tid);

diff --git a/src/mgr/MgrClient.h b/src/mgr/MgrClient.h
index 705dea8c4ef310823b58a7c944138f0a2db21a47..396bb7473b0b74e48655f94db39dab7abe3050fd 100644 (file)
--- a/src/mgr/MgrClient.h
+++ b/src/mgr/MgrClient.h
@@ -44,6 +44,7 @@ class MgrCommand : public CommandOp
   public:
 
   MgrCommand(ceph_tid_t t) : CommandOp(t) {}
+  MgrCommand() : CommandOp() {}
 };
 
 class MgrClient : public Dispatcher