Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[Markus Armbruster <armbru@redhat.com>]

David Hildenbrand <david@redhat.com> writes:

> On 20.11.18 21:41, Eric Blake wrote:
>> On 11/20/18 2:31 PM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> 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 <david@redhat.com>
>>>>> ---
>>>>>    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 <eblake@redhat.com>
>>>
>>> Thanks for your analysis, Eric.
>>>
>>> With the typo fixes:
>> 
>> Including the fix of my attempt at a typo fix :)
>> 
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks!