rgw: make RGWEnv return a const ref. to its map #15269

theanalyst · 2017-05-24T15:40:50Z

We already have a public method set for setting values in the RGW env
map, making the map as a read only. In addition:

Added const to other methods in RGWEnv which are getters
req_info::init_meta also reused the same iter name for an internal
find, changed the var name
add_grants_headers now accepts an RGWEnv instead of the map
use range based for loops wherever the code was changed\
req_info holds a ptr to a const RGWEnv, since we seem to only read the
values from it

Signed-off-by: Abhishek Lekshmanan abhishek@suse.com

rzarzynski

Thanks for bringing this up, @theanalyst! Please see my comments below.

rzarzynski · 2017-05-30T17:23:51Z

src/rgw/rgw_common.cc

@@ -201,7 +201,7 @@ static string get_abs_path(const string& request_uri) {
  return request_uri.substr(beg_pos, len - beg_pos);
 }

-req_info::req_info(CephContext *cct, class RGWEnv *e) : env(e) {
+req_info::req_info(CephContext *cct, class RGWEnv *_env) : env(_env) {


The underscore in _env may be omitted:

req_info::req_info(CephContext *cct, class RGWEnv *env) : env(env) {

A bit more const-correct variant would look like:

req_info::req_info(CephContext* const cct, class RGWEnv* const env) : env(env) {

rzarzynski · 2017-05-30T17:26:50Z

src/rgw/rgw_common.cc

-  map<string, string, ltstr_nocase>& m = env->get_map();
-  map<string, string, ltstr_nocase>::iterator iter;
-  for (iter = m.begin(); iter != m.end(); ++iter) {
+  for (const auto& iter: env->get_map()) {


Hmm, technically it isn't an iterator anymore. Maybe just kv instead?

rzarzynski · 2017-05-30T17:27:25Z

src/rgw/rgw_common.cc

@@ -427,8 +424,8 @@ void req_info::init_meta_info(bool *found_bad_meta)
      }
    }
  }
-  for (iter = x_meta_map.begin(); iter != x_meta_map.end(); ++iter) {
-    dout(10) << "x>> " << iter->first << ":" << rgw::crypt_sanitize::x_meta_map{iter->first, iter->second} << dendl;
+  for (const auto& iter: x_meta_map) {


The same as above (I stop mentioning that).

rzarzynski · 2017-05-30T17:29:33Z

src/rgw/rgw_common.h

@@ -383,16 +383,16 @@ class RGWEnv {
  void init(CephContext *cct);
  void init(CephContext *cct, char **envp);
  void set(const boost::string_ref& name, const boost::string_ref& val);
-  const char *get(const char *name, const char *def_val = NULL);
-  int get_int(const char *name, int def_val = 0);
+  const char *get(const char *name, const char *def_val = NULL) const;


As we're already touching the line it might be worth to switch to nullptr as well.

rzarzynski · 2017-05-30T17:30:58Z

src/rgw/rgw_common.cc

-          string old = iter->second;
+        auto it = x_meta_map.find(name_low);
+        if (it != x_meta_map.end()) {
+          string old = it->second;
          int pos = old.find_last_not_of(" \t"); /* get rid of any whitespaces after the value */


May we have size_t in place of int?

rzarzynski · 2017-05-30T17:33:19Z

src/rgw/rgw_rest_client.cc

-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  const auto & m = new_env.get_map();
+  for (const auto& iter: m) {
+    headers.push_back(pair<string, string>(iter.first, iter.second));


std::make_pair could be helpful here.

headers.emplace_back(iter) works here as well

rzarzynski · 2017-05-30T17:43:47Z

src/rgw/rgw_common.cc

-          string old = iter->second;
+        auto it = x_meta_map.find(name_low);
+        if (it != x_meta_map.end()) {
+          string old = it->second;
          int pos = old.find_last_not_of(" \t"); /* get rid of any whitespaces after the value */
          old = old.substr(0, pos + 1);


I know it's not directly related to the proposed changes but we might have an issue here. AFAIK std::string::npos usually is the max of size_t, so adding 1 to it would give us 0. If so, the substr(0, 0) eradicates the old value. This isn't coherent with the comment above if my understanding is correct.

checking for pos != string::npos (or use boost::string::trim_right..)

I vote for trim_right. We're already using boost::algorithm::trim in couple of places.

rzarzynski · 2017-05-30T17:57:04Z

src/rgw/rgw_common.cc

@@ -201,7 +201,7 @@ static string get_abs_path(const string& request_uri) {
  return request_uri.substr(beg_pos, len - beg_pos);
 }

-req_info::req_info(CephContext *cct, class RGWEnv *e) : env(e) {
+req_info::req_info(CephContext *cct, class RGWEnv *_env) : env(_env) {


Just a suggestion: maybe req_info::env could be turned into a pointer to constant. If I see correctly the methods of req_info call only RGWEnv::get.

Yeah I thought of going that route, and got around to fixing all the places where we mutate the map without the methods. I also vote for this.

theanalyst · 2017-05-30T20:46:18Z

src/rgw/rgw_common.cc

-  map<string, string, ltstr_nocase>& m = env->get_map();
-  map<string, string, ltstr_nocase>::iterator iter;
-  for (iter = m.begin(); iter != m.end(); ++iter) {
+  for (const auto& iter: env->get_map()) {


theanalyst · 2017-05-30T20:46:33Z

src/rgw/rgw_common.cc

-          string old = iter->second;
+        auto it = x_meta_map.find(name_low);
+        if (it != x_meta_map.end()) {
+          string old = it->second;
          int pos = old.find_last_not_of(" \t"); /* get rid of any whitespaces after the value */


theanalyst · 2017-05-30T20:47:32Z

src/rgw/rgw_common.cc

-  for (iter = x_meta_map.begin(); iter != x_meta_map.end(); ++iter) {
-    dout(10) << "x>> " << iter->first << ":" << rgw::crypt_sanitize::x_meta_map{iter->first, iter->second} << dendl;
+  for (const auto& iter: x_meta_map) {
+    dout(10) << "x>> " << iter.first << ":" << rgw::crypt_sanitize::x_meta_map{iter.first, iter.second} << dendl;


also do the dout->ldout stuff while I'm here

unfortunately don't have a context, I just looked at the code in github's ui which folded the code when I made the comment

theanalyst · 2017-05-30T22:39:40Z

src/rgw/rgw_common.cc

-          string old = iter->second;
+        auto it = x_meta_map.find(name_low);
+        if (it != x_meta_map.end()) {
+          string old = it->second;
          int pos = old.find_last_not_of(" \t"); /* get rid of any whitespaces after the value */
          old = old.substr(0, pos + 1);


checking for pos != string::npos (or use boost::string::trim_right..)

theanalyst · 2017-05-30T22:39:51Z

src/rgw/rgw_common.cc

@@ -427,8 +424,8 @@ void req_info::init_meta_info(bool *found_bad_meta)
      }
    }
  }
-  for (iter = x_meta_map.begin(); iter != x_meta_map.end(); ++iter) {
-    dout(10) << "x>> " << iter->first << ":" << rgw::crypt_sanitize::x_meta_map{iter->first, iter->second} << dendl;
+  for (const auto& iter: x_meta_map) {


theanalyst · 2017-05-30T22:40:07Z

src/rgw/rgw_common.h

@@ -383,16 +383,16 @@ class RGWEnv {
  void init(CephContext *cct);
  void init(CephContext *cct, char **envp);
  void set(const boost::string_ref& name, const boost::string_ref& val);
-  const char *get(const char *name, const char *def_val = NULL);
-  int get_int(const char *name, int def_val = 0);
+  const char *get(const char *name, const char *def_val = NULL) const;


theanalyst · 2017-05-30T22:41:04Z

src/rgw/rgw_common.h

-  const char *get(const char *name, const char *def_val = NULL);
-  int get_int(const char *name, int def_val = 0);
+  const char *get(const char *name, const char *def_val = NULL) const;
+  int get_int(const char *name, int def_val = 0) const;
  bool get_bool(const char *name, bool def_val = 0);


this should also be const if we go for const ptr

theanalyst · 2017-05-30T22:42:06Z

src/rgw/rgw_rest_client.cc

-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  const auto & m = new_env.get_map();
+  for (const auto& iter: m) {
+    headers.push_back(pair<string, string>(iter.first, iter.second));


theanalyst · 2017-05-30T22:42:42Z

src/rgw/rgw_rest_client.cc

-  for (iter = m.begin(); iter != m.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  for (const auto& iter: new_env.get_map()) {
+    headers.push_back(pair<string, string>(iter.first, iter.second));


theanalyst · 2017-05-30T22:48:32Z

src/rgw/rgw_rest_conn.cc


    /* add original headers that start with HTTP_X_AMZ_ */
 #define SEARCH_AMZ_PREFIX "HTTP_X_AMZ_"
-    for (map<string, string>::iterator iter = orig_map.lower_bound(SEARCH_AMZ_PREFIX); iter != orig_map.end(); ++iter) {
-      const string& name = iter->first;
+    for (const auto& iter: orig_map) {


hm this doesn't look right, use the lower bound, and maybe #define -> constexpr?

theanalyst · 2017-05-31T08:54:58Z

src/rgw/rgw_env.cc

  if (iter == env_map.end())
    return def_val;

  const char *s = iter->second.c_str();
-  return atoll(s);  
+  return atoll(s);


since stol throws have to catch the exceptions as well

theanalyst · 2017-06-01T16:55:37Z

will squash the PRs after reviews

rzarzynski · 2017-06-01T22:37:00Z

src/rgw/rgw_rest_conn.cc

-#define SEARCH_AMZ_PREFIX "HTTP_X_AMZ_"
-    for (const auto& iter: orig_map) {
-      const string& name = iter.first;
+    #define SEARCH_AMZ_PREFIX "HTTP_X_AMZ_"


What do you think about something like below?

static constexpr char SEARCH_AMZ_PREFIX[] = "HTTP_X_AMZ_";

rzarzynski · 2017-06-01T22:59:06Z

src/rgw/rgw_rest_conn.cc

      if (name == "HTTP_X_AMZ_DATE") /* dont forward date from original request */
        continue;
-      if (name.compare(0, sizeof(SEARCH_AMZ_PREFIX) - 1, "HTTP_X_AMZ_") != 0)
+      if (name.compare(0, sizeof(SEARCH_AMZ_PREFIX) - 1, SEARCH_AMZ_PREFIX) != 0)


I prefer to use std::strlen() instead of the sizeof() - 1. A reasonable compiler should optimise it out while it's a bit more descriptive and consistent with the case of having \0 in the middle of a char array. Though, this cosmetics is less than a suggestion.

let's make sure that compiler actually optimizes it correctly before changing it

The test program:

#include <cstring> #include <iostream> int main () { static constexpr char test_string[] = "Just a test C\0string"; std::cout << "the length is: " << std::strlen(test_string) << std::endl; }

Verification with GCC 4.8.4 on AMD64:

$ g++ -std=c++11 /tmp/test.cc -o /tmp/test $ /tmp/test the length is: 13 $ objdump -dC /tmp/test ... 000000000040089d <main>: 40089d: 55 push %rbp 40089e: 48 89 e5 mov %rsp,%rbp 4008a1: be c1 09 40 00 mov $0x4009c1,%esi 4008a6: bf 80 10 60 00 mov $0x601080,%edi 4008ab: e8 c0 fe ff ff callq 400770 <std::basic_ostream<char, std::char_traits<char> >& std::operator<< <std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*)@plt> 4008b0: be 0d 00 00 00 mov $0xd,%esi 4008b5: 48 89 c7 mov %rax,%rdi 4008b8: e8 c3 fe ff ff callq 400780 <std::ostream::operator<<(unsigned long)@plt> 4008bd: be a0 07 40 00 mov $0x4007a0,%esi 4008c2: 48 89 c7 mov %rax,%rdi 4008c5: e8 c6 fe ff ff callq 400790 <std::ostream::operator<<(std::ostream& (*)(std::ostream&))@plt> 4008ca: b8 00 00 00 00 mov $0x0,%eax 4008cf: 5d pop %rbp 4008d0: c3 retq ...

theanalyst · 2017-06-02T13:21:31Z

test this please

theanalyst · 2017-06-09T15:43:45Z

@rzarzynski updated

rzarzynski · 2017-06-11T23:51:01Z

src/rgw/rgw_rest_client.cc

@@ -454,7 +450,7 @@ int RGWRESTStreamWriteRequest::put_obj_init(RGWAccessKey& key, rgw_obj& obj, uin
  new_info.script_uri.append(resource);
  new_info.request_uri = new_info.script_uri;

-  map<string, string, ltstr_nocase>& m = new_env.get_map();
+


Do we need this empty line?

actually the next line also can be squashed if go for a range based for loop, pushing the changeset. I can always push back the previous version if we feel this is not necessary

rzarzynski · 2017-06-11T23:55:20Z

src/rgw/rgw_rest_client.cc

-  for (iter = m.begin(); iter != m.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  const auto & m = new_env.get_map();
+  for (const auto& iter: m) {


iter might be a bit confusing here. Maybe:

for (const auto& kv: new_env.get_map()) { headers.emplace_back(kv); }

thanks, more compact, did this in one other place, forgot here

rzarzynski · 2017-06-11T23:56:13Z

src/rgw/rgw_rest_client.cc

  }

  map<string, string>& meta_map = new_info.x_meta_map;
-  for (iter = meta_map.begin(); iter != meta_map.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  for (const auto& iter: meta_map) {


iter -> kv.

rzarzynski · 2017-06-11T23:58:06Z

src/rgw/rgw_rest_client.cc

-  for (iter = meta_map.begin(); iter != meta_map.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  for (const auto& iter: meta_map) {
+    headers.push_back(pair<string, string>(iter.first, iter.second));


headers.emplace_back(kv);

rzarzynski · 2017-06-11T23:59:21Z

src/rgw/rgw_rest_client.cc

-  map<string, string>::iterator iter;
-  for (iter = m.begin(); iter != m.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  for (const auto& iter: new_env.get_map()) {


iter -> kv.

rzarzynski · 2017-06-11T23:59:30Z

src/rgw/rgw_rest_client.cc

-  map<string, string>::iterator iter;
-  for (iter = m.begin(); iter != m.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  for (const auto& iter: new_env.get_map()) {


iter -> kv.

rzarzynski · 2017-06-12T00:00:05Z

src/rgw/rgw_rest_swift.cc

  const size_t put_prefix_len = strlen(put_prefix);
  const size_t del_prefix_len = strlen(del_prefix);

-  for (iter = m.begin(); iter != m.end(); ++iter) {
+  for (const auto& iter : s->info.env->get_map()) {


iter -> kv.

rzarzynski · 2017-06-12T00:02:57Z

@theanalyst: thanks for the update, Abhishek. I think this is the last iteration of review. The changes look good.

theanalyst · 2017-06-12T12:46:15Z

@rzarzynski thanks for the patient reviews, updated, also changed a for loop in rgw_rest_client.cc L453 to a range based for loop while i was at it, let me know if this is okay, or I can always fall back to the previous version

rzarzynski · 2017-06-12T13:05:48Z

src/rgw/rgw_rest_client.cc

-  for (iter = meta_map.begin(); iter != meta_map.end(); ++iter) {
-    headers.push_back(pair<string, string>(iter->first, iter->second));
+  for (const auto& kv: meta_map) {
+    headers.emplace_back(pair<string, string>(kv.first, kv.second));


Maybe:

for (const auto& kv: meta_map) { headers.emplace_back(kv); }

sure, silly me

rzarzynski · 2017-06-12T13:10:46Z

src/rgw/rgw_rest_client.cc

-  for (bliter = attrs.begin(); bliter != attrs.end(); ++bliter) {
-    bufferlist& bl = bliter->second;
-    const string& name = bliter->first;
+  for (auto attr: attrs) {


rzarzynski

LGTM. Thanks, @theanalyst! I'm scheduling a Teuthology run.

rzarzynski · 2017-06-12T16:19:54Z

jenkins retest this please (Agent went offline during the build)

rzarzynski · 2017-06-13T15:32:58Z

jenkins retest this please (cephtool-test-rados.sh)

rzarzynski · 2017-06-14T11:06:02Z

@theanalyst: the Teuthology runs look clean:

Unfortunately, we've gotten a (trivial) merge conflict in rgw/rgw_crypt.cc. Could you please rebase?

We already have a public method `set` for setting values in the RGW env map, making the map as a read only. In addition: - Added const to other methods in `RGWEnv` which are getters - `req_info::init_meta` also reused the same `iter` name for an internal find, changed the var name - `add_grants_headers` now accepts an RGWEnv instead of the map - use range based for loops wherever the code was changed - req_info holds a ptr to a const RGWEnv, since we seem to only read the values from it Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

theanalyst · 2017-06-14T12:48:26Z

@rzarzynski thanks! done

rzarzynski · 2017-06-14T13:39:30Z

jenkins retest this please (test_objectstore_memstore.sh)

theanalyst requested review from rzarzynski and cbodley May 24, 2017 15:40

cbodley added cleanup rgw labels May 25, 2017

rzarzynski reviewed May 30, 2017

View reviewed changes

rzarzynski self-assigned this May 30, 2017

theanalyst commented May 30, 2017

View reviewed changes

theanalyst commented May 31, 2017

View reviewed changes

theanalyst changed the title ~~rgw: make RGWEnv return a const ref. to its map~~ wip: rgw: make RGWEnv return a const ref. to its map Jun 1, 2017

rzarzynski reviewed Jun 1, 2017

View reviewed changes

theanalyst force-pushed the rgw/env-fix branch from 6d876e6 to 0d000e3 Compare June 2, 2017 11:38

theanalyst force-pushed the rgw/env-fix branch 2 times, most recently from 0579742 to 904c2d8 Compare June 6, 2017 14:13

theanalyst changed the title ~~wip: rgw: make RGWEnv return a const ref. to its map~~ rgw: make RGWEnv return a const ref. to its map Jun 9, 2017

rzarzynski reviewed Jun 11, 2017

View reviewed changes

rzarzynski reviewed Jun 12, 2017

View reviewed changes

theanalyst force-pushed the rgw/env-fix branch 2 times, most recently from a2b36d4 to 51d7fd5 Compare June 12, 2017 12:38

rzarzynski reviewed Jun 12, 2017

View reviewed changes

theanalyst force-pushed the rgw/env-fix branch from 51d7fd5 to 3ab0b69 Compare June 12, 2017 14:19

rzarzynski approved these changes Jun 12, 2017

View reviewed changes

theanalyst force-pushed the rgw/env-fix branch from 3ab0b69 to 1afcc38 Compare June 14, 2017 12:32

rzarzynski merged commit 04f9789 into ceph:master Jun 14, 2017

rgw: make RGWEnv return a const ref. to its map #15269

rgw: make RGWEnv return a const ref. to its map #15269

Conversation

theanalyst commented May 24, 2017 • edited Loading

rzarzynski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanalyst commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanalyst commented Jun 2, 2017

theanalyst commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski commented Jun 12, 2017

theanalyst commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski left a comment

Choose a reason for hiding this comment

rzarzynski commented Jun 12, 2017

rzarzynski commented Jun 13, 2017

rzarzynski commented Jun 14, 2017

theanalyst commented Jun 14, 2017

rzarzynski commented Jun 14, 2017

theanalyst commented May 24, 2017 •

edited

Loading

rzarzynski Jun 11, 2017 •

edited

Loading