From cafe8ec6af8f230a49a177fe5ccea808e0856a70 Mon Sep 17 00:00:00 2001 From: Pritha Srivastava Date: Tue, 12 Dec 2023 12:44:24 +0530 Subject: [PATCH] rgw/cache: changing `get_attr` api such that it returns int signifying success or error, and `attr_val` is added as an out param. Signed-off-by: Pritha Srivastava --- src/rgw/rgw_cache_driver.h | 2 +- src/rgw/rgw_redis_driver.cc | 26 +++++++++++++++++--------- src/rgw/rgw_redis_driver.h | 2 +- src/rgw/rgw_ssd_driver.cc | 28 +++++++++++++++------------- src/rgw/rgw_ssd_driver.h | 2 +- src/test/rgw/test_redis_driver.cc | 5 +++-- src/test/rgw/test_ssd_driver.cc | 12 +++++++++--- 7 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/rgw/rgw_cache_driver.h b/src/rgw/rgw_cache_driver.h index 732e2cd5ce017..4495b1d537825 100644 --- a/src/rgw/rgw_cache_driver.h +++ b/src/rgw/rgw_cache_driver.h @@ -29,7 +29,7 @@ class CacheDriver { virtual int set_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& attrs, optional_yield y) = 0; virtual int update_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& attrs, optional_yield y) = 0; virtual int delete_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& del_attrs, optional_yield y) = 0; - virtual std::string get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, optional_yield y) = 0; + virtual int get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, std::string& attr_val, optional_yield y) = 0; virtual int set_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, const std::string& attr_val, optional_yield y) = 0; /* Partition */ diff --git a/src/rgw/rgw_redis_driver.cc b/src/rgw/rgw_redis_driver.cc index fb2d6bcd9873c..a7b2823276fd0 100644 --- a/src/rgw/rgw_redis_driver.cc +++ b/src/rgw/rgw_redis_driver.cc @@ -423,7 +423,7 @@ int RedisDriver::delete_attrs(const DoutPrefixProvider* dpp, const std::string& } } -std::string RedisDriver::get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, optional_yield y) +int RedisDriver::get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, std::string& attr_val, optional_yield y) { std::string entry = partition_info.location + key; response value; @@ -437,15 +437,19 @@ std::string RedisDriver::get_attr(const DoutPrefixProvider* dpp, const std::stri redis_exec(conn, ec, req, resp, y); - if (ec) - return {}; + if (ec) { + attr_val = ""; + return -1; + } } catch(std::exception &e) { - return {}; + attr_val = ""; + return -1; } if (!std::get<0>(resp).value()) { ldpp_dout(dpp, 10) << "RedisDriver::" << __func__ << "(): Attribute was not found." << dendl; - return {}; + attr_val = ""; + return -1; } /* Retrieve existing value from cache */ @@ -456,13 +460,17 @@ std::string RedisDriver::get_attr(const DoutPrefixProvider* dpp, const std::stri redis_exec(conn, ec, req, value, y); - if (ec) - return {}; + if (ec) { + attr_val = ""; + return -1; + } } catch(std::exception &e) { - return {}; + attr_val = ""; + return -1; } - return std::get<0>(value).value(); + attr_val = std::get<0>(value).value(); + return 0; } int RedisDriver::set_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, const std::string& attr_val, optional_yield y) diff --git a/src/rgw/rgw_redis_driver.h b/src/rgw/rgw_redis_driver.h index b0b69522bdd2a..cf9714b1cd676 100644 --- a/src/rgw/rgw_redis_driver.h +++ b/src/rgw/rgw_redis_driver.h @@ -45,7 +45,7 @@ class RedisDriver : public CacheDriver { virtual int update_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& attrs, optional_yield y) override; virtual int delete_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& del_attrs, optional_yield y) override; virtual int set_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, const std::string& attr_val, optional_yield y) override; - virtual std::string get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, optional_yield y) override; + virtual int get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, std::string& attr_val, optional_yield y) override; void shutdown(); struct redis_response { diff --git a/src/rgw/rgw_ssd_driver.cc b/src/rgw/rgw_ssd_driver.cc index 8210f1c752953..3475563004245 100644 --- a/src/rgw/rgw_ssd_driver.cc +++ b/src/rgw/rgw_ssd_driver.cc @@ -358,8 +358,8 @@ int SSDDriver::update_attrs(const DoutPrefixProvider* dpp, const std::string& ke for (auto& it : attrs) { std::string attr_name = it.first; std::string attr_val = it.second.to_str(); - std::string old_attr_val = get_attr(dpp, key, attr_name, y); - int ret; + std::string old_attr_val; + auto ret = get_attr(dpp, key, attr_name, old_attr_val, y); if (old_attr_val.empty()) { ret = setxattr(location.c_str(), attr_name.c_str(), attr_val.c_str(), attr_val.size(), XATTR_CREATE); } else { @@ -419,7 +419,8 @@ int SSDDriver::get_attrs(const DoutPrefixProvider* dpp, const std::string& key, if (prefixloc == std::string::npos) { continue; } - std::string attr_value = get_attr(dpp, key, attr_name, y); + std::string attr_value; + get_attr(dpp, key, attr_name, attr_value, y); bufferlist bl_value; bl_value.append(attr_value); attrs.emplace(std::move(attr_name), std::move(bl_value)); @@ -449,7 +450,7 @@ int SSDDriver::set_attrs(const DoutPrefixProvider* dpp, const std::string& key, return 0; } -std::string SSDDriver::get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, optional_yield y) +int SSDDriver::get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, std::string& attr_val, optional_yield y) { std::string location = partition_info.location + key; ldpp_dout(dpp, 20) << "SSDCache: " << __func__ << "(): location=" << location << dendl; @@ -458,24 +459,26 @@ std::string SSDDriver::get_attr(const DoutPrefixProvider* dpp, const std::string int attr_size = getxattr(location.c_str(), attr_name.c_str(), nullptr, 0); if (attr_size < 0) { - auto ret = errno; - ldpp_dout(dpp, 0) << "ERROR: could not get attribute " << attr_name << ": " << cpp_strerror(ret) << dendl; - return ""; + ldpp_dout(dpp, 0) << "ERROR: could not get attribute " << attr_name << ": " << cpp_strerror(errno) << dendl; + attr_val = ""; + return errno; } if (attr_size == 0) { ldpp_dout(dpp, 0) << "ERROR: no attribute value found for attr_name: " << attr_name << dendl; - return ""; + attr_val = ""; + return 0; } - char attr_val_ptr[attr_size + 1]; - attr_size = getxattr(location.c_str(), attr_name.c_str(), attr_val_ptr, attr_size); + attr_val.resize(attr_size); + attr_size = getxattr(location.c_str(), attr_name.c_str(), attr_val.data(), attr_size); if (attr_size < 0) { ldpp_dout(dpp, 0) << "SSDCache: " << __func__ << "(): could not get attr value for attr name: " << attr_name << " key: " << key << dendl; + attr_val = ""; + return errno; } - attr_val_ptr[attr_size] = '\0'; - return std::string(attr_val_ptr); + return 0; } int SSDDriver::set_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, const std::string& attr_val, optional_yield y) @@ -502,7 +505,6 @@ int SSDDriver::delete_attr(const DoutPrefixProvider* dpp, const std::string& key std::string location = partition_info.location + key; ldpp_dout(dpp, 20) << "SSDCache: " << __func__ << "(): location=" << location << dendl; - auto attr_val = get_attr(dpp, key, attr_name, null_yield); //need this for free space calculation. auto ret = removexattr(location.c_str(), attr_name.c_str()); if (ret < 0) { ldpp_dout(dpp, 0) << "SSDCache: " << __func__ << "(): could not remove attr value for attr name: " << attr_name << " key: " << key << cpp_strerror(errno) << dendl; diff --git a/src/rgw/rgw_ssd_driver.h b/src/rgw/rgw_ssd_driver.h index 4084a0aaa456b..11dfdb077e34b 100644 --- a/src/rgw/rgw_ssd_driver.h +++ b/src/rgw/rgw_ssd_driver.h @@ -23,7 +23,7 @@ public: virtual int set_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& attrs, optional_yield y) override; virtual int update_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& attrs, optional_yield y) override; virtual int delete_attrs(const DoutPrefixProvider* dpp, const std::string& key, rgw::sal::Attrs& del_attrs, optional_yield y) override; - virtual std::string get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, optional_yield y) override; + virtual int get_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, std::string& attr_val, optional_yield y) override; virtual int set_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name, const std::string& attr_val, optional_yield y) override; int delete_attr(const DoutPrefixProvider* dpp, const std::string& key, const std::string& attr_name); diff --git a/src/test/rgw/test_redis_driver.cc b/src/test/rgw/test_redis_driver.cc index a57d1a2f3e923..e06ae124cdfec 100644 --- a/src/test/rgw/test_redis_driver.cc +++ b/src/test/rgw/test_redis_driver.cc @@ -464,8 +464,9 @@ TEST_F(RedisDriverFixture, GetAttrYield) ASSERT_EQ((bool)ec, false); EXPECT_EQ(std::get<0>(resp).value(), 0); } - - ASSERT_EQ("newVal", cacheDriver->get_attr(env->dpp, "testName", "attr", optional_yield{io, yield})); + std::string attr_val; + ASSERT_EQ(0, cacheDriver->get_attr(env->dpp, "testName", "attr", attr_val, optional_yield{io, yield})); + ASSERT_EQ("newVal", attr_val); cacheDriver->shutdown(); { diff --git a/src/test/rgw/test_ssd_driver.cc b/src/test/rgw/test_ssd_driver.cc index 6396dcf51efdc..04019f0f2b6b1 100644 --- a/src/test/rgw/test_ssd_driver.cc +++ b/src/test/rgw/test_ssd_driver.cc @@ -149,7 +149,9 @@ TEST_F(SSDDriverFixture, SetGetAttr) std::string attr_name = "user.ssd.testattr"; std::string attr_val = "testattrVal"; ASSERT_EQ(0, cacheDriver->set_attr(env->dpp, "testSetGetAttr", attr_name, attr_val, optional_yield{io, yield})); - ASSERT_EQ(attr_val, cacheDriver->get_attr(env->dpp, "testSetGetAttr", attr_name, optional_yield{io, yield})); + std::string attr_val_ret; + ASSERT_EQ(0, cacheDriver->get_attr(env->dpp, "testSetGetAttr", attr_name, attr_val_ret, optional_yield{io, yield})); + ASSERT_EQ(attr_val, attr_val_ret); }); io.run(); @@ -163,10 +165,14 @@ TEST_F(SSDDriverFixture, DeleteAttr) std::string attr_name = "user.ssd.testattr"; std::string attr_val = "testattrVal"; ASSERT_EQ(0, cacheDriver->set_attr(env->dpp, "testDeleteAttr", attr_name, attr_val, optional_yield{io, yield})); - ASSERT_EQ(attr_val, cacheDriver->get_attr(env->dpp, "testDeleteAttr", attr_name, optional_yield{io, yield})); + std::string attr_val_ret; + ASSERT_EQ(0, cacheDriver->get_attr(env->dpp, "testDeleteAttr", attr_name, attr_val_ret, optional_yield{io, yield})); + ASSERT_EQ(attr_val, attr_val_ret); + attr_val_ret.clear(); ASSERT_EQ(0, cacheDriver->delete_attr(env->dpp, "testDeleteAttr", attr_name)); - ASSERT_EQ("", cacheDriver->get_attr(env->dpp, "testDeleteAttr", attr_name, optional_yield{io, yield})); + ASSERT_EQ(ENODATA, cacheDriver->get_attr(env->dpp, "testDeleteAttr", attr_name, attr_val_ret, optional_yield{io, yield})); + ASSERT_EQ("", attr_val_ret); }); io.run(); -- 2.39.5