From: Adam C. Emerson Date: Tue, 30 Jan 2018 05:05:49 +0000 (-0500) Subject: common/admin_socket: Use std::mutex/condition_variable/thread X-Git-Tag: v13.0.2~240^2~4 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=c47d3dea85fabca2fef032666ecac883d418c7a8;p=ceph.git common/admin_socket: Use std::mutex/condition_variable/thread And fix a bug where we exit in one code-path without releasing the lock. Signed-off-by: Adam C. Emerson --- diff --git a/src/common/admin_socket.cc b/src/common/admin_socket.cc index 9478736a16b1..49d988b6f465 100644 --- a/src/common/admin_socket.cc +++ b/src/common/admin_socket.cc @@ -11,17 +11,19 @@ * Foundation. See file COPYING. * */ +#include +#include #include "common/admin_socket.h" #include "common/admin_socket_client.h" +#include "common/dout.h" #include "common/errno.h" #include "common/pipe.h" #include "common/safe_io.h" +#include "common/Thread.h" #include "common/version.h" #include "include/compat.h" -#include -#include // re-include our assert to clobber the system one; fix dout: #include "include/assert.h" @@ -42,7 +44,7 @@ using std::ostringstream; * the application exits normally. */ static pthread_mutex_t cleanup_lock = PTHREAD_MUTEX_INITIALIZER; -static std::vector cleanup_files; +static std::vector cleanup_files; static bool cleanup_atexit = false; static void remove_cleanup_file(const char *file) @@ -88,17 +90,8 @@ static void add_cleanup_file(const char *file) AdminSocket::AdminSocket(CephContext *cct) - : m_cct(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), - m_getdescs_hook(NULL) -{ -} + : m_cct(cct) +{} AdminSocket::~AdminSocket() { @@ -150,7 +143,7 @@ std::string AdminSocket::destroy_shutdown_pipe() return oss.str(); } - join(); + th.join(); // Close read end. Doing this before join() blocks the listenter and prevents // joining. @@ -236,7 +229,7 @@ std::string AdminSocket::bind_and_listen(const std::string &sock_path, int *fd) return ""; } -void* AdminSocket::entry() +void AdminSocket::entry() noexcept { ldout(m_cct, 5) << "entry start" << dendl; while (true) { @@ -255,7 +248,7 @@ void* AdminSocket::entry() } lderr(m_cct) << "AdminSocket: poll(2) error: '" << cpp_strerror(err) << dendl; - return PFL_FAIL; + return; } if (fds[0].revents & POLLIN) { @@ -264,7 +257,7 @@ void* AdminSocket::entry() } if (fds[1].revents & POLLIN) { // Parent wants us to shut down - return PFL_SUCCESS; + return; } } ldout(m_cct, 5) << "entry exit" << dendl; @@ -375,7 +368,7 @@ bool AdminSocket::do_accept() format = "json-pretty"; cmd_getval(m_cct, cmdmap, "prefix", c); - m_lock.Lock(); + std::unique_lock l(lock); map::iterator p; string match = c; while (match.size()) { @@ -408,12 +401,12 @@ bool AdminSocket::do_accept() // removing this hook. in_hook = true; auto match_hook = p->second; - m_lock.Unlock(); + l.unlock(); bool success = (validate(match, cmdmap, out) && match_hook->call(match, cmdmap, format, out)); - m_lock.Lock(); + l.lock(); in_hook = false; - in_hook_cond.Signal(); + in_hook_cond.notify_all(); if (!success) { ldout(m_cct, 0) << "AdminSocket: request '" << match << "' args '" << args @@ -434,7 +427,7 @@ bool AdminSocket::do_accept() rval = true; } } - m_lock.Unlock(); + l.unlock(); VOID_TEMP_FAILURE_RETRY(close(connection_fd)); return rval; @@ -459,7 +452,7 @@ int AdminSocket::register_command(std::string_view command, std::string_view help) { int ret; - m_lock.Lock(); + std::unique_lock l(lock); if (m_hooks.count(command)) { ldout(m_cct, 5) << "register_command " << command << " hook " << hook << " EEXIST" << dendl; ret = -EEXIST; @@ -470,14 +463,13 @@ int AdminSocket::register_command(std::string_view command, m_help.emplace(command, help); ret = 0; } - m_lock.Unlock(); return ret; } int AdminSocket::unregister_command(std::string_view command) { int ret; - m_lock.Lock(); + std::unique_lock l(lock); if (m_hooks.count(command)) { ldout(m_cct, 5) << "unregister_command " << command << dendl; m_hooks.erase(m_hooks.find(command)); @@ -487,16 +479,13 @@ int AdminSocket::unregister_command(std::string_view 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); - } + in_hook_cond.wait(l, [this]() { return !in_hook; }); ret = 0; } else { ldout(m_cct, 5) << "unregister_command " << command << " ENOENT" << dendl; ret = -ENOENT; - } - m_lock.Unlock(); + } return ret; } @@ -612,7 +601,7 @@ bool AdminSocket::init(const std::string& path) register_command("get_command_descriptions", "get_command_descriptions", m_getdescs_hook, "list available commands"); - create("admin_socket"); + th = make_named_thread("admin_socket", &AdminSocket::entry, this); add_cleanup_file(m_path.c_str()); return true; } diff --git a/src/common/admin_socket.h b/src/common/admin_socket.h index 36d96178a0a3..8d9471474096 100644 --- a/src/common/admin_socket.h +++ b/src/common/admin_socket.h @@ -15,13 +15,20 @@ #ifndef CEPH_COMMON_ADMIN_SOCKET_H #define CEPH_COMMON_ADMIN_SOCKET_H +#include +#include +#include #include +#include -#include "common/Cond.h" +#include "include/buffer.h" +#include "common/cmdparse.h" class AdminSocket; class CephContext; +using namespace std::literals; + inline constexpr auto CEPH_ADMIN_SOCK_VERSION = "2"sv; class AdminSocketHook { @@ -31,11 +38,11 @@ public: virtual ~AdminSocketHook() {} }; -class AdminSocket : public Thread +class AdminSocket { public: AdminSocket(CephContext *cct); - ~AdminSocket() override; + ~AdminSocket(); AdminSocket(const AdminSocket&) = delete; AdminSocket& operator =(const AdminSocket&) = delete; @@ -78,7 +85,7 @@ public: */ int unregister_command(std::string_view command); - bool init(const string& path); + bool init(const std::string& path); void chown(uid_t uid, gid_t gid); void chmod(mode_t mode); @@ -91,7 +98,8 @@ private: std::string destroy_shutdown_pipe(); std::string bind_and_listen(const std::string &sock_path, int *fd); - void *entry() override; + std::thread th; + void entry() noexcept; bool do_accept(); bool validate(const std::string& command, const cmdmap_t& cmdmap, @@ -99,14 +107,15 @@ private: CephContext *m_cct; std::string m_path; - int m_sock_fd; - 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; + int m_sock_fd = -1; + int m_shutdown_rd_fd = -1; + int m_shutdown_wr_fd = -1; + + bool in_hook = false; + std::condition_variable in_hook_cond; + std::mutex lock; // protects m_hooks, m_descs, m_help + AdminSocketHook *m_version_hook = nullptr, *m_help_hook = nullptr, + *m_getdescs_hook = nullptr; std::map> m_hooks; std::map> m_descs; diff --git a/src/common/admin_socket_client.cc b/src/common/admin_socket_client.cc index 32eb66479c09..3742c3f1b2b2 100644 --- a/src/common/admin_socket_client.cc +++ b/src/common/admin_socket_client.cc @@ -12,13 +12,15 @@ * */ +#include +#include +#include + #include "common/admin_socket.h" #include "common/errno.h" #include "common/safe_io.h" #include "common/admin_socket_client.h" -#include - using std::ostringstream; const char* get_rand_socket_path() diff --git a/src/common/ceph_context.cc b/src/common/ceph_context.cc index c3533539eb53..a19989fcb08e 100644 --- a/src/common/ceph_context.cc +++ b/src/common/ceph_context.cc @@ -24,10 +24,15 @@ #include "common/admin_socket.h" #include "common/perf_counters.h" #include "common/code_environment.h" +#include "common/Cond.h" +#include "common/config.h" #include "common/ceph_crypto.h" #include "common/HeartbeatMap.h" #include "common/errno.h" #include "common/Graylog.h" + +#include "log/Log.h" + #include "auth/Crypto.h" #include "include/str_list.h" #include "common/PluginRegistry.h" diff --git a/src/common/common_init.cc b/src/common/common_init.cc index d3114026c40a..5b37962f9b1d 100644 --- a/src/common/common_init.cc +++ b/src/common/common_init.cc @@ -14,7 +14,10 @@ #include "common/admin_socket.h" #include "common/ceph_argparse.h" +#include "common/ceph_context.h" #include "common/common_init.h" +#include "common/config.h" +#include "common/dout.h" #include "common/valgrind.h" #include "common/zipkin_trace.h"