[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes |
Date: |
Wed, 21 Nov 2018 15:16:39 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
David Hildenbrand <address@hidden> writes:
> On 20.11.18 21:41, Eric Blake wrote:
>> On 11/20/18 2:31 PM, Markus Armbruster wrote:
>>> Eric Blake <address@hidden> writes:
>>>
>>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>>> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>>>>
>>>> s/inifities/infinities/
>>>>
>>>>> They shouldn't. Fix that.
>>>>>
>>>>> The fix makes use of qemu_strtod_finite(). To avoid ugly casts,
>>>>> change the @end parameter of qemu_strtosz() & friends from char **
>>>>> to const char **.
>>>>>
>>>>> Also, add two test cases, testing that "inf" and "NaN" are properly
>>>>> rejected.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>>> ---
>>>>> include/qemu/cutils.h | 6 +++---
>>>>> monitor.c | 2 +-
>>>>> tests/test-cutils.c | 24 +++++++++++++++++-------
>>>>> util/cutils.c | 16 +++++++---------
>>>>> 4 files changed, 28 insertions(+), 20 deletions(-)
>>>>>
>>>>
>>>>> +++ b/util/cutils.c
>>>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>>>> * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>>>
>>>> Pre-existing, but since you're touching this area: the second 'Return'
>>>> is unusual capitalization for being mid-sentence. You could even
>>>> s/Return/of/
>>>
>>> "of"?
>>
>> "or" (ouch - wrong time for my fingers to be slipping on the keyboard)
>
> Shouldn't that be "and" and s/Return/Returns/
>
>
> "Returns -ERANGE on overflow and -EINVAL on other errors".
I prefer imperative mood for function contracts: "Convert string to
bytes", "Return -ERANGE on overflow", and so forth.
Other than that, I like your phrasing. I'd put a comma before "and",
though.
> I can include that fixup (whetever version you guys prefer)
>
>>
>>
>>>> It's some hairy code to think about, but I can't find anything wrong
>>>> with it. Typo fixes are minor, so
>>>>
>>>> Reviewed-by: Eric Blake <address@hidden>
>>>
>>> Thanks for your analysis, Eric.
>>>
>>> With the typo fixes:
>>
>> Including the fix of my attempt at a typo fix :)
>>
>>> Reviewed-by: Markus Armbruster <address@hidden>
>
> Thanks!
- Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, (continued)
- Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, Eric Blake, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, Eric Blake, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, Markus Armbruster, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, David Hildenbrand, 2018/11/21
- Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, Markus Armbruster, 2018/11/21
[Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 7/9] test-string-input-visitor: Use virtual walk, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 9/9] test-string-input-visitor: Add range overflow tests, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 8/9] test-string-input-visitor: Split off uint64 list tests, David Hildenbrand, 2018/11/20