From b2a2b959ba32339f599eb4924a91f63ba542e4d8 Mon Sep 17 00:00:00 2001 From: Yehuda Sadeh Date: Fri, 23 Mar 2018 16:38:51 -0700 Subject: [PATCH] rgw: fixes following code review Signed-off-by: Yehuda Sadeh --- src/common/ceph_json.cc | 19 ++-- src/common/ceph_json.h | 2 + src/rgw/rgw_coroutine.cc | 2 - src/rgw/rgw_cr_rest.cc | 110 +++++++++-------------- src/rgw/rgw_cr_rest.h | 39 ++++++-- src/rgw/rgw_http_client.cc | 2 - src/rgw/rgw_sync_module_aws.cc | 8 +- src/test/common/test_json_formattable.cc | 4 +- 8 files changed, 88 insertions(+), 98 deletions(-) diff --git a/src/common/ceph_json.cc b/src/common/ceph_json.cc index 803a97a6cd3dd..231eb0ee57efd 100644 --- 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 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 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 cbd6e1bf2266e..076020bf71e4c 100644 --- 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 1980f372eac12..47e2d0630b84f 100644 --- 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 efdab6e820d69..2d602fc77c939 100644 --- 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 546f1b7eeebb9..63477f7ad270e 100644 --- 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( new RGWRESTSendResource(conn, method, path, params, nullptr, http_manager)); @@ -211,7 +207,6 @@ class RGWSendRESTResourceCR : public RGWSendRawRESTResourceCR { 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 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 a3e23e3d6e3a1..a4ed68868cd44 100644 --- 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 dc680eb064319..346504b9a48bb 100644 --- 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 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 > 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 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 c{new AWSSyncConfig_Connection}; diff --git a/src/test/common/test_json_formattable.cc b/src/test/common/test_json_formattable.cc index 6708506cc9bc6..88a7af33ceb96 100644 --- 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 - * - * Author: Loic Dachary + * 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 -- 2.39.5