cls/cmpomap: empty values are 0 in U64 comparisons

previously, when trying to use cmpomap interfaces on an omap key with
an empty value, U64 comparisons would fail to decode with -EIO. so
cmp_set_vals() and cmp_rm_keys() are unable to update or remove such
keys

for backward-compatibility with rgw's data sync error repo, where the
keys used to have empty values, enable these comparisons by treating an
empty value as 0

Fixes: https://tracker.ceph.com/issues/52128
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 23339590ca693c6577eb6de3b47103d60ff57a8b)

diff --git a/src/cls/cmpomap/client.h b/src/cls/cmpomap/client.h
index c55fbc4105c31d40e4de2fd732b50a4a447429f9..013d85cc7ee203f46d8627d1d3abd3f5a7d4c444 100644 (file)
--- a/src/cls/cmpomap/client.h
+++ b/src/cls/cmpomap/client.h
@@ -26,7 +26,8 @@ static constexpr uint32_t max_keys = 1000;
 /// process each of the omap value comparisons according to the same rules as
 /// cmpxattr(), and return -ECANCELED if a comparison is unsuccessful. for
 /// comparisons with Mode::U64, failure to decode an input value is reported
-/// as -EINVAL, while failure to decode a stored value is reported as -EIO
+/// as -EINVAL, an empty stored value is compared as 0, and failure to decode
+/// a stored value is reported as -EIO
 [[nodiscard]] int cmp_vals(librados::ObjectReadOperation& op,
                            Mode mode, Op comparison, ComparisonMap values,
                            std::optional<ceph::bufferlist> default_value);
@@ -34,9 +35,9 @@ static constexpr uint32_t max_keys = 1000;
 /// process each of the omap value comparisons according to the same rules as
 /// cmpxattr(). any key/value pairs that compare successfully are overwritten
 /// with the corresponding input value. for comparisons with Mode::U64, failure
-/// to decode an input value is reported as -EINVAL. decode failure of a stored
-/// value is treated as an unsuccessful comparison and is not reported as an
-/// error
+/// to decode an input value is reported as -EINVAL. an empty stored value is
+/// compared as 0, while decode failure of a stored value is treated as an
+/// unsuccessful comparison and is not reported as an error
 [[nodiscard]] int cmp_set_vals(librados::ObjectWriteOperation& writeop,
                                Mode mode, Op comparison, ComparisonMap values,
                                std::optional<ceph::bufferlist> default_value);
@@ -44,8 +45,9 @@ static constexpr uint32_t max_keys = 1000;
 /// process each of the omap value comparisons according to the same rules as
 /// cmpxattr(). any key/value pairs that compare successfully are removed. for
 /// comparisons with Mode::U64, failure to decode an input value is reported as
-/// -EINVAL. decode failure of a stored value is treated as an unsuccessful
-/// comparison and is not reported as an error
+/// -EINVAL. an empty stored value is compared as 0, while decode failure of a
+/// stored value is treated as an unsuccessful comparison and is not reported
+/// as an error
 [[nodiscard]] int cmp_rm_keys(librados::ObjectWriteOperation& writeop,
                               Mode mode, Op comparison, ComparisonMap values);
 

diff --git a/src/cls/cmpomap/server.cc b/src/cls/cmpomap/server.cc
index 691832bfe63663f4e244ac6556d4a51ff35e50de..86e16d940f3efc2bfc484b8606fbdb84e2f87c2f 100644 (file)
--- a/src/cls/cmpomap/server.cc
+++ b/src/cls/cmpomap/server.cc
@@ -37,17 +37,20 @@ static int compare_values(Op op, const T& lhs, const T& rhs)
 
 static int compare_values_u64(Op op, uint64_t lhs, const bufferlist& value)
 {
-  try {
-    // decode existing value as rhs
-    uint64_t rhs;
-    auto p = value.cbegin();
-    using ceph::decode;
-    decode(rhs, p);
-    return compare_values(op, lhs, rhs);
-  } catch (const buffer::error&) {
-    // failures to decode existing values are reported as EIO
-    return -EIO;
+  // empty values compare as 0 for backward compat
+  uint64_t rhs = 0;
+  if (value.length()) {
+    try {
+      // decode existing value as rhs
+      auto p = value.cbegin();
+      using ceph::decode;
+      decode(rhs, p);
+    } catch (const buffer::error&) {
+      // failures to decode existing values are reported as EIO
+      return -EIO;
+    }
   }
+  return compare_values(op, lhs, rhs);
 }
 
 static int compare_value(Mode mode, Op op, const bufferlist& input,

diff --git a/src/test/cls_cmpomap/test_cls_cmpomap.cc b/src/test/cls_cmpomap/test_cls_cmpomap.cc
index c10939b96266fc1b0c119c613e7b7b8dab9a6b1f..c8abb5266ca2731290c2302c5190c3c4d4ace9d3 100644 (file)
--- a/src/test/cls_cmpomap/test_cls_cmpomap.cc
+++ b/src/test/cls_cmpomap/test_cls_cmpomap.cc
@@ -270,7 +270,7 @@ TEST_F(CmpOmap, cmp_vals_u64_invalid_default)
   ASSERT_EQ(ioctx.create(oid, true), 0);
   const std::string key = "key";
   const bufferlist input = u64_buffer(0);
-  const bufferlist def; // empty buffer can't be decoded as u64
+  const bufferlist def = string_buffer("bbb"); // can't be decoded as u64
   EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::EQ, {{key, input}}, def), -EIO);
   EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::NE, {{key, input}}, def), -EIO);
   EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::GT, {{key, input}}, def), -EIO);
@@ -279,6 +279,21 @@ TEST_F(CmpOmap, cmp_vals_u64_invalid_default)
   EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::LTE, {{key, input}}, def), -EIO);
 }
 
+TEST_F(CmpOmap, cmp_vals_u64_empty_default)
+{
+  const std::string oid = __PRETTY_FUNCTION__;
+  ASSERT_EQ(ioctx.create(oid, true), 0);
+  const std::string key = "key";
+  const bufferlist input = u64_buffer(1);
+  const bufferlist def; // empty buffer defaults to 0
+  EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::EQ, {{key, input}}, def), -ECANCELED);
+  EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::NE, {{key, input}}, def), 0);
+  EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::GT, {{key, input}}, def), 0);
+  EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::GTE, {{key, input}}, def), 0);
+  EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::LT, {{key, input}}, def), -ECANCELED);
+  EXPECT_EQ(do_cmp_vals(oid, Mode::U64, Op::LTE, {{key, input}}, def), -ECANCELED);
+}
+
 TEST_F(CmpOmap, cmp_vals_u64_invalid_value)
 {
   const std::string oid = __PRETTY_FUNCTION__;
@@ -665,4 +680,41 @@ TEST_F(CmpOmap, cmp_rm_keys_over_max_keys)
   EXPECT_EQ(cmp_rm_keys(op, Mode::U64, Op::EQ, std::move(comparisons)), -E2BIG);
 }
 
+// test upgrades from empty omap values to u64
+TEST_F(CmpOmap, cmp_rm_keys_u64_empty)
+{
+  const std::string oid = __PRETTY_FUNCTION__;
+  const bufferlist value1; // empty buffer
+  const bufferlist value2 = u64_buffer(42);
+  {
+    std::map<std::string, bufferlist> vals = {
+      {"eq", value1},
+      {"ne", value1},
+      {"gt", value1},
+      {"gte", value1},
+      {"lt", value1},
+      {"lte", value1},
+    };
+    ASSERT_EQ(ioctx.omap_set(oid, vals), 0);
+  }
+
+  ASSERT_EQ(do_cmp_rm_keys(oid, Mode::U64, Op::EQ, {{"eq", value2}}), 0);
+  ASSERT_EQ(do_cmp_rm_keys(oid, Mode::U64, Op::NE, {{"ne", value2}}), 0);
+  ASSERT_EQ(do_cmp_rm_keys(oid, Mode::U64, Op::GT, {{"gt", value2}}), 0);
+  ASSERT_EQ(do_cmp_rm_keys(oid, Mode::U64, Op::GTE, {{"gte", value2}}), 0);
+  ASSERT_EQ(do_cmp_rm_keys(oid, Mode::U64, Op::LT, {{"lt", value2}}), 0);
+  ASSERT_EQ(do_cmp_rm_keys(oid, Mode::U64, Op::LTE, {{"lte", value2}}), 0);
+
+  {
+    std::map<std::string, bufferlist> vals;
+    ASSERT_EQ(get_vals(oid, &vals), 0);
+    EXPECT_EQ(vals.count("eq"), 1);
+    EXPECT_EQ(vals.count("ne"), 0);
+    EXPECT_EQ(vals.count("gt"), 0);
+    EXPECT_EQ(vals.count("gte"), 0);
+    EXPECT_EQ(vals.count("lt"), 1);
+    EXPECT_EQ(vals.count("lte"), 1);
+  }
+}
+
 } // namespace cls::cmpomap