Re: [PATCH v2 04/19] test-cutils: Test more integer corner cases

[Hanna Czenczek <hreitz@redhat.com>]

On 12.05.23 04:10, Eric Blake wrote:
> We have quite a few undertested and underdocumented integer parsing
> corner cases.  To ensure that any changes we make in the code are
> intentional rather than accidental semantic changes, it is time to add
> more unit tests of existing behavior.
>
> In particular, this demonstrates that parse_uint() and qemu_strtou64()
> behave differently.  For "-0", it's hard to argue why parse_uint needs
> to reject it (it's not a negative integer), but the documentation sort
> of mentions it; but it is intentional that all other negative values
> are treated as ERANGE with value 0 (compared to qemu_strtou64()
> treating "-2" as success and UINT64_MAX-1, for example).
>
> Also, when mixing overflow/underflow with a check for no trailing
> junk, parse_uint_full favors ERANGE over EINVAL, while qemu_strto[iu]*
> favor EINVAL.  This behavior is outside the C standard, so we can pick
> whatever we want, but it would be nice to be consistent.
>
> Note that C requires that "9223372036854775808" fail strtoll() with
> ERANGE/INT64_MAX, but "-9223372036854775808" pass with INT64_MIN; we
> weren't testing this.  For strtol(), the behavior depends on whether
> long is 32- or 64-bits (the cutoff point either being the same as
> strtoll() or at "-2147483648").  Meanwhile, C is clear that
> "-18446744073709551615" pass stroull() (but not strtoll) with value 1,
> even though we want it to fail parse_uint().  And although
> qemu_strtoui() has no C counterpart, it makes more sense if we design
> it like 32-bit strtoul() (that is, where "-4294967296" be an alternate
> acceptable spelling for "1".  We aren't there yet, so some of the
> tests added in this patch have FIXME comments.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/unit/test-cutils.c | 799 ++++++++++++++++++++++++++++++++++++---
>   1 file changed, 738 insertions(+), 61 deletions(-)
>
> diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> index 1eeaf21ae22..89c10f5307a 100644
> --- a/tests/unit/test-cutils.c
> +++ b/tests/unit/test-cutils.c

[...]

> @@ -717,34 +890,75 @@ static void test_qemu_strtoui_max(void)
>
>   static void test_qemu_strtoui_overflow(void)
>   {
> -    char *str = g_strdup_printf("%lld", (long long)UINT_MAX + 1ll);
> -    char f = 'X';
> -    const char *endptr = &f;
> -    unsigned int res = 999;
> +    const char *str;
> +    const char *endptr;
> +    unsigned int res;
>       int err;
>
> +    str = "4294967296"; /* UINT_MAX + 1ll */
> +    endptr = "somewhere";
> +    res = 999;
>       err = qemu_strtoui(str, &endptr, 0, &res);
> +    g_assert_cmpint(err, ==, -ERANGE);
> +    g_assert_cmpint(res, ==, UINT_MAX);

Why cmpint and not cmpuint here?  (I see you’re using cmpint instead of 
cmpuint in many strtou* test functions below, too.)

[...]

> @@ -1325,31 +1697,67 @@ static void test_qemu_strtoul_max(void)

[...]

>   static void test_qemu_strtoul_underflow(void)
>   {
> -    const char *str = "-99999999999999999999999999999999999999999999";
> -    char f = 'X';
> -    const char *endptr = &f;
> -    unsigned long res = 999;
> +    const char *str;
> +    const char *endptr;
> +    unsigned long res;
>       int err;
>
> +    /* 1 less than -ULONG_MAX */
> +    str = ULONG_MAX == UINT_MAX ? "-4294967297" : "-18446744073709551617";

Technically these are 2 less than -ULONG_MAX, not 1 less.

Hanna