bluestore/NVMEDevice: do not deference a dangled pointer

* pass coremask_arg to the working thread by value.
  after 81249ab9d, get_val<>() returns a temporary variables instead of a
  reference to the variant held by config. so calling string::c_str(), and
  passing the returned `const char*` to a thread is not advisable. instead
  we should pass a string by value. since spdk_env_init() will copy the
  settings passed in by opt. it's safe even to destruct the coremask_arg
  afterward.
* use ffsll() to find the LSB. it's not a bottleneck, but it's easier
  and simpler, and probably improves the readability.
* refactor the NVMEManager::try_get() method: to define the variables
  when they are used for the first time.

Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/os/bluestore/NVMEDevice.cc b/src/os/bluestore/NVMEDevice.cc
index c4510665a8ef5eb27cdfaeaf9e76af265dab1788..9a357cb820acabf63309f3da1634e126179555cc 100644 (file)
--- a/src/os/bluestore/NVMEDevice.cc
+++ b/src/os/bluestore/NVMEDevice.cc
@@ -17,6 +17,7 @@
 
 #include <unistd.h>
 #include <stdlib.h>
+#include <strings.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
@@ -580,37 +581,9 @@ static void attach_cb(void *cb_ctx, const struct spdk_nvme_transport_id *trid,
 int NVMEManager::try_get(const string &sn_tag, SharedDriverData **driver)
 {
   Mutex::Locker l(lock);
-  int r = 0;
-  unsigned long long core_value;
-  uint32_t core_num = 0;
-  int m_core_arg = -1;
-  uint32_t mem_size_arg = (uint32_t)g_conf->get_val<uint64_t>("bluestore_spdk_mem");
-  char *coremask_arg = (char *)(g_conf->get_val<std::string>("bluestore_spdk_coremask")).c_str();
-
   if (sn_tag.empty()) {
-    r = -ENOENT;
-    derr << __func__ << " empty serial number: " << cpp_strerror(r) << dendl;
-    return r;
-  }
-
-  core_value = strtoll(coremask_arg, NULL, 16);
-  for (uint32_t i = 0; i < sizeof(long long) * 8; i++) {
-    bool tmp = (core_value >> i) & 0x1;
-    if (tmp) {
-      core_num += 1;
-      // select the least signficant bit as the master core
-      if(m_core_arg < 0) {
-        m_core_arg = i;
-      }
-    }
-  }
-
-  // at least one core is needed for using spdk
-  if (core_num < 1) {
-    r = -ENOENT;
-    derr << __func__ << " invalid spdk coremask, at least one core is needed: "
-         << cpp_strerror(r) << dendl;
-    return r;
+    derr << __func__ << " empty serial number" << dendl;
+    return -ENOENT;
   }
 
   for (auto &&it : shared_driver_datas) {
@@ -620,6 +593,27 @@ int NVMEManager::try_get(const string &sn_tag, SharedDriverData **driver)
     }
   }
 
+  auto coremask_arg = g_conf->get_val<std::string>("bluestore_spdk_coremask");
+  int m_core_arg = -1;
+  try {
+    auto core_value = stoull(coremask_arg, nullptr, 16);
+    m_core_arg = ffsll(core_value);
+  } catch (const std::logic_error& e) {
+    derr << __func__ << " invalid bluestore_spdk_coremask: "
+        << coremask_arg << dendl;
+    return -EINVAL;
+  }
+  // at least one core is needed for using spdk
+  if (m_core_arg == 0) {
+    derr << __func__ << " invalid bluestore_spdk_coremask, "
+        << "at least one core is needed" << dendl;
+    return -ENOENT;
+  }
+  m_core_arg -= 1;
+
+  uint32_t mem_size_arg = (uint32_t)g_conf->get_val<uint64_t>("bluestore_spdk_mem");
+
+
   if (!init) {
     init = true;
     dpdk_thread = std::thread(
@@ -629,7 +623,7 @@ int NVMEManager::try_get(const string &sn_tag, SharedDriverData **driver)
 
         spdk_env_opts_init(&opts);
         opts.name = "nvme-device-manager";
-        opts.core_mask = coremask_arg;
+        opts.core_mask = coremask_arg.c_str();
         opts.master_core = m_core_arg;
         opts.mem_size = mem_size_arg;
         spdk_env_init(&opts);