common: asok should drop lock to call hook

Otherwise, hooks cannot take any locks which
were held during registration of hooks, because
it creates a cycle.

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

diff --git a/src/common/admin_socket.cc b/src/common/admin_socket.cc
index aa0146fe1c3bbccbf2989a2ead45f5949984dd69..2c86949d1f02cc67e2ef817b578ba1b00184cd07 100644 (file)
--- a/src/common/admin_socket.cc
+++ b/src/common/admin_socket.cc
@@ -109,6 +109,7 @@ AdminSocket::AdminSocket(CephContext *cct)
     m_sock_fd(-1),
     m_shutdown_rd_fd(-1),
     m_shutdown_wr_fd(-1),
+    in_hook(false),
     m_lock("AdminSocket::m_lock"),
     m_version_hook(NULL),
     m_help_hook(NULL),
@@ -384,9 +385,22 @@ bool AdminSocket::do_accept()
     lderr(m_cct) << "AdminSocket: request '" << c << "' not defined" << dendl;
   } else {
     string args;
-    if (match != c)
+    if (match != c) {
       args = c.substr(match.length() + 1);
-    bool success = p->second->call(match, cmdmap, format, out);
+    }
+
+    // Drop lock to avoid cycles in cases where the hook takes
+    // the same lock that was held during calls to register/unregister,
+    // and set in_hook to allow unregister to wait for us before
+    // removing this hook.
+    in_hook = true;
+    auto match_hook = p->second;
+    m_lock.Unlock();
+    bool success = match_hook->call(match, cmdmap, format, out);
+    m_lock.Lock();
+    in_hook = false;
+    in_hook_cond.Signal();
+
     if (!success) {
       ldout(m_cct, 0) << "AdminSocket: request '" << match << "' args '" << args
                      << "' to " << p->second << " failed" << dendl;
@@ -439,6 +453,14 @@ int AdminSocket::unregister_command(std::string command)
     m_hooks.erase(command);
     m_descs.erase(command);
     m_help.erase(command);
+
+    // If we are currently processing a command, wait for it to
+    // complete in case it referenced the hook that we are
+    // unregistering.
+    if (in_hook) {
+      in_hook_cond.Wait(m_lock);
+    }
+
     ret = 0;
   } else {
     ldout(m_cct, 5) << "unregister_command " << command << " ENOENT" << dendl;

diff --git a/src/common/admin_socket.h b/src/common/admin_socket.h
index bad235a277f6eecab49c04a3a6dc7e487dc4a101..3a6351da6fd36cf732df27799448dc2ba2d7b909 100644 (file)
--- a/src/common/admin_socket.h
+++ b/src/common/admin_socket.h
@@ -22,6 +22,7 @@
 #include <map>
 #include "include/buffer_fwd.h"
 #include "common/cmdparse.h"
+#include "common/Cond.h"
 
 class AdminSocket;
 class CephContext;
@@ -63,7 +64,11 @@ public:
   int register_command(std::string command, std::string cmddesc, AdminSocketHook *hook, std::string help);
 
   /**
-   * unregister an admin socket command
+   * unregister an admin socket command.
+   *
+   * If a command is currently in progress, this will block until it
+   * is done.  For that reason, you must not hold any locks required
+   * by your hook while you call this.
    *
    * @param command command string
    * @return 0 on succest, -ENOENT if command dne.
@@ -91,6 +96,8 @@ private:
   int m_shutdown_rd_fd;
   int m_shutdown_wr_fd;
 
+  bool in_hook;
+  Cond in_hook_cond;
   Mutex m_lock;    // protects m_hooks, m_descs, m_help
   AdminSocketHook *m_version_hook, *m_help_hook, *m_getdescs_hook;