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

[Hanna Czenczek <hreitz@redhat.com>]

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