From fa2faeefd8b01e951f952232033ddddeaf4f5c10 Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@redhat.com>
Date: Thu, 18 Jan 2018 12:47:51 -0600
Subject: [PATCH] common/options,librbd/Utils: refactor RBD feature validation

Move conversion of RBD features to/from string/uint64_t to helpers, and
use those from the option validator.

Keep the resulting features in integer form to avoid breaking Cinder.
Meh.

Change behavior from prior version:

- fail to parse if any of the specified features is invalid (previously
  we would parse the ones we understood and ignore the others).

Signed-off-by: Sage Weil <sage@redhat.com>
---
 src/CMakeLists.txt     |   1 +
 src/common/options.cc  |  79 ++++++-------------------------
 src/librbd/Features.cc | 105 +++++++++++++++++++++++++++++++++++++++++
 src/librbd/Features.h  |  16 +++++++
 src/librbd/Utils.cc    |   6 ++-
 5 files changed, 140 insertions(+), 67 deletions(-)
 create mode 100644 src/librbd/Features.cc
 create mode 100644 src/librbd/Features.h

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index b454f7d866043..5e1e949b7bd33 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -536,6 +536,7 @@ set(libcommon_files
   common/dns_resolve.cc
   common/hostname.cc
   common/util.cc
+  librbd/Features.cc
   arch/probe.cc
   ${auth_files}
   ${mds_files})
diff --git a/src/common/options.cc b/src/common/options.cc
index 505cf277e7fae..71d97704c2e7c 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -14,6 +14,9 @@
 // Definitions for enums
 #include "common/perf_counters.h"
 
+// rbd feature validation
+#include "librbd/Features.h"
+
 
 void Option::dump_value(const char *field_name,
     const Option::value_t &v, Formatter *f) const
@@ -5817,71 +5820,17 @@ static std::vector<Option> get_rbd_options() {
         "+4 -> exclusive-lock, +8 -> object-map, +16 -> fast-diff, "
         "+32 -> deep-flatten, +64 -> journaling, +128 -> data-pool")
     .set_safe()
-    .set_validator([](std::string *value, std::string *error_message){
-      static const std::map<std::string, uint64_t> FEATURE_MAP = {
-        {RBD_FEATURE_NAME_LAYERING, RBD_FEATURE_LAYERING},
-        {RBD_FEATURE_NAME_STRIPINGV2, RBD_FEATURE_STRIPINGV2},
-        {RBD_FEATURE_NAME_EXCLUSIVE_LOCK, RBD_FEATURE_EXCLUSIVE_LOCK},
-        {RBD_FEATURE_NAME_OBJECT_MAP, RBD_FEATURE_OBJECT_MAP},
-        {RBD_FEATURE_NAME_FAST_DIFF, RBD_FEATURE_FAST_DIFF},
-        {RBD_FEATURE_NAME_DEEP_FLATTEN, RBD_FEATURE_DEEP_FLATTEN},
-        {RBD_FEATURE_NAME_JOURNALING, RBD_FEATURE_JOURNALING},
-        {RBD_FEATURE_NAME_DATA_POOL, RBD_FEATURE_DATA_POOL},
-      };
-      static_assert((RBD_FEATURE_OPERATIONS << 1) > RBD_FEATURES_ALL,
-                    "new RBD feature added");
-
-      // convert user-friendly comma delimited feature name list to a bitmask
-      // that is used by the librbd API
-      uint64_t features = 0;
-      error_message->clear();
-
-      try {
-        features = boost::lexical_cast<decltype(features)>(*value);
-
-        uint64_t unsupported_features = (features & ~RBD_FEATURES_ALL);
-        if (unsupported_features != 0ull) {
-          features &= RBD_FEATURES_ALL;
-
-          std::stringstream ss;
-          ss << "ignoring unknown feature mask 0x"
-             << std::hex << unsupported_features;
-          *error_message = ss.str();
-        }
-        uint64_t internal_features = (features & RBD_FEATURES_INTERNAL);
-        if (internal_features != 0ULL) {
-          features &= ~RBD_FEATURES_INTERNAL;
-
-          std::stringstream ss;
-          ss << "ignoring internal feature mask 0x"
-             << std::hex << internal_features;
-          *error_message = ss.str();
-        }
-      } catch (const boost::bad_lexical_cast& ) {
-        int r = 0;
-        std::vector<std::string> feature_names;
-        boost::split(feature_names, *value, boost::is_any_of(","));
-        for (auto feature_name: feature_names) {
-          boost::trim(feature_name);
-          auto feature_it = FEATURE_MAP.find(feature_name);
-          if (feature_it != FEATURE_MAP.end()) {
-            features += feature_it->second;
-          } else {
-            if (!error_message->empty()) {
-              *error_message += ", ";
-            }
-            *error_message += "ignoring unknown feature " + feature_name;
-            r = -EINVAL;
-          }
-        }
-
-        if (features == 0 && r == -EINVAL) {
-          features = RBD_FEATURES_DEFAULT;
-        }
-      }
-      *value = stringify(features);
-      return 0;
-    }),
+    .set_validator([](std::string *value, std::string *error_message) {
+	ostringstream ss;
+	uint64_t features = librbd::rbd_features_from_string(*value, &ss);
+	// Leave this in integer form to avoid breaking Cinder.  Someday
+	// we would like to present this in string form instead...
+	*value = stringify(features);
+	if (ss.str().size()) {
+	  return -EINVAL;
+	}
+	return 0;
+      }),
 
     Option("rbd_op_threads", Option::TYPE_INT, Option::LEVEL_ADVANCED)
     .set_default(1)
diff --git a/src/librbd/Features.cc b/src/librbd/Features.cc
new file mode 100644
index 0000000000000..5c8c085839cc5
--- /dev/null
+++ b/src/librbd/Features.cc
@@ -0,0 +1,105 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#include <boost/lexical_cast.hpp>
+#include <boost/algorithm/string.hpp>
+
+#include "librbd/Features.h"
+#include "include/rbd/features.h"
+
+#include <map>
+#include <vector>
+
+static const std::map<std::string, uint64_t> RBD_FEATURE_MAP = {
+  {RBD_FEATURE_NAME_LAYERING, RBD_FEATURE_LAYERING},
+  {RBD_FEATURE_NAME_STRIPINGV2, RBD_FEATURE_STRIPINGV2},
+  {RBD_FEATURE_NAME_EXCLUSIVE_LOCK, RBD_FEATURE_EXCLUSIVE_LOCK},
+  {RBD_FEATURE_NAME_OBJECT_MAP, RBD_FEATURE_OBJECT_MAP},
+  {RBD_FEATURE_NAME_FAST_DIFF, RBD_FEATURE_FAST_DIFF},
+  {RBD_FEATURE_NAME_DEEP_FLATTEN, RBD_FEATURE_DEEP_FLATTEN},
+  {RBD_FEATURE_NAME_JOURNALING, RBD_FEATURE_JOURNALING},
+  {RBD_FEATURE_NAME_DATA_POOL, RBD_FEATURE_DATA_POOL},
+};
+static_assert((RBD_FEATURE_OPERATIONS << 1) > RBD_FEATURES_ALL,
+	      "new RBD feature added");
+
+
+namespace librbd {
+
+std::string rbd_features_to_string(uint64_t features,
+				   std::ostream *err)
+{
+  std::string r;
+  for (auto& i : RBD_FEATURE_MAP) {
+    if (features & i.second) {
+      if (r.empty()) {
+	r += ",";
+      }
+      r += i.first;
+      features &= ~i.second;
+    }
+  }
+  if (err && features) {
+    *err << "ignoring unknown feature mask 0x"
+	 << std::hex << features << std::dec;
+  }
+  return r;
+}
+
+uint64_t rbd_features_from_string(const std::string& orig_value,
+				  std::ostream *err)
+{
+  uint64_t features = 0;
+  std::string value = orig_value;
+  boost::trim(value);
+
+  // empty string means default features
+  if (!value.size()) {
+    return RBD_FEATURES_DEFAULT;
+  }
+
+  try {
+    // numeric?
+    features = boost::lexical_cast<uint64_t>(value);
+
+    // drop unrecognized bits
+    uint64_t unsupported_features = (features & ~RBD_FEATURES_ALL);
+    if (unsupported_features != 0ull) {
+      features &= RBD_FEATURES_ALL;
+      if (err) {
+	*err << "ignoring unknown feature mask 0x"
+             << std::hex << unsupported_features << std::dec;
+      }
+    }
+    uint64_t internal_features = (features & RBD_FEATURES_INTERNAL);
+    if (internal_features != 0ULL) {
+      features &= ~RBD_FEATURES_INTERNAL;
+      if (err) {
+	*err << "ignoring internal feature mask 0x"
+	     << std::hex << internal_features;
+      }
+    }
+  } catch (boost::bad_lexical_cast&) {
+    // feature name list?
+    bool errors = false;
+    std::vector<std::string> feature_names;
+    boost::split(feature_names, value, boost::is_any_of(","));
+    for (auto feature_name: feature_names) {
+      boost::trim(feature_name);
+      auto feature_it = RBD_FEATURE_MAP.find(feature_name);
+      if (feature_it != RBD_FEATURE_MAP.end()) {
+	features += feature_it->second;
+      } else if (err) {
+	if (errors) {
+	  *err << ", ";
+	} else {
+	  errors = true;
+	}
+	*err << "ignoring unknown feature " << feature_name;
+      }
+    }
+  }
+  return features;
+}
+
+} // namespace librbd
diff --git a/src/librbd/Features.h b/src/librbd/Features.h
new file mode 100644
index 0000000000000..6a88827cf26c6
--- /dev/null
+++ b/src/librbd/Features.h
@@ -0,0 +1,16 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#pragma once
+
+#include <string>
+#include <ostream>
+
+namespace librbd {
+
+  std::string rbd_features_to_string(uint64_t features,
+				     std::ostream *err);
+  uint64_t rbd_features_from_string(const std::string& value,
+				    std::ostream *err);
+
+} // librbd
diff --git a/src/librbd/Utils.cc b/src/librbd/Utils.cc
index 6f71841d7ee77..e92c79907e9da 100644
--- a/src/librbd/Utils.cc
+++ b/src/librbd/Utils.cc
@@ -10,6 +10,7 @@
 #include "include/rbd/features.h"
 #include "common/dout.h"
 #include "librbd/ImageCtx.h"
+#include "librbd/Features.h"
 
 #define dout_subsys ceph_subsys_rbd
 #undef dout_prefix
@@ -66,10 +67,11 @@ std::string generate_image_id(librados::IoCtx &ioctx) {
 
 uint64_t get_rbd_default_features(CephContext* cct)
 {
-  auto str_val = cct->_conf->get_val<std::string>("rbd_default_features");
-  return boost::lexical_cast<uint64_t>(str_val);
+  auto value = cct->_conf->get_val<std::string>("rbd_default_features");
+  return librbd::rbd_features_from_string(value, nullptr);
 }
 
+
 bool calc_sparse_extent(const bufferptr &bp,
                         size_t sparse_size,
                         uint64_t length,
-- 
2.39.5