common: move validation in Option and add a test

Signed-off-by: John Spray <john.spray@redhat.com>

src/common/config.cc

@@ -996,12 +996,9 @@ int md_config_t::set_val_impl(const std::string &raw_val, const Option &opt,
 
   std::string val = raw_val;
 
-  // Special validator callback?
-  if (opt.validator) {
-    int r = opt.validator(&val, error_message);
-    if (r < 0) {
-      return r;
-    }
+  int r = opt.pre_validate(&val, error_message);
+  if (r != 0) {
+    return r;
   }
 
   Option::value_t new_value;
@@ -1054,40 +1051,11 @@ int md_config_t::set_val_impl(const std::string &raw_val, const Option &opt,
     ceph_abort();
   }
 
-  // Generic validation: min
-  if (!boost::get<boost::blank>(&(opt.min))) {
-    if (new_value < opt.min) {
-      std::ostringstream oss;
-      oss << "Value '" << new_value << "' is below minimum " << opt.min;
-      *error_message = oss.str();
-      return -EINVAL;
-    }
+  r = opt.validate(new_value, error_message);
+  if (r != 0) {
+    return r;
   }
 
-  // Generic validation: max
-  if (!boost::get<boost::blank>(&(opt.max))) {
-    if (new_value > opt.max) {
-      std::ostringstream oss;
-      oss << "Value '" << new_value << "' exceeds maximum " << opt.max;
-      *error_message = oss.str();
-      return -EINVAL;
-    }
-  }
-
-  // Generic validation: enum
-  if (!opt.enum_allowed.empty() && opt.type == Option::TYPE_STR) {
-    auto found = std::find(opt.enum_allowed.begin(), opt.enum_allowed.end(),
-                           boost::get<std::string>(new_value));
-    if (found == opt.enum_allowed.end()) {
-      std::ostringstream oss;
-      oss << "'" << new_value << "' is not one of the permitted "
-                 "values: " << joinify(opt.enum_allowed.begin(),
-                                       opt.enum_allowed.end(),
-                                       std::string(", "));
-      *error_message = oss.str();
-      return -EINVAL;
-    }
-  }
 
   // Apply the value to its entry in the `values` map
   values[opt.name] = new_value;

src/common/options.cc

@@ -33,6 +33,55 @@ void Option::dump_value(const char *field_name,
   }
 }
 
+int Option::pre_validate(std::string *new_value, std::string *err) const
+{
+  if (validator) {
+    return validator(new_value, err);
+  } else {
+    return 0;
+  }
+}
+
+int Option::validate(const Option::value_t &new_value, std::string *err) const
+{
+  // Generic validation: min
+  if (!boost::get<boost::blank>(&(min))) {
+    if (new_value < min) {
+      std::ostringstream oss;
+      oss << "Value '" << new_value << "' is below minimum " << min;
+      *err = oss.str();
+      return -EINVAL;
+    }
+  }
+
+  // Generic validation: max
+  if (!boost::get<boost::blank>(&(max))) {
+    if (new_value > max) {
+      std::ostringstream oss;
+      oss << "Value '" << new_value << "' exceeds maximum " << max;
+      *err = oss.str();
+      return -EINVAL;
+    }
+  }
+
+  // Generic validation: enum
+  if (!enum_allowed.empty() && type == Option::TYPE_STR) {
+    auto found = std::find(enum_allowed.begin(), enum_allowed.end(),
+                           boost::get<std::string>(new_value));
+    if (found == enum_allowed.end()) {
+      std::ostringstream oss;
+      oss << "'" << new_value << "' is not one of the permitted "
+                 "values: " << joinify(enum_allowed.begin(),
+                                       enum_allowed.end(),
+                                       std::string(", "));
+      *err = oss.str();
+      return -EINVAL;
+    }
+  }
+
+  return 0;
+}
+
 void Option::dump(Formatter *f) const
 {
   f->open_object_section("option");

src/common/options.h

@@ -132,6 +132,12 @@ struct Option {
 
   void dump_value(const char *field_name, const value_t &v, Formatter *f) const;
 
+  // Validate and potentially modify incoming string value
+  int pre_validate(std::string *new_value, std::string *err) const;
+
+  // Validate properly typed value against bounds
+  int validate(const Option::value_t &new_value, std::string *err) const;
+
   // const char * must be explicit to avoid it being treated as an int
   Option& set_value(value_t& v, const char *new_value) {
     v = std::string(new_value);
@@ -219,6 +225,12 @@ struct Option {
     return *this;
   }
 
+  Option& set_enum_allowed(const std::list<std::string> allowed)
+  {
+    enum_allowed = allowed;
+    return *this;
+  }
+
   Option &set_safe() {
     safe = true;
     return *this;

src/test/common/test_config.cc

@@ -190,6 +190,47 @@ TEST(md_config_t, set_val)
   }
 }
 
+TEST(Option, validation)
+{
+  Option opt_int("foo", Option::TYPE_INT, Option::LEVEL_BASIC);
+  opt_int.set_min_max(5, 10);
+
+  std::string msg;
+  EXPECT_EQ(-EINVAL, opt_int.validate(Option::value_t(int64_t(4)), &msg));
+  EXPECT_EQ(-EINVAL, opt_int.validate(Option::value_t(int64_t(11)), &msg));
+  EXPECT_EQ(0, opt_int.validate(Option::value_t(int64_t(7)), &msg));
+
+  Option opt_enum("foo", Option::TYPE_STR, Option::LEVEL_BASIC);
+  opt_enum.set_enum_allowed({"red", "blue"});
+  EXPECT_EQ(0, opt_enum.validate(Option::value_t(std::string("red")), &msg));
+  EXPECT_EQ(0, opt_enum.validate(Option::value_t(std::string("blue")), &msg));
+  EXPECT_EQ(-EINVAL, opt_enum.validate(Option::value_t(std::string("green")), &msg));
+
+  Option opt_validator("foo", Option::TYPE_INT, Option::LEVEL_BASIC);
+  opt_validator.set_validator([](std::string *value, std::string *error_message){
+      if (*value == std::string("one")) {
+        *value = "1";
+        return 0;
+      } else if (*value == std::string("666")) {
+        return -EINVAL;
+      } else {
+        return 0;
+      }
+  });
+
+  std::string input = "666";  // An explicitly forbidden value
+  EXPECT_EQ(-EINVAL, opt_validator.pre_validate(&input, &msg));
+  EXPECT_EQ(input, "666");
+
+  input = "123";  // A permitted value with no special behaviour
+  EXPECT_EQ(0, opt_validator.pre_validate(&input, &msg));
+  EXPECT_EQ(input, "123");
+
+  input = "one";  // A value that has a magic conversion
+  EXPECT_EQ(0, opt_validator.pre_validate(&input, &msg));
+  EXPECT_EQ(input, "1");
+}
+
 /*
  * Local Variables:
  * compile-command: "cd ../.. ;