[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 2/3] quorum: implement bdrv_add_child() and
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v10 2/3] quorum: implement bdrv_add_child() and bdrv_del_child() |
Date: |
Wed, 9 Mar 2016 16:27:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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