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 <[email protected]>
salieri11 · Apr 2, 2024 · cafe8ec · cafe8ec
1 parent 2a45af5
commit cafe8ec
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 30 deletions.
diff --git 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
@@ -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<std::string> 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
@@ -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
@@ -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
@@ -23,7 +23,7 @@ class SSDDriver : public CacheDriver {
   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
@@ -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
@@ -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();