[PATCH 0/2] Minor integer parsing improvements

[PATCH 0/2] Minor integer parsing improvements

[Markus Armbruster]

Not for 4.2, of course.

Markus Armbruster (2):
  util/cutils: Turn FIXME comment into QEMU_BUILD_BUG_ON()
  test-keyval: Tighten test of trailing crap after size

 tests/test-keyval.c | 2 +-
 util/cutils.c       | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH 1/2] util/cutils: Turn FIXME comment into QEMU_BUILD_BUG_ON()

[Markus Armbruster]

qemu_strtoi64() assumes int64_t is long long.  This is marked FIXME.
Replace by a QEMU_BUILD_BUG_ON() to avoid surprises.

Same for qemu_strtou64().

Fix a typo in qemu_strtoul()'s contract while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/cutils.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/util/cutils.c b/util/cutils.c
index fd591cadf0..b372dd3e68 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -502,7 +502,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base,
  * Convert string @nptr to an int64_t.
  *
  * Works like qemu_strtol(), except it stores INT64_MAX on overflow,
- * and INT_MIN on underflow.
+ * and INT64_MIN on underflow.
  */
 int qemu_strtoi64(const char *nptr, const char **endptr, int base,
                  int64_t *result)
@@ -517,8 +517,9 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
         return -EINVAL;
     }
 
+    /* This assumes int64_t is long long TODO relax */
+    QEMU_BUILD_BUG_ON(sizeof(int64_t) != sizeof(long long)); 
     errno = 0;
-    /* FIXME This assumes int64_t is long long */
     *result = strtoll(nptr, &ep, base);
     return check_strtox_error(nptr, ep, endptr, errno);
 }
@@ -541,8 +542,9 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
         return -EINVAL;
     }
 
+    /* This assumes uint64_t is unsigned long long TODO relax */
+    QEMU_BUILD_BUG_ON(sizeof(uint64_t) != sizeof(unsigned long long));
     errno = 0;
-    /* FIXME This assumes uint64_t is unsigned long long */
     *result = strtoull(nptr, &ep, base);
     /* Windows returns 1 for negative out-of-range values.  */
     if (errno == ERANGE) {
-- 
2.21.0

Re: [PATCH 1/2] util/cutils: Turn FIXME comment into QEMU_BUILD_BUG_ON()

[Philippe Mathieu-Daudé]

On 11/25/19 2:38 PM, Markus Armbruster wrote:
> qemu_strtoi64() assumes int64_t is long long.  This is marked FIXME.
> Replace by a QEMU_BUILD_BUG_ON() to avoid surprises.
> 
> Same for qemu_strtou64().
> 
> Fix a typo in qemu_strtoul()'s contract while there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   util/cutils.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index fd591cadf0..b372dd3e68 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -502,7 +502,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base,
>    * Convert string @nptr to an int64_t.
>    *
>    * Works like qemu_strtol(), except it stores INT64_MAX on overflow,
> - * and INT_MIN on underflow.
> + * and INT64_MIN on underflow.
>    */
>   int qemu_strtoi64(const char *nptr, const char **endptr, int base,
>                    int64_t *result)
> @@ -517,8 +517,9 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
>           return -EINVAL;
>       }
>   
> +    /* This assumes int64_t is long long TODO relax */
> +    QEMU_BUILD_BUG_ON(sizeof(int64_t) != sizeof(long long));

Ready for 128-bit!
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       errno = 0;
> -    /* FIXME This assumes int64_t is long long */
>       *result = strtoll(nptr, &ep, base);
>       return check_strtox_error(nptr, ep, endptr, errno);
>   }
> @@ -541,8 +542,9 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
>           return -EINVAL;
>       }
>   
> +    /* This assumes uint64_t is unsigned long long TODO relax */
> +    QEMU_BUILD_BUG_ON(sizeof(uint64_t) != sizeof(unsigned long long));
>       errno = 0;
> -    /* FIXME This assumes uint64_t is unsigned long long */
>       *result = strtoull(nptr, &ep, base);
>       /* Windows returns 1 for negative out-of-range values.  */
>       if (errno == ERANGE) {
> 

[PATCH 2/2] test-keyval: Tighten test of trailing crap after size

[Markus Armbruster]

test_keyval_visit_size() should test for trailing crap after size with
and without suffix.  It does test the latter: "sz2=16Gi" has size
"16G" followed by crap "i".  It fails to test the former "sz1=16E" is
a syntactically valid size that overflows uint64_t.  Replace by
"sz1=0Z".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-keyval.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 09b0ae3c68..e331a84149 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -478,7 +478,7 @@ static void test_keyval_visit_size(void)
     visit_free(v);
 
     /* Trailing crap */
-    qdict = keyval_parse("sz1=16E,sz2=16Gi", NULL, &error_abort);
+    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
-- 
2.21.0

Re: [PATCH 2/2] test-keyval: Tighten test of trailing crap after size

[Eric Blake]

On 11/25/19 7:38 AM, Markus Armbruster wrote:
> test_keyval_visit_size() should test for trailing crap after size with
> and without suffix.  It does test the latter: "sz2=16Gi" has size
> "16G" followed by crap "i".  It fails to test the former "sz1=16E" is
> a syntactically valid size that overflows uint64_t.  Replace by
> "sz1=0Z".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-keyval.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/test-keyval.c b/tests/test-keyval.c
> index 09b0ae3c68..e331a84149 100644
> --- a/tests/test-keyval.c
> +++ b/tests/test-keyval.c
> @@ -478,7 +478,7 @@ static void test_keyval_visit_size(void)
>       visit_free(v);
>   
>       /* Trailing crap */
> -    qdict = keyval_parse("sz1=16E,sz2=16Gi", NULL, &error_abort);
> +    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);

Does this actually test both failure cases, or does it abort the parse 
after the first failure (sz1=0Z) without ever hitting the second half of 
the parse (sz2=16Gi)?

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

Re: [PATCH 2/2] test-keyval: Tighten test of trailing crap after size

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/25/19 7:38 AM, Markus Armbruster wrote:
>> test_keyval_visit_size() should test for trailing crap after size with
>> and without suffix.  It does test the latter: "sz2=16Gi" has size
>> "16G" followed by crap "i".  It fails to test the former "sz1=16E" is
>> a syntactically valid size that overflows uint64_t.  Replace by
>> "sz1=0Z".
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/test-keyval.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/test-keyval.c b/tests/test-keyval.c
>> index 09b0ae3c68..e331a84149 100644
>> --- a/tests/test-keyval.c
>> +++ b/tests/test-keyval.c
>> @@ -478,7 +478,7 @@ static void test_keyval_visit_size(void)
>>       visit_free(v);
>>         /* Trailing crap */
>> -    qdict = keyval_parse("sz1=16E,sz2=16Gi", NULL, &error_abort);
>> +    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);
>
> Does this actually test both failure cases, or does it abort the parse
> after the first failure (sz1=0Z) without ever hitting the second half
> of the parse (sz2=16Gi)?

Fair question!  Short answer: yes, we check both.

Long answer follows.

       /* Trailing crap */
       qdict = keyval_parse("time1=89ks,time2=ns", NULL, &error_abort);

keyval_parse() must succeed: it takes &error_abort.

        v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
        qobject_unref(qdict);

Can't fail.

        visit_start_struct(v, NULL, NULL, 0, &error_abort);

Must succeed.

        visit_type_size(v, "sz1", &sz, &err);
        error_free_or_abort(&err);

This is where we parse "0Z".  Must fail.  We continue.

        visit_type_size(v, "sz2", &sz, &err);
        error_free_or_abort(&err);

This is where we parse "16Gi".  Must fail.  We continue.

        visit_end_struct(v, NULL);
        visit_free(v);
    }

Clear now?

Re: [PATCH 2/2] test-keyval: Tighten test of trailing crap after size

[Eric Blake]

On 11/25/19 9:31 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/25/19 7:38 AM, Markus Armbruster wrote:
>>> test_keyval_visit_size() should test for trailing crap after size with
>>> and without suffix.  It does test the latter: "sz2=16Gi" has size
>>> "16G" followed by crap "i".  It fails to test the former "sz1=16E" is
>>> a syntactically valid size that overflows uint64_t.  Replace by
>>> "sz1=0Z".
>>>

>>>          /* Trailing crap */
>>> -    qdict = keyval_parse("sz1=16E,sz2=16Gi", NULL, &error_abort);
>>> +    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);
>>
>> Does this actually test both failure cases, or does it abort the parse
>> after the first failure (sz1=0Z) without ever hitting the second half
>> of the parse (sz2=16Gi)?
> 
> Fair question!  Short answer: yes, we check both.

Aha - keyval_parse() just sets up the parser, while the check for double 
failures is in the test code below.

> Clear now?

Yes.
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 2/2] test-keyval: Tighten test of trailing crap after size

[Laurent Vivier]

Le 25/11/2019 à 14:38, Markus Armbruster a écrit :
> test_keyval_visit_size() should test for trailing crap after size with
> and without suffix.  It does test the latter: "sz2=16Gi" has size
> "16G" followed by crap "i".  It fails to test the former "sz1=16E" is
> a syntactically valid size that overflows uint64_t.  Replace by
> "sz1=0Z".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-keyval.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/test-keyval.c b/tests/test-keyval.c
> index 09b0ae3c68..e331a84149 100644
> --- a/tests/test-keyval.c
> +++ b/tests/test-keyval.c
> @@ -478,7 +478,7 @@ static void test_keyval_visit_size(void)
>      visit_free(v);
>  
>      /* Trailing crap */
> -    qdict = keyval_parse("sz1=16E,sz2=16Gi", NULL, &error_abort);
> +    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);
>      v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>      qobject_unref(qdict);
>      visit_start_struct(v, NULL, NULL, 0, &error_abort);
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH 0/2] Minor integer parsing improvements

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191125133846.27790-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/2] Minor integer parsing improvements
Type: series
Message-id: 20191125133846.27790-1-armbru@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
f4a50eb test-keyval: Tighten test of trailing crap after size
1b74bc5 util/cutils: Turn FIXME comment into QEMU_BUILD_BUG_ON()

=== OUTPUT BEGIN ===
1/2 Checking commit 1b74bc5bc45a (util/cutils: Turn FIXME comment into QEMU_BUILD_BUG_ON())
ERROR: trailing whitespace
#35: FILE: util/cutils.c:523:
+    QEMU_BUILD_BUG_ON(sizeof(int64_t) != sizeof(long long)); $

total: 1 errors, 0 warnings, 28 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit f4a50eb158af (test-keyval: Tighten test of trailing crap after size)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191125133846.27790-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com