On Fri, May 20, 2011 at 4:25 PM, Stefan Schmidt wrote:
>> -static int parse_vendprod(struct usb_vendprod *vp, const char *str)
>> +static void parse_vendprod(u_int16_t *vendor, u_int16_t *product, const
>> char *str)
>> {
>> - unsigned long vend, prod;
>> const char *colon;
>>
>> + *vendor = strtoul(str, NULL, 16);
>> colon = strchr(str, ':');
>> - if (!colon || strlen(colon) < 2)
>> - return -EINVAL;
>> -
>> - vend = strtoul(str, NULL, 16);
>> - prod = strtoul(colon+1, NULL, 16);
>> -
>> - if (vend > 0xffff || prod > 0xffff)
>> - return -EINVAL;
>
> Any reason you removed the error handling here?
The values are stuffed directly into *vendor and *product which are
both u_int16_t so the value can not be higher than 0xffff in any case.
>> case 'd':
>> - /* Parse device */
>> - if (parse_vendprod(&vendprod, optarg) < 0) {
>> - fprintf(stderr, "unable to parse `%s' as a
>> vendor:product\n", optarg);
>> -
>> - exit(2);
>> - }
>> - dif->vendor = vendprod.vendor;
>> - dif->product = vendprod.product;
>> - dif->flags |= (DFU_IFF_VENDOR | DFU_IFF_PRODUCT);
>> + /* Parse device ID */
>> + parse_vendprod(&dif->vendor, &dif->product, optarg);
>
> Please keep the error handling like the former code.
For the same reason, parse_vendprod can never fail. If the user
provides rubbish, the values will be zero, which is equivalent to not
being filtered on. I instead added the "Filter on" message below so
that the user can verify his filter expression was recognized.
One could look at the field length parsed by strtoul() by passing a
"endptr" instead of NULL as second argument, and from there verify
that the whole string provided by the user was recognized. However
this will complicate this small code quite a lot and is IMO not worth
it. Even if the user gets this wrong, there is not much harm that can
be done, either no devices will be found because of wrong filter, or
more than one device because of no filter, which will stop the
program.
The only case in which the old code would detect an error was numeric
overflow (or missing value after the colon, which now is a valid
case), so I don't consider it much of a loss.
>
>> + printf("Filter on vendor = 0x%04x product = 0x%04x\n",
>> dif->vendor, dif->product);
>> + if (dif->vendor)
>> + dif->flags |= DFU_IFF_VENDOR;
>> + if (dif->product)
>> + dif->flags |= DFU_IFF_PRODUCT;
>> break;
>> case 'p':
>> /* Parse device path */
Cheers,
Tormod
_______________________________________________
devel mailing list
devel@lists.openmoko.org
https://lists.openmoko.org/mailman/listinfo/devel
