Project

General

Profile

Backport #16680

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

Added by Brad Hubbard almost 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
High
Assignee:
Target version:
Release:
hammer
Crash signature (v1):
Crash signature (v2):


Related issues

Copied from Ceph - Bug #13829: config set with negative value results in "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument" Resolved 11/19/2015

History

#1 Updated by Brad Hubbard almost 6 years ago

  • Copied from Bug #13829: config set with negative value results in "error setting 'filestore_merge_threshold' to '-40': (22) Invalid argument" added

#2 Updated by Brad Hubbard almost 6 years ago

  • Status changed from New to Fix Under Review

#3 Updated by Kefu Chai almost 6 years ago

  • Target version deleted (v0.94.8)

#4 Updated by Kefu Chai almost 6 years ago

  • Status changed from Fix Under Review to In Progress

#5 Updated by Nathan Cutler almost 6 years ago

  • Description updated (diff)

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

#6 Updated by Brad Hubbard almost 6 years ago

  • Status changed from In Progress to Resolved
  • Target version set to v0.94.8

Also available in: Atom PDF