Re: [PATCH 06/11] test-cutils: Add more coverage to qemu_strtosz

[Hanna Czenczek]

On 09.05.23 14:31, Hanna Czenczek wrote:
> On 08.05.23 22:03, Eric Blake wrote:
> > Add some more strings that the user might send our way.  In
> > particular, some of these additions include FIXME comments showing
> > where our parser doesn't quite behave the way we want.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >   tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 215 insertions(+), 11 deletions(-)
>
> I wonder: The plan is to have "1.5e+1k" be parsed as "1.5e" + endptr 
> == "+1k"; but "0x1p1" is not parsed at all (could be "0x1" + "p1"). Is 
> that fully intentional?
>
> (Similarly, "1.1.k" is also not parsed at all, but the problem there 
> is not just two decimal points, but also that "1.1" would be an 
> invalid size in itself, so it really shouldn’t be parsed at all.)
>
> I don’t think it matters to users, really, but I still wonder.
>
> > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> > index afae2ee5331..9fa6fb042e8 100644
> > --- a/tests/unit/test-cutils.c
> > +++ b/tests/unit/test-cutils.c
>
> [...]
>
> > @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void)
> >       err = qemu_strtosz(str, NULL, &res);
> >       g_assert_cmpint(err, ==, -EINVAL);
> >       g_assert_cmphex(res, ==, 0xbaadf00d);
> > +
> > +    /* FIXME overflow in fraction is buggy */
> > +    str = "1.5E999";
> > +    endptr = NULL;
> > +    res = 0xbaadf00d;
> > +    err = qemu_strtosz(str, &endptr, &res);
> > +    g_assert_cmpint(err, ==, 0);
> > +    g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */);

So…  I have no idea what happens here but this always fails with 
“assertion failed (res == EiB): (1 == 1152921504606846976)”.  But when I 
replace the EiB by 1, it suddenly fails with “assertion failed (res == 
1): (1152921504606846976 == 1)” instead.  Replacing the EiB by anything 
but 1 also tells me that res is 1.

Now, here’s the kicker.  I put an `fprintf(stderr, "res == %" PRIu64 
"\n", res);` before this g_assert_cmpuint() (changed to (res, ==, 1))…  
And it passes.

Sometimes I really want to change professions.

(Of note is that changing the g_assert() below into a g_assert_true() 
also has g_assert_cmpuint(res, ==, 1) pass.)

> > +    g_assert(endptr == str + 9 /* FIXME + 4 */);
>
> This is “correct” (i.e. it’s the value we’ll get right now, which is 
> the wrong one), but gcc complains that the array index is out of 
> bounds (well...), which breaks the build.

Oh, it also isn’t correct, I think it needs to be str + 8.  As a bonus, 
the compiler doesn’t complain then (for some reason…?  it still seems 
out of bounds).

(Otherwise, to get around the complaint, I used 
g_assert_cmphex((uintptr_t)endptr, ==, (uintptr_t)str + 8).  Which is 
another thing, patch 1 explained to me that we shouldn’t use g_assert() :))

Hanna