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
