Bug #13829
config set with negative value results in "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument"
0%
Description
In the filestore-config-ref we say...
filestore merge threshold
Description: Min number of files in a subdir before merging into parent NOTE: A > negative value means to disable subdir merging
But running the following on 0.94.3 (95cefea9fd9ab740263bf8bb4796fd864d9afe2b) gives an error.
# ceph --admin-daemon /var/run/ceph/ceph-osd.312.asok config set filestore_merge_threshold -40 { "error": "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument" }
0.94.1 (e4bfad3a3c51054df7e537a724c8d0bf9be972ff) is not affected so I did a git bisect to find the commit that introduced this behavior.
$ git bisect bad 9b947fa320b77e0055a581005353c2561a12a198 is the first bad commit commit 9b947fa320b77e0055a581005353c2561a12a198 Author: Kefu Chai <kchai@redhat.com> Date: Wed Apr 29 15:41:08 2015 +0800 common/config: detect overflow of int values * #include "strtol.h" in strtol.cc, to ensure the function defintions are consistent. * add a test accordingly * fix the testcase of converting 1024E. * do not accept integers overflow after adding SI suffix * do not accept integers underflow (i.e. negative values) Fixes: #11484
That patch includes the following code to src/common/strtol.cc.
163 long long r_ll = strict_strtoll(v, 10, err); 164 165 if (r_ll < 0) { 166 *err = "strict_sistrtoll: value should not be negative"; 167 return 0; 168 } 169 170 uint64_t r = r_ll; 171 if (err->empty() && m > 0) { 172 if (r > (std::numeric_limits<uint64_t>::max() >> m)) { 173 *err = "strict_sistrtoll: value seems to be too large"; 174 return 0; 175 } 176 r <<= m; 177 }
and this to src/common/strtol.h
34 template <typename Target> 35 Target strict_si_cast(const char *str, std::string *err) { 36 uint64_t ret = strict_sistrtoll(str, err); 37 if (!err->empty()) 38 return ret; 39 if (ret > (uint64_t)std::numeric_limits<Target>::max()) { 40 err->append("The option value '"); 41 err->append(str); 42 err->append("' seems to be too large"); 43 return 0; 44 } 45 return ret; 46 }
Negative values fail both the test for a negative value (of course) and the tests
for uint64_t overflow.
Working with kefu on a patch so assigning this to myself.
Related issues
Associated revisions
common: Allow config set with negative value
A recent commit disabled negative values but they are required for variables
such as filestore_merge_threshold.
Fixes: #13829
Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
qa: Add test for #13829
qa/workunits/cephtool/test.sh: add test for setting negative int options
src/test/daemon_config.cc: remove tests for failed negative values
Fixes: #13829
Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
common: Allow config set with negative value
A recent commit disabled negative values but they are required for variables
such as filestore_merge_threshold.
Modified patch to remove C++11 specific elements so it will build for hammer
Fixes: #13829
Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
(cherry picked from commit 8b777a0c346bc70fd10d07e89368b3785b58f10e)
qa: Add test for #13829
qa/workunits/cephtool/test.sh: add test for setting negative int options
src/test/daemon_config.cc: remove tests for failed negative values
Fixes: #13829
Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
(cherry picked from commit 994ac2942d04584d1617e6d4bbd5b880b1ea0448)
History
#1 Updated by Kefu Chai over 8 years ago
- Description updated (diff)
#2 Updated by Kefu Chai over 8 years ago
brad, i attached a POC to explain my idea to this change.
#3 Updated by Brad Hubbard about 8 years ago
- Status changed from New to In Progress
#4 Updated by Brad Hubbard about 8 years ago
- Status changed from In Progress to Resolved
Fixed in commit 389ecbbc2a9d7d4d334ea31a5c4120e9acc1b9aa
#5 Updated by Brad Hubbard over 7 years ago
- Copied to Backport #16680: hammer: config set with negative value results in "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument" added
#6 Updated by Kefu Chai over 7 years ago
- Backport set to hammer
#7 Updated by Kefu Chai over 7 years ago
- Status changed from Resolved to Pending Backport
#8 Updated by Brad Hubbard over 7 years ago
- Status changed from Pending Backport to Resolved