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 <colin.mccabe@dreamhost.com>

diff --git a/src/common/config.cc b/src/common/config.cc
index bb2fb9790bf4eff2bfa25d81aaba403a3ad756db..32ddfd4ab904725e5e8c84dd69eca2af51a553ae 100644 (file)
--- a/src/common/config.cc
+++ b/src/common/config.cc
@@ -509,6 +509,8 @@ parse_config_files(const char *conf_files,
                   std::deque<std::string> *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<const char*>& 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<const char*>& args)
       }
     }
   }
+  return 0;
 }
 
 int md_config_t::parse_injectargs(std::vector<const char*>& 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 465bb9c772ef6dca86624736694cc0abb50e43b1..76bd7059302a853e17dbd792ffe4ce03b93615e5 100644 (file)
--- 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<const char*>& args);
+  int parse_argv(std::vector<const char*>& 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 b5796f9b19d16c35a66876bf321bd0a700ce101f..f8b16be9f65ddd42119b394cbff860f02a5312b4 100644 (file)
--- 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 63d4e32de6a96f80b6e8eae5c98d113e81a711f4..ed1933220d33fa4574892482a2233ce41e19d6ab 100644 (file)
--- 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 d891fe8944fe50e8f4dc2e20eef2a3b2fde4fc18..24b27def453dfc88a5cd393a36a842ae8313dbdf 100644 (file)
--- 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 2a334f6f53f5f69c0c2c437f6308df57037af0d6..92f5ecc9f04a1e79ce15735dfd0a223a82948cfb 100644 (file)
--- 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<const char*> 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 2c6acee08782ab65cb6607bf91121a733d40d51b..3adcc794ed8745cd5b84d17188d5063e28811879 100644 (file)
--- 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<const char*> 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 0c97dbbac69aa5f3d664b242c8ed2734abd017b5..938531ab7686a1b9ca5a80c4f963e0848e86a789 100644 (file)
--- 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);
+}