rgw: fixes following code review

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>

diff --git a/src/common/ceph_json.cc b/src/common/ceph_json.cc
index 803a97a6cd3ddbb2487553e7f04323408b9464c2..231eb0ee57efda3cf8ec51996b1f0579e7c0dc7b 100644 (file)
--- a/src/common/ceph_json.cc
+++ b/src/common/ceph_json.cc
@@ -696,13 +696,13 @@ int JSONFormattable::set(const string& name, const string& val)
 
   bool is_valid_json = jp.parse(val.c_str(), val.size());
 
-  for (auto i : tok) {
+  for (const auto& i : tok) {
     vector<field_entity> v;
     int ret = parse_entity(i, &v);
     if (ret < 0) {
       return ret;
     }
-    for (auto vi : v) {
+    for (const auto& vi : v) {
       if (f->type == FMT_NONE) {
         if (vi.is_obj) {
           f->type = FMT_OBJ;
@@ -756,13 +756,13 @@ int JSONFormattable::erase(const string& name)
   JSONFormattable *parent = nullptr;
   field_entity last_entity;
 
-  for (auto i : tok) {
+  for (auto& i : tok) {
     vector<field_entity> v;
     int ret = parse_entity(i, &v);
     if (ret < 0) {
       return ret;
     }
-    for (auto vi : v) {
+    for (const auto& vi : v) {
       if (f->type == FMT_NONE ||
           f->type == FMT_STRING) {
         if (vi.is_obj) {
@@ -787,16 +787,17 @@ int JSONFormattable::erase(const string& name)
         if (vi.is_obj) {
           return -EINVAL;
         }
-        if (vi.index < 0) {
-          vi.index = f->arr.size() + vi.index;
-          if (vi.index < 0) { /* out of bounds, nothing to remove */
+        int index = vi.index;
+        if (index < 0) {
+          index = f->arr.size() + index;
+          if (index < 0) { /* out of bounds, nothing to remove */
             return 0;
           }
         }
-        if ((size_t)vi.index >= f->arr.size()) {
+        if ((size_t)index >= f->arr.size()) {
           return 0; /* index beyond array boundaries */
         }
-        f = &f->arr[vi.index];
+        f = &f->arr[index];
       }
       last_entity = vi;
     }

diff --git a/src/common/ceph_json.h b/src/common/ceph_json.h
index cbd6e1bf2266ea968bd943238a3f4a34b4d6bf87..076020bf71e4c699b7beb1c0d831010e82321e1d 100644 (file)
--- a/src/common/ceph_json.h
+++ b/src/common/ceph_json.h
@@ -558,9 +558,11 @@ struct JSONFormattable {
     return def(string(def_val));
   }
 
+#if 0
   string operator ()(const string& def_val) const {
     return def(def_val);
   }
+#endif
 
   int operator()(int def_val) const {
     return def(def_val);

diff --git a/src/rgw/rgw_coroutine.cc b/src/rgw/rgw_coroutine.cc
index 1980f372eac124b886cbc2af601a27c817e0c519..47e2d0630b84f3c9c70787910c545759236bf382 100644 (file)
--- a/src/rgw/rgw_coroutine.cc
+++ b/src/rgw/rgw_coroutine.cc
@@ -573,13 +573,11 @@ void RGWCoroutinesManager::_schedule(RGWCoroutinesEnv *env, RGWCoroutinesStack *
 
 void RGWCoroutinesManager::set_sleeping(RGWCoroutine *cr, bool flag)
 {
-  RWLock::WLocker wl(lock);
   cr->set_sleeping(flag);
 }
 
 void RGWCoroutinesManager::io_complete(RGWCoroutine *cr, const rgw_io_id& io_id)
 {
-  RWLock::WLocker wl(lock);
   cr->io_complete(io_id);
 }
 

diff --git a/src/rgw/rgw_cr_rest.cc b/src/rgw/rgw_cr_rest.cc
index efdab6e820d697dcd86df5f8f2802094e66c2b63..2d602fc77c93981510cbb25ea4b2556520cbbe03 100644 (file)
--- a/src/rgw/rgw_cr_rest.cc
+++ b/src/rgw/rgw_cr_rest.cc
@@ -10,100 +10,70 @@
 #define dout_context g_ceph_context
 #define dout_subsys ceph_subsys_rgw
 
-class RGWCRHTTPGetDataCB : public RGWHTTPStreamRWRequest::ReceiveCB {
-  Mutex lock;
-  RGWCoroutinesEnv *env;
-  RGWCoroutine *cr;
-  RGWHTTPStreamRWRequest *req;
-  rgw_io_id io_id;
-  bufferlist data;
-  bufferlist extra_data;
-  bool got_all_extra_data{false};
-  bool paused{false};
-public:
-  RGWCRHTTPGetDataCB(RGWCoroutinesEnv *_env, RGWCoroutine *_cr, RGWHTTPStreamRWRequest *_req) : lock("RGWCRHTTPGetDataCB"), env(_env), cr(_cr), req(_req) {
-    io_id = req->get_io_id(RGWHTTPClient::HTTPCLIENT_IO_READ |RGWHTTPClient::HTTPCLIENT_IO_CONTROL);
-    req->set_in_cb(this);
-  }
+RGWCRHTTPGetDataCB::RGWCRHTTPGetDataCB(RGWCoroutinesEnv *_env, RGWCoroutine *_cr, RGWHTTPStreamRWRequest *_req) : lock("RGWCRHTTPGetDataCB"), env(_env), cr(_cr), req(_req) {
+  io_id = req->get_io_id(RGWHTTPClient::HTTPCLIENT_IO_READ |RGWHTTPClient::HTTPCLIENT_IO_CONTROL);
+  req->set_in_cb(this);
+}
 
-  int handle_data(bufferlist& bl, bool *pause) override {
-    {
-      uint64_t bl_len = bl.length();
+int RGWCRHTTPGetDataCB::handle_data(bufferlist& bl, bool *pause) {
+  {
+    uint64_t bl_len = bl.length();
 
-      Mutex::Locker l(lock);
+    Mutex::Locker l(lock);
 
-      if (!got_all_extra_data) {
-        uint64_t max = extra_data_len - extra_data.length();
-        if (max > bl_len) {
-          max = bl_len;
-        }
-        bl.splice(0, max, &extra_data);
-        bl_len -= max;
-        got_all_extra_data = extra_data.length() == extra_data_len;
+    if (!got_all_extra_data) {
+      uint64_t max = extra_data_len - extra_data.length();
+      if (max > bl_len) {
+        max = bl_len;
       }
-
-      data.append(bl);
+      bl.splice(0, max, &extra_data);
+      bl_len -= max;
+      got_all_extra_data = extra_data.length() == extra_data_len;
     }
 
-#define GET_DATA_WINDOW_SIZE 2 * 1024 * 1024
-    uint64_t data_len = data.length();
-    if (data_len >= GET_DATA_WINDOW_SIZE) {
-      env->manager->io_complete(cr, io_id);
-    }
-    if (data_len >= 2 * GET_DATA_WINDOW_SIZE) {
-      *pause = true;
-      paused = true;
-    }
-    return 0;
+    data.append(bl);
   }
 
-  void claim_data(bufferlist *dest, uint64_t max) {
-    bool need_to_unpause = false;
-
-    {
-      Mutex::Locker l(lock);
+#define GET_DATA_WINDOW_SIZE 2 * 1024 * 1024
+  uint64_t data_len = data.length();
+  if (data_len >= GET_DATA_WINDOW_SIZE) {
+    env->manager->io_complete(cr, io_id);
+  }
+  if (data_len >= 2 * GET_DATA_WINDOW_SIZE) {
+    *pause = true;
+    paused = true;
+  }
+  return 0;
+}
 
-      if (data.length() == 0) {
-        return;
-      }
+void RGWCRHTTPGetDataCB::claim_data(bufferlist *dest, uint64_t max) {
+  bool need_to_unpause = false;
 
-      if (data.length() < max) {
-        max = data.length();
-      }
+  {
+    Mutex::Locker l(lock);
 
-      data.splice(0, max, dest);
-      need_to_unpause = (paused && data.length() <= GET_DATA_WINDOW_SIZE);
+    if (data.length() == 0) {
+      return;
     }
 
-    if (need_to_unpause) {
-      req->unpause_receive();
+    if (data.length() < max) {
+      max = data.length();
     }
-  }
 
-  bufferlist& get_extra_data() {
-    return extra_data;
+    data.splice(0, max, dest);
+    need_to_unpause = (paused && data.length() <= GET_DATA_WINDOW_SIZE);
   }
 
-  bool has_data() {
-    return (data.length() > 0);
+  if (need_to_unpause) {
+    req->unpause_receive();
   }
-
-  bool has_all_extra_data() {
-    return got_all_extra_data;
-  }
-};
-
-
-RGWStreamReadHTTPResourceCRF::~RGWStreamReadHTTPResourceCRF()
-{
-  delete in_cb;
 }
 
 int RGWStreamReadHTTPResourceCRF::init()
 {
   env->stack->init_new_io(req);
 
-  in_cb = new RGWCRHTTPGetDataCB(env, caller, req);
+  in_cb.emplace(env, caller, req);
 
   int r = http_manager->add_request(req);
   if (r < 0) {

diff --git a/src/rgw/rgw_cr_rest.h b/src/rgw/rgw_cr_rest.h
index 546f1b7eeebb980b292a70cdf47b99e5e4aefe90..63477f7ad270ed3c9f831d15ce884c51600967a9 100644 (file)
--- a/src/rgw/rgw_cr_rest.h
+++ b/src/rgw/rgw_cr_rest.h
@@ -147,10 +147,6 @@ class RGWSendRawRESTResourceCR: public RGWSimpleCoroutine {
     request_cleanup();
   }
 
-  void set_input_bl(bufferlist bl){
-    input_bl = std::move(bl);
-  }
-
   int send_request() override {
     auto op = boost::intrusive_ptr<RGWRESTSendResource>(
         new RGWRESTSendResource(conn, method, path, params, nullptr, http_manager));
@@ -211,7 +207,6 @@ class RGWSendRESTResourceCR : public RGWSendRawRESTResourceCR<T> {
     jf.flush(ss);
     //bufferlist bl;
     this->input_bl.append(ss.str());
-    //set_input_bl(std::move(bl));
   }
 
 };
@@ -327,7 +322,36 @@ public:
   }
 };
 
-class RGWCRHTTPGetDataCB;
+class RGWCRHTTPGetDataCB : public RGWHTTPStreamRWRequest::ReceiveCB {
+  Mutex lock;
+  RGWCoroutinesEnv *env;
+  RGWCoroutine *cr;
+  RGWHTTPStreamRWRequest *req;
+  rgw_io_id io_id;
+  bufferlist data;
+  bufferlist extra_data;
+  bool got_all_extra_data{false};
+  bool paused{false};
+public:
+  RGWCRHTTPGetDataCB(RGWCoroutinesEnv *_env, RGWCoroutine *_cr, RGWHTTPStreamRWRequest *_req);
+
+  int handle_data(bufferlist& bl, bool *pause) override;
+
+  void claim_data(bufferlist *dest, uint64_t max);
+
+  bufferlist& get_extra_data() {
+    return extra_data;
+  }
+
+  bool has_data() {
+    return (data.length() > 0);
+  }
+
+  bool has_all_extra_data() {
+    return got_all_extra_data;
+  }
+};
+
 
 class RGWStreamReadResourceCRF {
 protected:
@@ -365,7 +389,7 @@ class RGWStreamReadHTTPResourceCRF : public RGWStreamReadResourceCRF {
 
   RGWHTTPStreamRWRequest *req{nullptr};
 
-  RGWCRHTTPGetDataCB *in_cb{nullptr};
+  std::optional<RGWCRHTTPGetDataCB> in_cb;
 
   bufferlist extra_data;
 
@@ -397,7 +421,6 @@ public:
                                                                 http_manager(_http_manager) {
     rest_obj.key = _src_key;
   }
-  ~RGWStreamReadHTTPResourceCRF() override;
 
   int init() override;
   int read(bufferlist *data, uint64_t max, bool *need_retry) override; /* reentrant */

diff --git a/src/rgw/rgw_http_client.cc b/src/rgw/rgw_http_client.cc
index a3e23e3d6e3a1e291ea30f2161855119ee5cc10c..a4ed68868cd44aa13bb95bfcfb96371efbb732a0 100644 (file)
--- a/src/rgw/rgw_http_client.cc
+++ b/src/rgw/rgw_http_client.cc
@@ -315,8 +315,6 @@ size_t RGWHTTPClient::receive_http_data(void * const ptr,
     dout(0) << "WARNING: client->receive_data() returned ret=" << ret << dendl;
   }
 
-  skip_bytes = 0;
- 
   if (pause) {
     dout(20) << "RGWHTTPClient::receive_http_data(): pause" << dendl;
     skip_bytes = len;

diff --git a/src/rgw/rgw_sync_module_aws.cc b/src/rgw/rgw_sync_module_aws.cc
index dc680eb064319c416374e94989893d940dc3687a..346504b9a48bb57c5f89534aba63369677805810 100644 (file)
--- a/src/rgw/rgw_sync_module_aws.cc
+++ b/src/rgw/rgw_sync_module_aws.cc
@@ -128,7 +128,7 @@ struct ACLMapping {
   dest_id(d) {}
 
   void init(const JSONFormattable& config) {
-    string t = config["type"];
+    const string& t = config["type"];
 
     if (t == "email") {
       type = ACL_TYPE_EMAIL_USER;
@@ -166,7 +166,7 @@ struct ACLMappings {
   map<string, ACLMapping> acl_mappings;
 
   void init(const JSONFormattable& config) {
-    for (auto c : config.array()) {
+    for (auto& c : config.array()) {
       ACLMapping m;
       m.init(c);
 
@@ -186,7 +186,7 @@ struct AWSSyncConfig_ACLProfiles {
   map<string, std::shared_ptr<ACLMappings> > acl_profiles;
 
   void init(const JSONFormattable& config) {
-    for (auto c : config.array()) {
+    for (auto& c : config.array()) {
       const string& profile_id = c["id"];
 
       std::shared_ptr<ACLMappings> ap{new ACLMappings};
@@ -514,7 +514,7 @@ struct AWSSyncConfig {
       init_profile(cct, default_conf, default_profile, false);
     }
 
-    for (auto conn : config["connections"].array()) {
+    for (auto& conn : config["connections"].array()) {
       auto new_conn = conn;
 
       std::shared_ptr<AWSSyncConfig_Connection> c{new AWSSyncConfig_Connection};

diff --git a/src/test/common/test_json_formattable.cc b/src/test/common/test_json_formattable.cc
index 6708506cc9bc6cc2113bdcd6899c9652fb0758ec..88a7af33ceb96f4944e49f62d9c84b131d490a3e 100644 (file)
--- a/src/test/common/test_json_formattable.cc
+++ b/src/test/common/test_json_formattable.cc
@@ -3,9 +3,7 @@
 /*
  * Ceph - scalable distributed file system
  *
- * Copyright (C) 2013 Cloudwatt <libre.licensing@cloudwatt.com>
- *
- * Author: Loic Dachary <loic@dachary.org>
+ * Copyright (C) 2018 Red Hat Inc.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public