Re: [Qemu-devel] [PATCH v4 1/5] Use qemu_strtoul instead of strtol

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 01/09/2016 01:35 AM, Eric Blake wrote:
> On 01/08/2016 01:37 AM, Cao jin wrote:
>> strtol() don`t guarantee errno to be ERANGE on overflow.
>
> I stand slightly corrected: C99 requires ERANGE on overflow, but not
> EINVAL; it is POSIX that adds EINVAL, but does not properly require it.
>   At any rate, my main point was that errno is not always properly set by
> all strtol implementations, and furthermore that you can't rely on it
> being set to a sane value if you didn't pre-set it to 0.
>

Got you:)

>> This wrapper returns either -EINVAL or the errno set by strtol()
>> function (e.g -ERANGE).
>
> The subject line doesn't start with a topic.  Maybe a better commit
> message would be:
>
> xen: Use qemu_strtoul instead of strtol
>
> No need to roll our own (with slightly incorrect handling of errno),
> when we can use the common version.
>

ok, looks good.

>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/xen/xen-host-pci-device.c | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>
>>       buf[rc] = 0;
>> -    value = strtol(buf, &endptr, base);
>> -    if (endptr == buf || *endptr != '\n') {
>> -        rc = -1;
>> -    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
>> -        rc = -errno;
>> -    } else {
>> -        rc = 0;
>> +    rc = qemu_strtoul(buf, &endptr, base, &value);
>
> Why did you switch from strtol() to qemu_strtoul()?  Was signed parsing
> incorrect, and unsigned parsing a bug fix?  If so, please mention it in
> the commit message as intentional.  Otherwise, use qemu_strtol() (and
> adjust the commit message accordingly).
>

Yes, I should mention it, sorry for forgetting to explain it. Because 
what it want to read from host are values in pci device config space, 
which are non-negative, like "vendor ID", "device ID", "class code", 
etc. so unsigned parsing seems more suitable in this case. we can tell 
it also from the local variable "unsigned long value;"

-- 
Yours Sincerely,

Cao jin