Project

General

Profile

Bug #13829

config set with negative value results in "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument"

Added by Brad Hubbard about 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
OSD
Target version:
-
Start date:
11/19/2015
Due date:
% Done:

0%

Source:
Community (user)
Tags:
Backport:
hammer
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Pull request ID:

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.

test.cc View (1.62 KB) Kefu Chai, 11/19/2015 07:58 AM


Related issues

Copied to Ceph - Backport #16680: hammer: config set with negative value results in "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument" Resolved

Associated revisions

Revision 8b777a0c (diff)
Added by Brad Hubbard almost 3 years ago

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 <>
Signed-off-by: Brad Hubbard <>

Revision 994ac294 (diff)
Added by Brad Hubbard almost 3 years ago

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 <>

Revision f70e4adf (diff)
Added by Brad Hubbard over 2 years ago

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 <>
Signed-off-by: Brad Hubbard <>
(cherry picked from commit 8b777a0c346bc70fd10d07e89368b3785b58f10e)

Revision 3a30ffc2 (diff)
Added by Brad Hubbard over 2 years ago

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 <>
(cherry picked from commit 994ac2942d04584d1617e6d4bbd5b880b1ea0448)

History

#1 Updated by Kefu Chai about 3 years ago

  • Description updated (diff)

#2 Updated by Kefu Chai about 3 years ago

brad, i attached a POC to explain my idea to this change.

#3 Updated by Brad Hubbard almost 3 years ago

  • Status changed from New to In Progress

#4 Updated by Brad Hubbard almost 3 years ago

  • Status changed from In Progress to Resolved

Fixed in commit 389ecbbc2a9d7d4d334ea31a5c4120e9acc1b9aa

#5 Updated by Brad Hubbard over 2 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 2 years ago

  • Backport set to hammer

#7 Updated by Kefu Chai over 2 years ago

  • Status changed from Resolved to Pending Backport

#8 Updated by Brad Hubbard over 2 years ago

  • Status changed from Pending Backport to Resolved

Also available in: Atom PDF