From: Colin Patrick McCabe Date: Fri, 5 Aug 2011 19:49:37 +0000 (-0700) Subject: config: more thread-safety stuff X-Git-Tag: v0.33~15^2~2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=9f608c726cfd504bbacac4b162399b0b291ab9cd;p=ceph.git config: more thread-safety stuff * Don't allow parse_argv, parse_env, or parse_config_files to be used after threads have been started. * Don't allow set_val to be used to change unsafe variables after threads have been started. * Test Signed-off-by: Colin McCabe --- diff --git a/src/common/config.cc b/src/common/config.cc index bb2fb9790bf4..32ddfd4ab904 100644 --- a/src/common/config.cc +++ b/src/common/config.cc @@ -509,6 +509,8 @@ parse_config_files(const char *conf_files, std::deque *parse_errors, int flags) { Mutex::Locker l(lock); + if (internal_safe_to_start_threads) + return -ENOSYS; if (!conf_files) { const char *c = getenv("CEPH_CONF"); if (c) { @@ -583,14 +585,20 @@ void md_config_t:: parse_env() { Mutex::Locker l(lock); + if (internal_safe_to_start_threads) + return; if (getenv("CEPH_KEYRING")) keyring = getenv("CEPH_KEYRING"); } -void md_config_t:: +int md_config_t:: parse_argv(std::vector& args) { Mutex::Locker l(lock); + if (internal_safe_to_start_threads) { + return -ENOSYS; + } + // In this function, don't change any parts of the configuration directly. // Instead, use set_val to set them. This will allow us to send the proper // observer notifications later. @@ -658,6 +666,7 @@ parse_argv(std::vector& args) } } } + return 0; } int md_config_t::parse_injectargs(std::vector& args, @@ -802,8 +811,20 @@ set_val(const char *key, const char *val) for (int i = 0; i < NUM_CONFIG_OPTIONS; ++i) { config_option *opt = &config_optionsp[i]; - if (strcmp(opt->name, k.c_str()) == 0) + if (strcmp(opt->name, k.c_str()) == 0) { + if (internal_safe_to_start_threads) { + // If threads have been started... + if ((opt->type == OPT_STR) || (opt->type == OPT_ADDR)) { + // And this is NOT an integer valued variable.... + if (observers.find(opt->name) == observers.end()) { + // And there is no observer to safely change it... + // You lose. + return -ENOSYS; + } + } + } return set_val_impl(v.c_str(), opt); + } } // couldn't find a configuration option with key 'key' diff --git a/src/common/config.h b/src/common/config.h index 465bb9c772ef..76bd7059302a 100644 --- a/src/common/config.h +++ b/src/common/config.h @@ -105,7 +105,7 @@ public: void parse_env(); // Absorb config settings from argv - void parse_argv(std::vector& args); + int parse_argv(std::vector& args); // Expand all metavariables. Make any pending observer callbacks. void apply_changes(std::ostringstream *oss); diff --git a/src/include/ceph/libceph.h b/src/include/ceph/libceph.h index b5796f9b19d1..f8b16be9f65d 100644 --- a/src/include/ceph/libceph.h +++ b/src/include/ceph/libceph.h @@ -51,7 +51,7 @@ void ceph_shutdown(struct ceph_mount_info *cmount); */ int ceph_conf_read_file(struct ceph_mount_info *cmount, const char *path_list); -void ceph_conf_parse_argv(struct ceph_mount_info *cmount, int argc, const char **argv); +int ceph_conf_parse_argv(struct ceph_mount_info *cmount, int argc, const char **argv); /* Sets a configuration value from a string. * Returns 0 on success, error code otherwise. */ diff --git a/src/include/rados/librados.h b/src/include/rados/librados.h index 63d4e32de6a9..ed1933220d33 100644 --- a/src/include/rados/librados.h +++ b/src/include/rados/librados.h @@ -72,7 +72,7 @@ void rados_shutdown(rados_t cluster); int rados_conf_read_file(rados_t cluster, const char *path); /* Parse argv */ -void rados_conf_parse_argv(rados_t cluster, int argc, const char **argv); +int rados_conf_parse_argv(rados_t cluster, int argc, const char **argv); /* Sets a configuration value from a string. * Returns 0 on success, error code otherwise. */ diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp index d891fe8944fe..24b27def453d 100644 --- a/src/include/rados/librados.hpp +++ b/src/include/rados/librados.hpp @@ -288,7 +288,7 @@ namespace librados int connect(); void shutdown(); int conf_read_file(const char * const path) const; - void conf_parse_argv(int argc, const char ** argv) const; + int conf_parse_argv(int argc, const char ** argv) const; int conf_set(const char *option, const char *value); int conf_get(const char *option, std::string &val); diff --git a/src/libceph.cc b/src/libceph.cc index 2a334f6f53f5..92f5ecc9f04a 100644 --- a/src/libceph.cc +++ b/src/libceph.cc @@ -140,12 +140,16 @@ public: return 0; } - void conf_parse_argv(int argc, const char **argv) + int conf_parse_argv(int argc, const char **argv) { + int ret; vector args; argv_to_vec(argc, argv, args); - cct->_conf->parse_argv(args); + ret = cct->_conf->parse_argv(args); + if (ret) + return ret; cct->_conf->apply_changes(NULL); + return 0; } int conf_set(const char *option, const char *value) @@ -236,10 +240,10 @@ extern "C" int ceph_conf_read_file(struct ceph_mount_info *cmount, const char *p return cmount->conf_read_file(path); } -extern "C" void ceph_conf_parse_argv(struct ceph_mount_info *cmount, int argc, +extern "C" int ceph_conf_parse_argv(struct ceph_mount_info *cmount, int argc, const char **argv) { - cmount->conf_parse_argv(argc, argv); + return cmount->conf_parse_argv(argc, argv); } extern "C" int ceph_conf_set(struct ceph_mount_info *cmount, const char *option, diff --git a/src/librados.cc b/src/librados.cc index 2c6acee08782..3adcc794ed87 100644 --- a/src/librados.cc +++ b/src/librados.cc @@ -3023,10 +3023,10 @@ conf_read_file(const char * const path) const return rados_conf_read_file((rados_t)client, path); } -void librados::Rados:: +int librados::Rados:: conf_parse_argv(int argc, const char ** argv) const { - rados_conf_parse_argv((rados_t)client, argc, argv); + return rados_conf_parse_argv((rados_t)client, argc, argv); } int librados::Rados:: @@ -3263,14 +3263,17 @@ extern "C" int rados_conf_read_file(rados_t cluster, const char *path_list) return 0; } -extern "C" void rados_conf_parse_argv(rados_t cluster, int argc, const char **argv) +extern "C" int rados_conf_parse_argv(rados_t cluster, int argc, const char **argv) { librados::RadosClient *client = (librados::RadosClient *)cluster; md_config_t *conf = client->cct->_conf; vector args; argv_to_vec(argc, argv, args); - conf->parse_argv(args); + int ret = conf->parse_argv(args); + if (ret) + return ret; conf->apply_changes(NULL); + return 0; } extern "C" int rados_conf_set(rados_t cluster, const char *option, const char *value) diff --git a/src/test/daemon_config.cc b/src/test/daemon_config.cc index 0c97dbbac69a..938531ab7686 100644 --- a/src/test/daemon_config.cc +++ b/src/test/daemon_config.cc @@ -39,6 +39,9 @@ TEST(DaemonConfig, SimpleSet) { } TEST(DaemonConfig, ArgV) { + ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads", + "false")); + int ret; const char *argv[] = { "foo", "--debug", "22", "--keyfile", "/tmp/my-keyfile", NULL }; @@ -59,6 +62,9 @@ TEST(DaemonConfig, ArgV) { ret = g_ceph_context->_conf->get_val("debug", &tmp, sizeof(buf)); ASSERT_EQ(ret, 0); ASSERT_EQ(string("22"), string(buf)); + + ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads", + "true")); } TEST(DaemonConfig, InjectArgs) { @@ -162,3 +168,31 @@ TEST(DaemonConfig, InjectArgsLogfile) { // Clean up the garbage unlink(tmpfile); } + +TEST(DaemonConfig, ThreadSafety1) { + int ret; + // Verify that we can't change this, since internal_safe_to_start_threads has + // been set. + ret = g_ceph_context->_conf->set_val("osd_data", ""); + ASSERT_EQ(ret, -ENOSYS); + + ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads", + "false")); + + // Ok, now we can change this. Since this is just a test, and there are no + // OSD threads running, we know changing osd_data won't actually blow up the + // world. + ret = g_ceph_context->_conf->set_val("osd_data", "/tmp/crazydata"); + ASSERT_EQ(ret, 0); + + char buf[128]; + char *tmp = buf; + memset(buf, 0, sizeof(buf)); + ret = g_ceph_context->_conf->get_val("osd_data", &tmp, sizeof(buf)); + ASSERT_EQ(ret, 0); + ASSERT_EQ(string("/tmp/crazydata"), string(buf)); + + ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads", + "false")); + ASSERT_EQ(ret, 0); +}