From 01e4c1f3f96949d8ed55de9c49c138e3d8f1d808 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Tue, 10 Aug 2021 15:40:25 -0400 Subject: [PATCH] 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 (cherry picked from commit 23339590ca693c6577eb6de3b47103d60ff57a8b) --- src/cls/cmpomap/client.h | 14 +++--- src/cls/cmpomap/server.cc | 23 +++++----- src/test/cls_cmpomap/test_cls_cmpomap.cc | 54 +++++++++++++++++++++++- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/src/cls/cmpomap/client.h b/src/cls/cmpomap/client.h index c55fbc4105c3..013d85cc7ee2 100644 --- 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 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 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 691832bfe636..86e16d940f3e 100644 --- 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 c10939b96266..c8abb5266ca2 100644 --- 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 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 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 -- 2.47.3