common/options: convert a millisecs opt to a chrono::milliseconds when paring it

Option always parses a new string value and convert it to a value_t
before validating it. and value_t is an alias of boost::variant<...>.
and we use "new_value < min" to tell if the new_value is out of the
bound or not, where both "new_value" and "min" are instances of
value_t. so it is critcal that these two values contain the same type
of value, otherwise boost::variant::operator< would

> Returns:
> If which() == rhs.which() then: content_this < content_rhs, where content_this is the content of *this and content_rhs is the content of rhs. Otherwise: which() < rhs.which().

where which() indicates which type of value is contained in the value_t.

before this change, instead of converting a new value of milliseconds to
std::chrono::milliseconds, we convert it to an uint64_t, whose index in
the value_t is 2, while the milliseconds value's index is 9, so
"new_value < min" evaluates to true even if "new_value" is 100 and "min"
is 30.

after this change, the new value of a milliseconds option is converted
to std::chrono::milliseconds, so it is comparable with its min value and
max value.

a minimal test is added to reproduce this issue.

the change which added the support of millisec to option was
29690a338ba4482d187e6036903e138437ae3bb4 which is not included by any
LTS branches, so no need to backport this fix.

Fixes: https://tracker.ceph.com/issues/51375
Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/common/options.cc b/src/common/options.cc
index 5f89980d85a4b9984df83193b447deaa3f625e75..8ad93c01a0733672017aa30da850a7daa23fa251 100644 (file)
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -210,7 +210,7 @@ int Option::parse_value(
     }
   } else if (type == Option::TYPE_MILLISECS) {
     try {
-      *out = boost::lexical_cast<uint64_t>(val);
+      *out = std::chrono::milliseconds(boost::lexical_cast<uint64_t>(val));
     } catch (const boost::bad_lexical_cast& e) {
       *error_message = e.what();
       return -EINVAL;

diff --git a/src/test/common/CMakeLists.txt b/src/test/common/CMakeLists.txt
index 679acd24c82f1f58606ef960dca02f03c5b51abb..2107e3275e9e2c3d86a514c9704d3aa11f5d5d6d 100644 (file)
--- a/src/test/common/CMakeLists.txt
+++ b/src/test/common/CMakeLists.txt
@@ -353,6 +353,9 @@ add_ceph_unittest(unittest_cdc)
 add_executable(unittest_ceph_timer test_ceph_timer.cc)
 add_ceph_unittest(unittest_ceph_timer)
 
+add_executable(unittest_option test_option.cc)
+target_link_libraries(unittest_option ceph-common GTest::Main)
+add_ceph_unittest(unittest_option)
 
 add_executable(unittest_blocked_completion test_blocked_completion.cc)
 add_ceph_unittest(unittest_blocked_completion)

diff --git a/src/test/common/test_option.cc b/src/test/common/test_option.cc
new file mode 100644 (file)
index 0000000..dd6bab6
--- /dev/null
+++ b/src/test/common/test_option.cc
@@ -0,0 +1,71 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*-
+// vim: ts=8 sw=2 smarttab expandtab
+
+#include <string.h>
+#include <errno.h>
+#include <stdlib.h>
+
+#include <gtest/gtest.h>
+
+#include "common/options.h"
+
+TEST(Option, validate_min_max)
+{
+  auto opt = Option{"foo", Option::TYPE_MILLISECS, Option::LEVEL_ADVANCED}
+    .set_default(42)
+    .set_min_max(10, 128);
+  struct test_t {
+    unsigned new_val;
+    int expected_retval;
+  };
+  test_t tests[] =
+    {{9, -EINVAL},
+     {10, 0},
+     {11, 0},
+     {128, 0},
+     {1024, -EINVAL}
+  };
+  for (auto& test : tests) {
+    Option::value_t new_value = std::chrono::milliseconds{test.new_val};
+    std::string err;
+    GTEST_ASSERT_EQ(test.expected_retval, opt.validate(new_value, &err));
+  }
+}
+
+TEST(Option, parse)
+{
+  auto opt = Option{"foo", Option::TYPE_MILLISECS, Option::LEVEL_ADVANCED}
+    .set_default(42)
+    .set_min_max(10, 128);
+  struct test_t {
+    string new_val;
+    int expected_retval;
+    unsigned expected_parsed_val;
+  };
+  test_t tests[] =
+    {{"9", -EINVAL, 0},
+     {"10", 0, 10},
+     {"11", 0, 11},
+     {"128", 0, 128},
+     {"1024", -EINVAL, 0}
+  };
+  for (auto& test : tests) {
+    Option::value_t parsed_val;
+    std::string err;
+    GTEST_ASSERT_EQ(test.expected_retval,
+                    opt.parse_value(test.new_val, &parsed_val, &err));
+    if (test.expected_retval == 0) {
+      Option::value_t expected_parsed_val =
+        std::chrono::milliseconds{test.expected_parsed_val};
+      GTEST_ASSERT_EQ(parsed_val, expected_parsed_val);
+    }
+  }
+}
+
+/*
+ * Local Variables:
+ * compile-command: "cd ../../../build ;
+ *   ninja unittest_option && bin/unittest_option
+ *   "
+ * End:
+ */