From: Andreas Dilger Date: Fri, 10 May 2019 21:09:41 +0000 (-0600) Subject: LU-9091 obdclass: allow bare KMGTPE param suffix X-Git-Tag: 2.13.53~43 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=d105cc4a6448b3c842790d0e1f0380b94d087f6f LU-9091 obdclass: allow bare KMGTPE param suffix Allow sysfs_memparse() to parse bare "KMGTPE" suffixes as base-2. This simplifies the code to not append the unnecessary "iB" if the suffix is one of KMGTPE already, since these are assumed to be binary units already. Don't append a bare defunit of "B", since this unnecessarily copies the input string and takes the slow path for parsing the string, but doesn't actually change the resulting value. The test_string_to_size_one() may only have printed an error and not caused module load to actually fail. Print a message and return an error if the tests fail. Some checks expect to fail, so add test_string_to_size_err() to verify that an error was returned. Mark the obd_init_checks() function with __init so it is dropped after the module is loaded. Fixes: d9e0c9f346d0 ("LU-9091 sysfs: use string helper like functions for sysfs") Signed-off-by: Andreas Dilger Change-Id: I3cdf5f8f0aeca458ed1989366102c33ae83ebbe5 Reviewed-on: https://review.whamcloud.com/37620 Tested-by: jenkins Tested-by: Maloo Reviewed-by: James Simmons Reviewed-by: Wang Shilong Reviewed-by: Oleg Drokin --- diff --git a/lustre/obdclass/class_obd.c b/lustre/obdclass/class_obd.c index 438e142..8543769 100644 --- a/lustre/obdclass/class_obd.c +++ b/lustre/obdclass/class_obd.c @@ -524,131 +524,152 @@ struct miscdevice obd_psdev = { .fops = &obd_psdev_fops, }; -#define test_string_to_size_one(value, result, def_unit) \ -({ \ - u64 __size; \ - int __ret; \ - \ - BUILD_BUG_ON(strlen(value) >= 23); \ - __ret = sysfs_memparse((value), (result), &__size, \ - (def_unit)); \ - if (__ret == 0 && (u64)result != __size) \ - CERROR("string_helper: size %llu != result %llu\n",\ - __size, (u64)result); \ - __ret; \ +#define test_string_to_size_err(value, expect, def_unit, __rc) \ +({ \ + u64 __size; \ + int __ret; \ + \ + BUILD_BUG_ON(sizeof(value) >= 23); \ + __ret = sysfs_memparse(value, sizeof(value) - 1, &__size, def_unit); \ + if (__ret != __rc) \ + CERROR("string_helper: parsing '%s' expect rc %d != got %d\n", \ + value, __rc, __ret); \ + else if (!__ret && (u64)expect != __size) \ + CERROR("string_helper: parsing '%s' expect %llu != got %llu\n",\ + value, (u64)expect, __size); \ + __ret; \ }) +#define test_string_to_size_one(value, expect, def_unit) \ + test_string_to_size_err(value, expect, def_unit, 0) -static int obd_init_checks(void) +static int __init obd_init_checks(void) { - __u64 u64val, div64val; - char buf[64]; - int len, ret = 0; + __u64 u64val, div64val; + char buf[64]; + int len, ret = 0; CDEBUG(D_INFO, "OBD_OBJECT_EOF = %#llx\n", (__u64)OBD_OBJECT_EOF); - u64val = OBD_OBJECT_EOF; + u64val = OBD_OBJECT_EOF; CDEBUG(D_INFO, "u64val OBD_OBJECT_EOF = %#llx\n", u64val); - if (u64val != OBD_OBJECT_EOF) { + if (u64val != OBD_OBJECT_EOF) { CERROR("__u64 %#llx(%d) != 0xffffffffffffffff\n", - u64val, (int)sizeof(u64val)); - ret = -EINVAL; - } + u64val, (int)sizeof(u64val)); + ret = -EINVAL; + } len = snprintf(buf, sizeof(buf), "%#llx", u64val); - if (len != 18) { - CWARN("u64 hex wrong length! strlen(%s)=%d != 18\n", buf, len); - ret = -EINVAL; - } + if (len != 18) { + CERROR("u64 hex wrong length, strlen(%s)=%d != 18\n", buf, len); + ret = -EINVAL; + } - div64val = OBD_OBJECT_EOF; + div64val = OBD_OBJECT_EOF; CDEBUG(D_INFO, "u64val OBD_OBJECT_EOF = %#llx\n", u64val); - if (u64val != OBD_OBJECT_EOF) { + if (u64val != OBD_OBJECT_EOF) { CERROR("__u64 %#llx(%d) != 0xffffffffffffffff\n", - u64val, (int)sizeof(u64val)); - ret = -EOVERFLOW; - } - if (u64val >> 8 != OBD_OBJECT_EOF >> 8) { + u64val, (int)sizeof(u64val)); + ret = -EOVERFLOW; + } + if (u64val >> 8 != OBD_OBJECT_EOF >> 8) { CERROR("__u64 %#llx(%d) != 0xffffffffffffffff\n", - u64val, (int)sizeof(u64val)); - return -EOVERFLOW; - } - if (do_div(div64val, 256) != (u64val & 255)) { + u64val, (int)sizeof(u64val)); + ret = -EOVERFLOW; + } + if (do_div(div64val, 256) != (u64val & 255)) { CERROR("do_div(%#llx,256) != %llu\n", u64val, u64val & 255); - return -EOVERFLOW; - } - if (u64val >> 8 != div64val) { + ret = -EOVERFLOW; + } + if (u64val >> 8 != div64val) { CERROR("do_div(%#llx,256) %llu != %llu\n", - u64val, div64val, u64val >> 8); - return -EOVERFLOW; - } + u64val, div64val, u64val >> 8); + ret = -EOVERFLOW; + } len = snprintf(buf, sizeof(buf), "%#llx", u64val); - if (len != 18) { - CWARN("u64 hex wrong length! strlen(%s)=%d != 18\n", buf, len); - ret = -EINVAL; - } + if (len != 18) { + CERROR("u64 hex wrong length! strlen(%s)=%d != 18\n", buf, len); + ret = -EINVAL; + } len = snprintf(buf, sizeof(buf), "%llu", u64val); - if (len != 20) { - CWARN("u64 wrong length! strlen(%s)=%d != 20\n", buf, len); - ret = -EINVAL; - } + if (len != 20) { + CERROR("u64 wrong length! strlen(%s)=%d != 20\n", buf, len); + ret = -EINVAL; + } len = snprintf(buf, sizeof(buf), "%lld", u64val); - if (len != 2) { - CWARN("s64 wrong length! strlen(%s)=%d != 2\n", buf, len); - ret = -EINVAL; - } + if (len != 2) { + CERROR("s64 wrong length! strlen(%s)=%d != 2\n", buf, len); + ret = -EINVAL; + } if ((u64val & ~PAGE_MASK) >= PAGE_SIZE) { - CWARN("mask failed: u64val %llu >= %llu\n", u64val, - (__u64)PAGE_SIZE); - ret = -EINVAL; - } + CERROR("mask failed: u64val %llu >= %llu\n", u64val, + (__u64)PAGE_SIZE); + ret = -EINVAL; + } + if (ret) + RETURN(ret); /* invalid string */ - ret = test_string_to_size_one("256B34", 256, "B"); - if (ret == 0) + if (!test_string_to_size_err("256B34", 256, "B", -EINVAL)) { CERROR("string_helpers: format should be number then units\n"); - ret = test_string_to_size_one("132OpQ", 132, "B"); - if (ret == 0) + ret = -EINVAL; + } + if (!test_string_to_size_err("132OpQ", 132, "B", -EINVAL)) { CERROR("string_helpers: invalid units should be rejected\n"); - ret = 0; + ret = -EINVAL; + } + if (!test_string_to_size_err("1.82B", 1, "B", -EINVAL)) { + CERROR("string_helpers: 'B' with '.' should be invalid\n"); + ret = -EINVAL; + } + if (test_string_to_size_one("343\n", 343, "B")) { + CERROR("string_helpers: should ignore newline\n"); + ret = -EINVAL; + } + if (ret) + RETURN(ret); - /* small values */ - test_string_to_size_one("0B", 0, "B"); - ret = test_string_to_size_one("1.82B", 1, "B"); - if (ret == 0) - CERROR("string_helpers: number string with 'B' and '.' should be invalid\n"); + /* memparse unit handling */ ret = 0; - test_string_to_size_one("512B", 512, "B"); - test_string_to_size_one("1.067kB", 1067, "B"); - test_string_to_size_one("1.042KiB", 1067, "B"); - - /* Lustre special handling */ - test_string_to_size_one("16", 16777216, "MiB"); - test_string_to_size_one("65536", 65536, "B"); - test_string_to_size_one("128K", 131072, "B"); - test_string_to_size_one("1M", 1048576, "B"); - test_string_to_size_one("256.5G", 275414777856ULL, "GiB"); - - /* normal values */ - test_string_to_size_one("8.39MB", 8390000, "MiB"); - test_string_to_size_one("8.00MiB", 8388608, "MiB"); - test_string_to_size_one("256GB", 256000000, "GiB"); - test_string_to_size_one("238.731 GiB", 256335459385ULL, "GiB"); + ret += test_string_to_size_one("0B", 0, "B"); + ret += test_string_to_size_one("512B", 512, "B"); + ret += test_string_to_size_one("1.067kB", 1067, "B"); + ret += test_string_to_size_one("1.042KiB", 1067, "B"); + ret += test_string_to_size_one("8", 8388608, "M"); + ret += test_string_to_size_one("65536", 65536, "B"); + ret += test_string_to_size_one("128", 131072, "K"); + ret += test_string_to_size_one("1M", 1048576, "B"); + ret += test_string_to_size_one("0.5T", 549755813888ULL, "T"); + ret += test_string_to_size_one("256.5G", 275414777856ULL, "G"); + if (ret) + RETURN(ret); + + /* string helper values */ + ret += test_string_to_size_one("16", 16777216, "MiB"); + ret += test_string_to_size_one("8.39MB", 8390000, "MiB"); + ret += test_string_to_size_one("8.00MiB", 8388608, "MiB"); + ret += test_string_to_size_one("256GB", 256000000000ULL, "GiB"); + ret += test_string_to_size_one("238.731GiB", 256335459385ULL, "GiB"); + if (ret) + RETURN(ret); /* huge values */ - test_string_to_size_one("0.4TB", 400000000000ULL, "TiB"); - test_string_to_size_one("12.5TiB", 13743895347200ULL, "TiB"); - test_string_to_size_one("2PB", 2000000000000000ULL, "PiB"); - test_string_to_size_one("16PiB", 18014398509481984ULL, "PiB"); + ret += test_string_to_size_one("0.4TB", 400000000000ULL, "TiB"); + ret += test_string_to_size_one("12.5TiB", 13743895347200ULL, "TiB"); + ret += test_string_to_size_one("2PB", 2000000000000000ULL, "PiB"); + ret += test_string_to_size_one("16PiB", 18014398509481984ULL, "PiB"); + if (ret) + RETURN(ret); /* huge values should overflow */ - ret = test_string_to_size_one("1000EiB", 0, "EiB"); - if (ret != -EOVERFLOW) - CERROR("string_helpers: Failed to detect overflow\n"); - ret = test_string_to_size_one("1000EB", 0, "EiB"); - if (ret != -EOVERFLOW) - CERROR("string_helpers: Failed to detect overflow\n"); - ret = 0; + if (!test_string_to_size_err("1000EiB", 0, "EiB", -EOVERFLOW)) { + CERROR("string_helpers: failed to detect binary overflow\n"); + ret = -EINVAL; + } + if (!test_string_to_size_err("1000EB", 0, "EiB", -EOVERFLOW)) { + CERROR("string_helpers: failed to detect decimal overflow\n"); + ret = -EINVAL; + } - return ret; + return ret; } static int __init obdclass_init(void) @@ -660,7 +681,7 @@ static int __init obdclass_init(void) libcfs_kkuc_init(); err = obd_init_checks(); - if (err == -EOVERFLOW) + if (err) return err; #ifdef CONFIG_PROC_FS diff --git a/lustre/obdclass/lprocfs_status.c b/lustre/obdclass/lprocfs_status.c index e2eebfd..71d1edd 100644 --- a/lustre/obdclass/lprocfs_status.c +++ b/lustre/obdclass/lprocfs_status.c @@ -1650,7 +1650,7 @@ EXPORT_SYMBOL(lprocfs_read_helper); * * - ``-EINVAL``: @buffer is not a proper numerical string * - ``-EOVERFLOW``: results does not fit into 64 bits. - * - ``-E2BIG ``: @buffer is not large + * - ``-E2BIG ``: @buffer is too large (not a valid number) */ int string_to_size(u64 *size, const char *buffer, size_t count) { @@ -1660,10 +1660,10 @@ int string_to_size(u64 *size, const char *buffer, size_t count) * up into block size units so we don't support ZiB or YiB. */ static const char *const units_10[] = { - "kB", "MB", "GB", "TB", "PB", "EB" + "kB", "MB", "GB", "TB", "PB", "EB", }; static const char *const units_2[] = { - "KiB", "MiB", "GiB", "TiB", "PiB", "EiB" + "K", "M", "G", "T", "P", "E", }; static const char *const *const units_str[] = { [STRING_UNITS_2] = units_2, @@ -1673,30 +1673,34 @@ int string_to_size(u64 *size, const char *buffer, size_t count) [STRING_UNITS_10] = 1000, [STRING_UNITS_2] = 1024, }; - enum string_size_units unit; + enum string_size_units unit = STRING_UNITS_2; u64 whole, blk_size = 1; char kernbuf[22], *end; size_t len = count; int rc; int i; - if (count >= sizeof(kernbuf)) + if (count >= sizeof(kernbuf)) { + CERROR("count %zd > buffer %zd\n", count, sizeof(kernbuf)); return -E2BIG; + } *size = 0; - /* 'iB' is used for based 2 numbers. If @buffer contains only a 'B' - * or only numbers then we treat it as a direct number which doesn't - * matter if its STRING_UNITS_2 or STRING_UNIT_10. + /* The "iB" suffix is optionally allowed for indicating base-2 numbers. + * If suffix is only "B" and not "iB" then we treat it as base-10. */ - unit = strstr(buffer, "iB") ? STRING_UNITS_2 : STRING_UNITS_10; + end = strstr(buffer, "B"); + if (end && *(end - 1) != 'i') + unit = STRING_UNITS_10; + i = unit == STRING_UNITS_2 ? ARRAY_SIZE(units_2) - 1 : ARRAY_SIZE(units_10) - 1; do { - end = strstr(buffer, units_str[unit][i]); + end = strnstr(buffer, units_str[unit][i], count); if (end) { for (; i >= 0; i--) blk_size *= coeff[unit]; - len -= strlen(end); + len = end - buffer; break; } } while (i--); @@ -1706,19 +1710,21 @@ int string_to_size(u64 *size, const char *buffer, size_t count) */ if (!end) { /* 'B' is only acceptable letter at this point */ - end = strchr(buffer, 'B'); + end = strnchr(buffer, count, 'B'); if (end) { - len -= strlen(end); + len = end - buffer; if (count - len > 2 || - (count - len == 2 && strcmp(end, "B\n") != 0)) + (count - len == 2 && strcmp(end, "B\n") != 0)) { + CDEBUG(D_INFO, "unknown suffix '%s'\n", buffer); return -EINVAL; + } } /* kstrtoull will error out if it has non digits */ goto numbers_only; } - end = strchr(buffer, '.'); + end = strnchr(buffer, count, '.'); if (end) { /* need to limit 3 decimal places */ char rem[4] = "000"; @@ -1780,32 +1786,34 @@ EXPORT_SYMBOL(string_to_size); int sysfs_memparse(const char *buffer, size_t count, u64 *val, const char *defunit) { - char param[23]; + const char *param = buffer; + char tmp_buf[23]; int rc; - if (count >= sizeof(param)) - return -E2BIG; - count = strlen(buffer); - if (count && buffer[count - 1] == '\n') + while (count > 0 && isspace(buffer[count - 1])) count--; if (!count) - return -EINVAL; + RETURN(-EINVAL); - if (isalpha(buffer[count - 1])) { - if (buffer[count - 1] != 'B') { - scnprintf(param, sizeof(param), "%.*siB", - (int)count, buffer); - } else { - memcpy(param, buffer, sizeof(param)); + /* If there isn't already a unit on this value, append @defunit. + * Units of 'B' don't affect the value, so don't bother adding. + */ + if (!isalpha(buffer[count - 1]) && defunit[0] != 'B') { + if (count + 3 >= sizeof(tmp_buf)) { + CERROR("count %zd > size %zd\n", count, sizeof(param)); + RETURN(-E2BIG); } - } else { - scnprintf(param, sizeof(param), "%.*s%s", (int)count, + + scnprintf(tmp_buf, sizeof(tmp_buf), "%.*s%s", (int)count, buffer, defunit); + param = tmp_buf; + count = strlen(param); } - rc = string_to_size(val, param, strlen(param)); + rc = string_to_size(val, param, count); + return rc < 0 ? rc : 0; } EXPORT_SYMBOL(sysfs_memparse);