Re: [Qemu-block] [PATCH v10 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Max Reitz]

On 08.03.2016 03:57, Changlong Xie wrote:
> On 03/08/2016 12:02 AM, Max Reitz wrote:
>> On 07.03.2016 17:02, Eric Blake wrote:
>>> On 03/05/2016 11:13 AM, Max Reitz wrote:
>>>
>>>>> +    index = atoi(child->name + 9);
>>>>
>>>> Optional: Assert absence of an error:
>>>>
>>>
>>> Indeed, atoi() is worthless, because it cannot do error detection.
>>>
>>>> unsigned long index;
>>>> char *endptr;
>>>>
>>>> index = strtoul(child->name + 9, &endptr, 10);
>>>> assert(index >= 0 && !*endptr);
>>>
>>> Still incorrect; you aren't handling errno properly for detecting all
>>> errors.  Even better is to use qemu_strtoul(), which already handles
>>> proper error detection.
>>
>> Yeah, I keep forgetting that it returns ULONG_MAX on range error...
> 
> Yes, we should limit the range to INT_MAX. How do you like the following
> codes, i just steal it from xen_host_pci_get_value().
> 
> int rc;
> const char *endptr;
> unsigned long value;
> 
> assert(!strncmp(child->name, "children.", 9));
> rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);

Passing NULL instead of &endptr will make qemu_strtoul() check that the
string passed to it (child->name + 9) only consists of a number; which
should be true here, so you can do that (pass NULL instead of &endptr).

> if (!rc) {
>     assert(value <= INT_MAX);
>     index = value;
> } else {
>     error_setg_errno(errp, -rc, "Failed to parse value '%s'",
>                      child->name + 9);
>     return;
> }

You could simplify this as

assert(!rc && value <= INT_MAX);
index = value;

(It should be impossible for qemu_strtoul() to return an error here, so
an assert() is just as fine as a normal error.)

And you could get rid of the index = value assignment by making index an
unsigned long and replacing all instances of "value" by "index".

Max

> 
> Thanks
>     -Xie
> 
>>
>> Max
>>
> 
>

signature.asc
Description: OpenPGP digital signature