Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 10/12/10 17:52, Markus Armbruster wrote:
> Still not entirely happy, but maybe we can commit it as is, and fix it
> up later.

No worries, I think this is the most serious review I have ever received
for any piece of code, but you're finding valid points so it's good. If
all of QEMU had been reviewed like this we would be in really good shape :)

>> The following suffixes are supported:
>> B/b = bytes
>> K/k = KB
>> M/m = MB
>> G/g = GB
>> T/t = TB
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Would be nice if commit message documented that this affects -numa and
> -m.  In particular that they now accept more suffixes than before.

Will address this in the commit message.

>> +/*
>> + * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
>> + * M/m for MB, G/g for GB or T/t for TB. Default without any postfix
>> + * is MB. End pointer will be returned in *end, if end is valid.
> 
> Nitpick: There are plenty of invalid pointers we'll happily attempt to
> use.  "unless end is null" would be more precise.

Fixed

>> +    errno = 0;
>> +    val = strtod(nptr, &endptr);
>> +    if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
>> +        val == HUGE_VAL) {
> 
> ISO C permits implementations supporting infinities to make HUGE_VAL
> *not* +inf.  So this may not catch +inf.  val >= HUGE_VAL would.
> 
> But since we have to catch val * mul out of range further down anyway,
> the check for HUGE_VAL may be redundant here.

Valid point, fixed in the upcoming version.

>> +    c = *endptr++;
>> +    if (isspace(c) || c == '\0') {
>> +        c = 0;
>> +    } else if (!isspace(*endptr) && *endptr != 0) {
>> +        goto fail;
>> +    }
> 
> I'm not happy with this check.
> 
> If the caller needs a complete string consumed, then this check is
> insufficient, because it doesn't catch trailing garbage as long as it
> starts with whitespace.  The caller still needs to check !*endptr.
> 
> If the caller needs to continue parsing after the value, and expects
> anything but whitespace there, it has to copy the value first.  Only
> easy if the value is followed by some delimiter that can't occur in the
> value.  Example: parse a size value from something of them form
> name=value,name=value...  Need to copy up to the next comma or end of
> string.
> 
> The check complicates the second case without really helping the first
> case.
> 
> Nevertheless, it's good enough for the uses in this patch series, so I'm
> not insisting on getting this changed now.

I hadn't thought of case #2, but I think that is pretty easy to handle
by accepting ',' as a separator as well. It's worth keeping in kind that
the old code didn't do anything with trailing garbage either, it was
silently ignored.

For case #1 then I think it's ok to just accept trailing garbage, the
old code would simply use strtoull and leave it at that.

>> +    tmpval = (val * mul);
>> +    if (tmpval >= ~(size_t)0) {
>> +        goto fail;
> 
> val * mul may exceed the range of int64_t tmpval, and then the
> assignment has undefined behavior.  Obvious way to avoid that:
> 
>     if (val * mul >= ~(size_t)0) {
>         goto fail;
>     }
>     retval = val * mul;

Good point, fixed.

Updated version coming up shortly.

Jes