qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Hanna Czenczek
Subject: Re: [PATCH 06/11] test-cutils: Add more coverage to qemu_strtosz
Date: Tue, 9 May 2023 14:42:57 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]