New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common,config: OPT_FLOAT and OPT_DOUBLE output format in config show #15647

Merged
merged 2 commits into from Jul 7, 2017

Conversation

Projects
None yet
5 participants
@gmayyyha
Contributor

gmayyyha commented Jun 13, 2017

@Liuchang0812

LGTM, thanks for your work.

@@ -949,6 +953,10 @@ int md_config_t::_get_val(const char *key, char **buf, int len) const
ostringstream oss;
if (bool *flagp = boost::get<bool>(&cval)) {
oss << (*flagp ? "true" : "false");
} else if (float *fp = boost::get<float>(&cval)) {

This comment has been minimized.

@tchaikov

tchaikov Jun 13, 2017

Contributor

can we refactor this method to reuse md_config_t::_get_val(const char *key, std::string *value)?

This comment has been minimized.

@gmayyyha

gmayyyha Jun 13, 2017

Contributor

like this ?

@@ -946,33 +946,21 @@ int md_config_t::_get_val(const char *key, char **buf, int len) const
   if (!key)
     return -EINVAL;
 
-  string k(ConfFile::normalize_key_name(key));
-
-  config_value_t cval = _get_val(k.c_str());
-  if (!boost::get<invalid_config_value_t>(&cval)) {
-    ostringstream oss;
-    if (bool *flagp = boost::get<bool>(&cval)) {
-      oss << (*flagp ? "true" : "false");
-    } else if (float *fp = boost::get<float>(&cval)) {
-      oss << std::fixed << *fp ;
-    } else if (double *dp = boost::get<double>(&cval)) {
-      oss << std::fixed << *dp ;
-    } else {
-      oss << cval;
-    }
-    string str(oss.str());
-    int l = strlen(str.c_str()) + 1;
+  string val ;
+  if (!_get_val(key, &val)) {
+    int l = strlen(val.c_str()) + 1;
     if (len == -1) {
       *buf = (char*)malloc(l);
       if (!*buf)
         return -ENOMEM;
-      strcpy(*buf, str.c_str());
+      strcpy(*buf, val.c_str());
       return 0;
     }
-    snprintf(*buf, len, "%s", str.c_str());
+    snprintf(*buf, len, "%s", val.c_str());
     return (l > len) ? -ENAMETOOLONG : 0;
-  }
+  } 
 
+  string k(ConfFile::normalize_key_name(key));

This comment has been minimized.

@tchaikov

tchaikov Jun 13, 2017

Contributor

something like that, we can have two commits here. one for the refactor, the other for fixing the output format.

also, you don't need to use strlen() for the length of a std::string. you could just use val.length() instead. and please use strncpy() if possible.

@ceph-jenkins

This comment has been minimized.

Collaborator

ceph-jenkins commented Jun 14, 2017

Can one of the admins verify this patch?

int l = strlen(str.c_str()) + 1;
string val ;
if (!_get_val(key, &val)) {
int l = val.length()+1;

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

please add spaces around +

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

and could you reorder these two commits? so the refactor one is the first commit, and the fix is the second.

This comment has been minimized.

@gmayyyha

gmayyyha Jun 14, 2017

Contributor

spaces are already used

qq20170614-141954

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

could you check again?

int l = val.length()+1;

This comment has been minimized.

@gmayyyha

gmayyyha Jun 14, 2017

Contributor

is this correct now ?

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

no. @gmayyyha you can review your commit with the github web UI by yourself as well, see https://github.com/ceph/ceph/pull/15647/files

int l = strlen(str.c_str()) + 1;
string val ;
if (!_get_val(key, &val)) {
int l = val.length()+1;

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

could you check again?

int l = val.length()+1;
return (l > len) ? -ENAMETOOLONG : 0;
}
}

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

and please remove the trailing space.

This comment has been minimized.

@gmayyyha

gmayyyha Jun 16, 2017

Contributor

@tchaikov and now ?

gmayyyha added some commits Jun 14, 2017

common,config: refactor method md_config_t::_get_val(const char *, ch…
…ar**, int) const

Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
@Liuchang0812

thanks to your work. hope this could be merged as soon as possible. It is also needed to back-port to H/K release.

return (l > len) ? -ENAMETOOLONG : 0;
}
string k(ConfFile::normalize_key_name(key));

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jun 27, 2017

Contributor

k is useless. remove this line.

This comment has been minimized.

@gmayyyha

gmayyyha Jun 28, 2017

Contributor

@Liuchang0812 k is used as follows

965   for (int o = 0; o < subsys.get_num(); o++) {
966     std::string as_option = "debug_" + subsys.get_name(o);
967     if (k == as_option) {

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jun 28, 2017

Contributor

ok. good.

return 0;
}
snprintf(*buf, len, "%s", str.c_str());
snprintf(*buf, len, "%s", val.c_str());
return (l > len) ? -ENAMETOOLONG : 0;

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jun 27, 2017

Contributor

@tchaikov hi, kefu, I'm confused here. We should allocate memory when len is -1, and we will always return ENAMETOOLONG error code. is this what we expect?

This comment has been minimized.

@tchaikov

tchaikov Jun 28, 2017

Contributor

We should allocate memory when len is -1,

correct.

and we will always return ENAMETOOLONG error code. is this what we expect?

i don't think so.

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jun 28, 2017

Contributor

I created a issue to track this issue[1]. I will write some UT to verify it.

1: http://tracker.ceph.com/issues/20442

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jun 28, 2017

Contributor

I missed line957. _get_val will not return ENAMETOOLONG when len is -1.

if (len == -1) {
*buf = (char*)malloc(l);
if (!*buf)
return -ENOMEM;
strcpy(*buf, str.c_str());
strncpy(*buf, val.c_str(), l);

This comment has been minimized.

@Liuchang0812

Liuchang0812 Jun 27, 2017

Contributor

we could simplify line 956 and 959 to one line actually. anyway, I'm fine with both.

@tchaikov tchaikov added the needs-qa label Jun 28, 2017

@liewegas liewegas merged commit 5870a68 into ceph:master Jul 7, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details

@gmayyyha gmayyyha deleted the gmayyyha:format-config-show branch Jul 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment