common/config: make internal_safe_to_start_threads internal

There is no reason for this to be exposed like a normal config option
(even a legacy one) since it is an internal safety flag.

We do keep the clear_() helper there for the unit test.

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/common/ceph_context.cc b/src/common/ceph_context.cc
index 2f5256baf5479c0f58ef05de7338d29d3b0955bf..b2b3957fcb6d15d64cdefa6865019eac1049abee 100644 (file)
--- a/src/common/ceph_context.cc
+++ b/src/common/ceph_context.cc
@@ -743,7 +743,7 @@ void CephContext::start_service_thread()
 
   // Trigger callbacks on any config observers that were waiting for
   // it to become safe to start threads.
-  _conf->set_val("internal_safe_to_start_threads", "true");
+  _conf->set_safe_to_start_threads();
   _conf->call_all_observers();
 
   // start admin socket

diff --git a/src/common/config.cc b/src/common/config.cc
index 404928f0e369e86f211753d03d98dcaf450635b6..44fc75010676835419bdf659935f16360bf74292 100644 (file)
--- a/src/common/config.cc
+++ b/src/common/config.cc
@@ -221,7 +221,7 @@ int md_config_t::parse_config_files(const char *conf_files,
 {
   Mutex::Locker l(lock);
 
-  if (internal_safe_to_start_threads)
+  if (safe_to_start_threads)
     return -ENOSYS;
 
   if (!cluster.size() && !conf_files) {
@@ -372,7 +372,7 @@ int md_config_t::parse_config_files_impl(const std::list<std::string> &conf_file
 void md_config_t::parse_env()
 {
   Mutex::Locker l(lock);
-  if (internal_safe_to_start_threads)
+  if (safe_to_start_threads)
     return;
   if (getenv("CEPH_KEYRING")) {
     set_val_or_die("keyring", getenv("CEPH_KEYRING"));
@@ -430,7 +430,7 @@ void md_config_t::_show_config(std::ostream *out, Formatter *f)
 int md_config_t::parse_argv(std::vector<const char*>& args)
 {
   Mutex::Locker l(lock);
-  if (internal_safe_to_start_threads) {
+  if (safe_to_start_threads) {
     return -ENOSYS;
   }
 
@@ -662,13 +662,6 @@ void md_config_t::apply_changes(std::ostream *oss)
     _apply_changes(oss);
 }
 
-bool md_config_t::_internal_field(const string& s)
-{
-  if (s == "internal_safe_to_start_threads")
-    return true;
-  return false;
-}
-
 void md_config_t::_apply_changes(std::ostream *oss)
 {
   /* Maps observers to the configuration options that they care about which
@@ -698,8 +691,7 @@ void md_config_t::_apply_changes(std::ostream *oss)
     pair < obs_map_t::iterator, obs_map_t::iterator >
       range(observers.equal_range(key));
     if ((oss) &&
-       (!_get_val(key.c_str(), &bufptr, sizeof(buf))) &&
-       !_internal_field(key)) {
+       !_get_val(key.c_str(), &bufptr, sizeof(buf))) {
       (*oss) << key << " = '" << buf << "' ";
       if (range.first == range.second) {
        (*oss) << "(not observed, change may require restart) ";
@@ -750,6 +742,16 @@ void md_config_t::call_all_observers()
   }
 }
 
+void md_config_t::set_safe_to_start_threads()
+{
+  safe_to_start_threads = true;
+}
+
+void md_config_t::_clear_safe_to_start_threads()
+{
+  safe_to_start_threads = false;
+}
+
 int md_config_t::injectargs(const std::string& s, std::ostream *oss)
 {
   int ret;
@@ -839,7 +841,7 @@ int md_config_t::set_val(const std::string &key, const char *val,
   const auto &opt_iter = schema.find(k);
   if (opt_iter != schema.end()) {
     const Option &opt = opt_iter->second;
-    if ((!opt.is_safe()) && internal_safe_to_start_threads) {
+    if ((!opt.is_safe()) && safe_to_start_threads) {
       // If threads have been started and the option is not thread safe
       if (observers.find(opt.name) == observers.end()) {
         // And there is no observer to safely change it...

diff --git a/src/common/config.h b/src/common/config.h
index a4efd34efe74d4f62d03a68a812bedf4ea03e84d..e0266d882ca262c49ac755b0367120591586df85 100644 (file)
--- a/src/common/config.h
+++ b/src/common/config.h
@@ -55,7 +55,7 @@ extern const char *CEPH_CONF_FILE_DEFAULT;
  *
  * To prevent serious problems resulting from thread-safety issues, we disallow
  * changing std::string configuration values after
- * md_config_t::internal_safe_to_start_threads becomes true. You can still
+ * md_config_t::safe_to_start_threads becomes true. You can still
  * change integer or floating point values, and the option declared with
  * SAFE_OPTION macro. Notice the latter options can not be read directly
  * (conf->foo), one should use either observers or get_val() method
@@ -139,6 +139,9 @@ public:
   bool _internal_field(const string& k);
   void call_all_observers();
 
+  void set_safe_to_start_threads();
+  void _clear_safe_to_start_threads();  // this is only used by the unit test
+
   // Called by the Ceph daemons to make configuration changes at runtime
   int injectargs(const std::string &s, std::ostream *oss);
 
@@ -255,6 +258,10 @@ public:
   std::deque<std::string> parse_errors;
 private:
 
+  // This will be set to true when it is safe to start threads.
+  // Once it is true, it will never change.
+  bool safe_to_start_threads = false;
+
   obs_map_t observers;
   changed_set_t changed;
 

diff --git a/src/common/legacy_config_opts.h b/src/common/legacy_config_opts.h
index e7102a0d97b006c05dfc55fa08c7b85d73fcc326..7f65bb940a74642d3b5ed5bb00111565aa4fd2b6 100644 (file)
--- a/src/common/legacy_config_opts.h
+++ b/src/common/legacy_config_opts.h
@@ -1519,10 +1519,6 @@ OPTION(rgw_torrent_sha_unit, OPT_INT)    // torrent field piece length 512K
 
 OPTION(event_tracing, OPT_BOOL) // true if LTTng-UST tracepoints should be enabled
 
-// This will be set to true when it is safe to start threads.
-// Once it is true, it will never change.
-OPTION(internal_safe_to_start_threads, OPT_BOOL)
-
 OPTION(debug_deliberately_leak_memory, OPT_BOOL)
 
 OPTION(rgw_swift_custom_header, OPT_STR) // option to enable swift custom headers

diff --git a/src/common/options.cc b/src/common/options.cc
index 2e70f1c592ec34980df6410031c52b8672888303..6064e0c76bf69aeb6815e3f800c6f00bafc245dd 100644 (file)
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -4291,10 +4291,6 @@ std::vector<Option> get_global_options() {
     .set_default(false)
     .set_description(""),
 
-    Option("internal_safe_to_start_threads", Option::TYPE_BOOL, Option::LEVEL_ADVANCED)
-    .set_default(false)
-    .set_description(""),
-
     Option("debug_deliberately_leak_memory", Option::TYPE_BOOL, Option::LEVEL_DEV)
     .set_default(false)
     .set_description(""),

diff --git a/src/test/daemon_config.cc b/src/test/daemon_config.cc
index fc3a5c7a9859747a35d67523a139fdd25fae46d1..c1bdd238525321441e87ef0b10ad595beac22501 100644 (file)
--- a/src/test/daemon_config.cc
+++ b/src/test/daemon_config.cc
@@ -44,8 +44,7 @@ TEST(DaemonConfig, SimpleSet) {
 
 TEST(DaemonConfig, Substitution) {
   int ret;
-  ret = g_ceph_context->_conf->set_val("internal_safe_to_start_threads", "false");
-  ASSERT_EQ(0, ret);
+  g_conf->_clear_safe_to_start_threads();
   ret = g_ceph_context->_conf->set_val("host", "foo");
   ASSERT_EQ(0, ret);
   ret = g_ceph_context->_conf->set_val("public_network", "bar$host.baz", false);
@@ -61,8 +60,7 @@ TEST(DaemonConfig, Substitution) {
 
 TEST(DaemonConfig, SubstitutionTrailing) {
   int ret;
-  ret = g_ceph_context->_conf->set_val("internal_safe_to_start_threads", "false");
-  ASSERT_EQ(0, ret);
+  g_conf->_clear_safe_to_start_threads();
   ret = g_ceph_context->_conf->set_val("host", "foo");
   ASSERT_EQ(0, ret);
   ret = g_ceph_context->_conf->set_val("public_network", "bar$host", false);
@@ -78,8 +76,7 @@ TEST(DaemonConfig, SubstitutionTrailing) {
 
 TEST(DaemonConfig, SubstitutionBraces) {
   int ret;
-  ret = g_ceph_context->_conf->set_val("internal_safe_to_start_threads", "false");
-  ASSERT_EQ(0, ret);
+  g_conf->_clear_safe_to_start_threads();
   ret = g_ceph_context->_conf->set_val("host", "foo");
   ASSERT_EQ(0, ret);
   ret = g_ceph_context->_conf->set_val("public_network", "bar${host}baz", false);
@@ -94,8 +91,7 @@ TEST(DaemonConfig, SubstitutionBraces) {
 }
 TEST(DaemonConfig, SubstitutionBracesTrailing) {
   int ret;
-  ret = g_ceph_context->_conf->set_val("internal_safe_to_start_threads", "false");
-  ASSERT_EQ(0, ret);
+  g_conf->_clear_safe_to_start_threads();
   ret = g_ceph_context->_conf->set_val("host", "foo");
   ASSERT_EQ(0, ret);
   ret = g_ceph_context->_conf->set_val("public_network", "bar${host}", false);
@@ -127,8 +123,7 @@ TEST(DaemonConfig, SubstitutionMultiple) {
 }
 
 TEST(DaemonConfig, ArgV) {
-  ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads",
-                                      "false"));
+  g_conf->_clear_safe_to_start_threads();
 
   int ret;
   const char *argv[] = { "foo", "--log-graylog-port", "22",
@@ -151,8 +146,7 @@ TEST(DaemonConfig, ArgV) {
   ASSERT_EQ(0, ret);
   ASSERT_EQ(string("22"), string(buf));
 
-  ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads",
-                                      "true"));
+  g_conf->set_safe_to_start_threads();
 }
 
 TEST(DaemonConfig, InjectArgs) {
@@ -317,13 +311,12 @@ TEST(DaemonConfig, InjectArgsLogfile) {
 
 TEST(DaemonConfig, ThreadSafety1) {
   int ret;
-  // Verify that we can't change this, since internal_safe_to_start_threads has
+  // Verify that we can't change this, since safe_to_start_threads has
   // been set.
   ret = g_ceph_context->_conf->set_val("osd_data", "");
   ASSERT_EQ(-ENOSYS, ret);
 
-  ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads",
-                                      "false"));
+  g_conf->_clear_safe_to_start_threads();
 
   // 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
@@ -338,8 +331,7 @@ TEST(DaemonConfig, ThreadSafety1) {
   ASSERT_EQ(0, ret);
   ASSERT_EQ(string("/tmp/crazydata"), string(buf));
 
-  ASSERT_EQ(0, g_ceph_context->_conf->set_val("internal_safe_to_start_threads",
-                                      "false"));
+  g_conf->_clear_safe_to_start_threads();
   ASSERT_EQ(0, ret);
 }