Hello.
On Fri, 2011-05-20 at 17:04, Tormod Volden wrote:
> 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.
I stand corrected. Bigger then 0xffff for 2 byte would be quite
strange. ;)
> >> 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.
Agreed. Thanks for explaining it. I applied, tested and pushed all
four patches now.
regards
Stefan Schmidt
_______________________________________________
devel mailing list
devel@lists.openmoko.org
https://lists.openmoko.org/mailman/listinfo/devel
