On 2/5/21 4:07 AM, Vladimir Sementsov-Ogievskiy wrote: >> @@ -2106,6 +2137,21 @@ static void test_qemu_strtosz_invalid(void) >> err = qemu_strtosz(str, &endptr, &res); >> g_assert_cmpint(err, ==, -EINVAL); >> g_assert(endptr == str); >> + >> + str = "1.1e5"; >> + err = qemu_strtosz(str, &endptr, &res); >> + g_assert_cmpint(err, ==, -EINVAL); >> + g_assert(endptr == str); > > I'd add also test with 'E'
Will do. For v2, I'll probably split this patch, first into adding new test cases (with demonstrations of what we deem to be buggy parses), and the second showing how those cases improve as we change the code. > >> + >> + str = "1.1B"; >> + err = qemu_strtosz(str, &endptr, &res); >> + g_assert_cmpint(err, ==, -EINVAL); >> + g_assert(endptr == str); >> + >> + str = "0x1.8k"; >> + err = qemu_strtosz(str, &endptr, &res); >> + g_assert_cmpint(err, ==, -EINVAL); >> + g_assert(endptr == str); > > ha this function looks like it should have > > const cher *str[] = ["", " \t ", ... "0x1.8k"] > > and all cases in a for()... and all other test cases may be ... Oh, I > should say myself "don't refactor everything you see". I'll think about it. It's already odd that the tests are split between so many functions. >> @@ -668,7 +668,7 @@ static void test_opts_parse_size(void) >> g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1), >> ==, 0x20000000000000); >> g_assert_cmphex(qemu_opt_get_size(opts, "size3", 1), >> - ==, 0x20000000000000); >> + ==, 0x20000000000001); >> >> /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */ > > should fix comment? Yes, I did it in one file but not the other, so I'll make it consistent. The point of the comment post-patch is that we are demonstrating that our implementation is NOT bound by the limits of a double's precision. >> - retval = qemu_strtod_finite(nptr, &endptr, &val); >> + /* Parse integral portion as decimal. */ >> + retval = qemu_strtou64(nptr, &endptr, 10, &val); >> if (retval) { >> goto out; >> } >> - fraction = modf(val, &integral); >> - if (fraction != 0) { >> - mul_required = 1; >> + if (strchr(nptr, '-') != NULL) { >> + retval = -ERANGE; >> + goto out; >> + } > > Hmm, I have a question: does do_strtosz assumes that the whole nptr > string is a number? No. In fact, our testsuite demonstrates the difference between endptr as a pointer (we parse what we can and advance *endptr to the tail) and as NULL (trailing garbage is an error). > > If yes, then I don't understand what the matter of **end OUT-argument. > > If not, this if() is wrong, as you'll redject parsing first number of > "123425 - 2323" string.. Yep, good catch. This needs to use memchr, similar to the check for 'e' in the fraction portion below. >> + retval = qemu_strtod_finite(endptr, &endptr, &fraction); >> + if (retval) { >> + endptr = nptr; >> + goto out; >> + } >> + if (fraction >= 1.0 || memchr(nptr, 'e', endptr - nptr) >> + || memchr(nptr, 'E', endptr - nptr)) { >> + endptr = nptr; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org