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

[Eric Blake]

On Fri, May 19, 2023 at 04:27:10PM +0200, Hanna Czenczek wrote:
> 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.)

Probably a combination of copy-paste vs rebase patch re-ordering.
Yes, all test_*strtoui* tests should be using
g_assert_cmpuint(res...).  Will fix.

> 
> [...]
> 
> > @@ -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.

Indeed.  Both constants should end in 6, not 7.  Will fix.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org